580431 - unicode utilities for mozmill and jsbridge

Status: RESOLVED DUPLICATE of bug 506760

(Testing Graveyard :: Mozmill, enhancement)

Description

[Jeff Hammel]

Attachment: file utilities

adriank has written some nice utility functions for mozmill and jsbridge to help deal with unicode.  they should be reviewed and included at some point

Comment 1

[Jeff Hammel]

Attachment: jsbridge server utilities for unicode

here's the other one

Comment 2

[Jeff Hammel]

confirmed;  neither of these are applied

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 458824 [details] [diff] [review]
file utilities

>+    var file = Components.classes["@mozilla.org/file/local;1"]
>+            .createInstance(Components.interfaces.nsILocalFile);

To follow the code convention the leading dot of the 2nd line should be placed at the end of the first line and we don't indent with classes but Components. Don't we have Cc, and Ci defined here?

>+    var uri = ios.newFileURI(unicodeFile).spec;

this is not an URI but an URL. Would be good to make that clear here.

>+  var foStream = Components.classes["@mozilla.org/network/file-output-stream;1"]
>+                 .createInstance(Components.interfaces.nsIFileOutputStream);

snip

>+  var converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"]
>+                         .createInstance(Components.interfaces.nsIScriptableUnicodeConverter);

snip.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 458825 [details] [diff] [review]
jsbridge server utilities for unicode

>+      this.outstream = Cc['@mozilla.org/intl/converter-output-stream;1']
>+                    .createInstance(Ci.nsIConverterOutputStream);
>+      this.outstream.init(this.outputstream, 'UTF-8', 1024,
>+                    Ci.nsIConverterOutputStream.DEFAULT_REPLACEMENT_CHARACTER);
>       this.stream = transport.openInputStream(0, 0, 0);
>       this.instream = Cc['@mozilla.org/intl/converter-input-stream;1']
>           .createInstance(Ci.nsIConverterInputStream);

Please correct the indentation here too.

Comment 5

[Jeff Hammel]

Looking over these patches more closely, I'm not at all sure that this fixes or helps with anything.  While some of the utilities may be useful (although I'm not sure we need it now, I'm not 100% sure what problem they solve).

The JS bridge patch may be necessary.  That looks like something possibly important.  However, I'd like for the original author to comment on what it is supposed to do (and bonus points for documenting this behaviour in the code).

The patch to mozmill, however, I doubt provides the functionality that I assume it is supposed to provide -- namely, working around bug 559814.  It encodes a test file to a temporary unicode file and then loads this with mozIJSSubscriptLoader.  However, because of bug 377498, this won't work unless I'm mising a huge piece of the puzzle (See discussion on both of these bugs). The failing test case on bug 559814 *is* already in utf-8.  It is the subscript loader which insists on interpreting as utf-8.  Again, see both bugs for discussion.

To summarize:
 * the jsbridge patch looks important;  however, I'd really like the author (harth?) to comment on what problem this solves and if we need it

 * while the mozmill patch introduces some useful utilities, I don't think it actually solves any problems.  if our tests were in latin-1, there would be some utility in converting them to utf-8.  however this still wouldn't solve bug 559814 because of bug 377498 and I don't think (though I could be wrong) it solves any problems we have

Comment 6

[Adrian Kalla [:adriank]]

(In reply to comment #0)
> adriank has written some nice utility functions for mozmill and jsbridge to
> help deal with unicode.  they should be reviewed and included at some point

a small correction: the credits and copyrights go to Heather and Clint.
I just pulled that patches from Heathers repository ;)

Comment 7

[Jeff Hammel]

After conversations on IRC, it sounds like these patches are part of bug 506760. Moving the patches there and marking this as a duplicate.