959309 - Finish JSMime 0.2 and land it on comm-central

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Joshua Cranmer [:jcranmer]]

I don't have all the pieces quite ready for review yet, but the steps are:
1. Get JSMime 0.2 reviewed and push it to github/mozilla-comm/jsmime
2. Import JSMime 0.2 into comm-central
3. Import various fixes due to changed API of structured headers
4. Use JSMime for header parsing (bug 858337 and bug 790855)

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: JSMime 0.2

I'd try to do a patch that represents diffs to this code from the current version in comm-central, but the starting point of my scratch repository is an accumulation of about 5 patches, and then I did code motion, function renaming, and some reformatting, so even the little bit of it that is still the same is rather affected by some major patching.

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Import JSMime 0.2 into comm-central

For clarity, the changes to mailnews/mime/jsmime are a blanket import from jsmime, using a script I'll upload shortly. For blame purposes, I tweaked the results to record copies of data from mailnews/test/data and to handle some renames of files after the fact (which turned out to be slightly less useful than I thought). This applies on top of bug 842632, although there shouldn't be any conflicts with reordering these two patches.

test_parser.js was rearranged to be a test specifically of MimeParser's helpers instead of the internal parser tests itself (which are copied and better-expanded on in jsmime/test). The parts that were deleted are still retained in the tests there, so we're not really losing any test coverage.

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: JSMime import script

For expository purposes.

Comment 4

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8359887 [details] [diff] [review]
Import JSMime 0.2 into comm-central

Review of attachment 8359887 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a fan of APIs that change behaviour based on options parameters; I find them hard to use and maintain because there's an extra cognitive load in keeping track of what the options are during any given call, and there's always a risk of bad behaviour if someone calls with an unexpected combination of options. Also, there's a risk that a patch fixing issues with one set of options will break some other set of options. I'm not quite at the level of r- for that issue, but I encourage you to look for places where we can provide fewer alternatives in the APIs.

So, the r- is just for cleanup of the for..of loop issues, and particularly the [value, key] vs. [key, value] iterator, and hopefully a fix of the "quote entire strings" I point out in test_header_emitter.js.

::: mailnews/mime/jsmime/LICENSE
@@ +1,3 @@
> +Copyright (c) 2013 Joshua Cranmer
> +
> +Permission is hereby granted, free of charge, to any person obtaining a copy

I'd like a separate opinion (Gerv, maybe?) on including this license - it looks fine to me, but I know enough to know that isn't worth much. I understand that this is an "import and use a 3rd party library" situation, so I don't expect a problem.

::: mailnews/mime/jsmime/jsmime.js
@@ +1,1 @@
> +(function (root, fn) {

Do we want a copyright notice (at least by reference) in every file? I know we do when the content is copyright Mozilla.

@@ +179,5 @@
> +  let type = mediatype + '/' + subtype;
> +  let structure = new Map();
> +  structure.mediatype = mediatype;
> +  structure.subtype = subtype;
> +  structure.type = type;

It looks a bit odd to me to have both JS attributes and .set() values on a Map; if you're going to keep this you should document it well so it doesn't surprise others.

@@ +180,5 @@
> +  let structure = new Map();
> +  structure.mediatype = mediatype;
> +  structure.subtype = subtype;
> +  structure.type = type;
> +  params.forEach(function (value, name) {

I was going to ask for

for (let [value, name] of params) {
 ...
}

here, since we're requiring ES6 already, but according to MDN support is weak outside Mozilla, so I guess we should leave it as Array.forEach(...). Though, given that you're using function*(), Set, and Map, for..of isn't much of a stretch.

@@ +302,5 @@
> + * @param {Boolean} [opts.qstring]  If true, recognize quoted strings.
> + * @param {Boolean} [opts.dliteral] If true, recognize domain literals.
> + * @param {Boolean} [opts.comments] If true, recognize comments.
> + * @param {Boolean} [opts.rfc2047]  If true, parse and decode RFC 2047
> + *                                  encoded-words.

I'd prefer to have separate routines that parse the specific cases we want, rather than one complex routine with tricky interacting options. Among other things, that allows us to explicitly not provide combinations that don't make sense, which may help reduce bugs caused by confused callers.

@@ +494,5 @@
> +  // Only attempt to convert the headerValue if it contains non-ASCII
> +  // characters.
> +  if (/[\x80-\xff]/.exec(headerValue)) {
> +    // First convert the value to a typed-array for TextDecoder.
> +    let typedarray = mimeutils.stringToTypedArray(headerValue);

Would it mess up too many APIs to always pass around undecoded data (i.e. raw octets to/from the network) as Uint8Array rather than JS String? It would make the undecoded/decoded state of the data obvious (perhaps painfully so).

@@ +581,5 @@
> +      return false;
> +    }
> +
> +    // Make the buffer be a typed array for what follows
> +    buffer = mimeutils.stringToTypedArray(buffer);

Why not have mimeutils.decode_*() return Uint8Array? Similar to the earlier comment, this makes it obvious we're handling raw octets rather than meaningful JS strings.

@@ +677,5 @@
> + */
> +function parseAddressingHeader(header, doRFC2047) {
> +  // Default to true
> +  if (doRFC2047 === undefined)
> +    doRFC2047 = true;

ES6 allows you to declare the default value:

function parseAddressingHeader(header, doRFC2047 = true) {

but as with the earlier comment, I'm not a fan of flags that substantially change API behaviour. Does the real world ever require doRFC2047===false? I only see it in your test cases.

@@ +989,5 @@
> + *
> + * @param {String} value The RFC 2231-encoded string to decode.
> + * @return The Unicode version of the string.
> + */
> +function decode2231Value(value) {

I don't like that this routine is passed a parameter that's part String (outside the quotes) and part octets (inside the quotes), though it would take quite a bit of restructuring and maybe flag-driven-behaviour removal from the caller to resolve this.

@@ +1326,5 @@
> +   * @private
> +   */
> +  this._cachedHeaders = new Map();
> +  Object.defineProperty(this, "rawHeaderText",
> +    {get: function () { return rawHeaders; }});

Name the StructuredHeaders parameter 'rawHeaderText' so it's a little clearer that it is different from _rawHeaders

@@ +1448,5 @@
> +
> +/**
> + * An equivalent of Map.@@iterator, applied to the structured header
> + * representations. This is the function that makes
> + * for (let [header, key] of headers) work properly.

Iterator should return [key, value] rather than [value, key] (oddly, the code below looks to me like it actually returns [key, value] but I see callers that expect [value, key], so I'm not sure what's going on)

@@ +1453,5 @@
> + */
> +StructuredHeaders.prototype[Symbol.iterator] = function*() {
> +  // Iterate over all the raw headers, and use the cached headers to retrieve
> +  // them.
> +  for (let [header, value] of this._rawHeaders) {

Since you don't use 'value' in this loop,

for (let [headerName, ] of this._rawHeaders)

or

for (let headerName of this._rawHeaders.keys())

@@ +1492,5 @@
> + * An equivalent of Map.values, applied to the structured header
> + * representations.
> + */
> +StructuredHeaders.prototype.values = function* () {
> +  for (let [header, value] of this) {

for (let [, value] of this) {

@@ +1543,5 @@
> + *    strformat: one of {binarystring, unicode, typedarray} [default=binarystring]
> + *      How to treat output strings:
> + *        binarystring: Data is a JS string with chars in the range [\x00-\xff]
> + *        unicode: Data for text parts is converted to UTF-16; data for other
> + *          parts is a typed array buffer, akin to typedarray.

Can we get away with only supporting your 'unicode' option here, rather than requiring callers to figure out which one to use?

@@ +2269,5 @@
> + *     not including the final CRLF pair. If this count would be exceeded, then
> + *     an error will be thrown and encoding will not be possible.
> + *   @param [options.useASCII=true] {Boolean}
> + *     If true, then RFC 2047 and RFC 2231 encoding of headers will be performed
> + *     as needed to retain headers as ASCII.

Do we ever *not* want to fully encode headers to wire format?

@@ +2574,5 @@
> +    for (let i = 0; i < encodedText.length; i++) {
> +      if (encodedText[i] < 0x20 || encodedText[i] >= 0x7F ||
> +          qpForbidden.contains(binaryString[i])) {
> +        let ch = encodedText[i];
> +        let hexString = "0123456789abcdef";

Not sure if this is a meaningless micro-optimization, but make this const HEXSTRING = "..." outside the function definition.

@@ +2595,5 @@
> + *
> + * @protected
> + * @param {String}  text          The text to add to the output.
> + * @param {Boolean} mayBreakAfter If true, the current position in the output is a
> + *                      candidate for inserting a line break.

Rather than "current position", say "the position after the text is added to the output is a candidate..."

@@ +2778,5 @@
> +};
> +
> +/**
> + * Add an unstructured header value to the output. This effectively means only
> + * inserting line breaks were necessary, and using RFC 2047 encoding where

s/were/where

::: mailnews/mime/jsmime/test/test_header_emitter.js
@@ +51,5 @@
> +      [[{name: "", email: "hi![e-mail]"}], "\"hi!bad\"@all.com"],
> +      [[{name: "", email: "\"hi!bad\"@all.com"}], "\"hi!bad\"@all.com"],
> +      [[{name: "Doe, John", email: "[e-mail]"}], "\"Doe, John\" <[e-mail]>"],
> +      [[{name: "A really, really long name to quote", email: "[e-mail]"}],
> +        "A \"really,\" really long name\r\n to quote <[e-mail]>"],

This is a bit odd - while it may be legal, it looks better to quote the entire name field if any part of it needs to be quoted.

Comment 5

[Joshua Cranmer [:jcranmer]]

(In reply to :Irving Reid from comment #4)
> ::: mailnews/mime/jsmime/jsmime.js
> @@ +1,1 @@
> > +(function (root, fn) {
> 
> Do we want a copyright notice (at least by reference) in every file? I know
> we do when the content is copyright Mozilla.

The jsmime.js file is produced by a script; the script doesn't add a copyright block, so I figured that keeping the LICENSE file in the source directory would be sufficient to express copyright claims (hence why I included that in the input).

> @@ +179,5 @@
> > +  let type = mediatype + '/' + subtype;
> > +  let structure = new Map();
> > +  structure.mediatype = mediatype;
> > +  structure.subtype = subtype;
> > +  structure.type = type;
> 
> It looks a bit odd to me to have both JS attributes and .set() values on a
> Map; if you're going to keep this you should document it well so it doesn't
> surprise others.

This pattern was suggested to me by JS developers when I talked about the desirability of having non-enumerable parameters to the map. I've documented this property in the comments on parseStructuredHeader (see <https://github.com/jcranmer/jsmime/blob/master/headerparser.js#L852>) (I couldn't think of any other public method to attach documentation to).

> I was going to ask for
> 
> for (let [value, name] of params) {
>  ...
> }
> 
> here, since we're requiring ES6 already, but according to MDN support is
> weak outside Mozilla, so I guess we should leave it as Array.forEach(...).
> Though, given that you're using function*(), Set, and Map, for..of isn't
> much of a stretch.

Hooo boy, this requires some background. I don't want to tie this code specifically to SpiderMonkey. Naturally, I therefore attempted to get it working on node.js (and therefore v8) so I could use those test frameworks as the basis for an automated testsuite. However, v8 has been really lagging in ES6 support, so I started changing less necessary features as I came across error messages. And then I kept realizing that v8 was extremely lacking in the necessary features--its Maps have neither for-of support nor .forEach support and I gave up. Since forEach is mockable whereas for-of isn't, I switched to .forEach in a few places but not universally. This should explain why, e.g., I only sometimes use let-destructuring (it depends on if I went to change it before or after I gave up trying to get it to work on v8 for the nth time).

> @@ +302,5 @@
> > + * @param {Boolean} [opts.qstring]  If true, recognize quoted strings.
> > + * @param {Boolean} [opts.dliteral] If true, recognize domain literals.
> > + * @param {Boolean} [opts.comments] If true, recognize comments.
> > + * @param {Boolean} [opts.rfc2047]  If true, parse and decode RFC 2047
> > + *                                  encoded-words.
> 
> I'd prefer to have separate routines that parse the specific cases we want,
> rather than one complex routine with tricky interacting options. Among other
> things, that allows us to explicitly not provide combinations that don't
> make sense, which may help reduce bugs caused by confused callers.

I disagree. I moved to the getHeaderTokens model because it helped me make a consistent parser for complex cases; it's also a strong deterrent from using incomprehensible regexes when parsing. Finally, I think it's also the better defense against subtle bugs: quoted string handling really needs to be shared, etc., and having one place where it all happens is better. The options just enable or disable specific possible productions and do not have trickier interaction than manually implementing a parser that detects various subsets of them.

> @@ +494,5 @@
> > +  // Only attempt to convert the headerValue if it contains non-ASCII
> > +  // characters.
> > +  if (/[\x80-\xff]/.exec(headerValue)) {
> > +    // First convert the value to a typed-array for TextDecoder.
> > +    let typedarray = mimeutils.stringToTypedArray(headerValue);
> 
> Would it mess up too many APIs to always pass around undecoded data (i.e.
> raw octets to/from the network) as Uint8Array rather than JS String? It
> would make the undecoded/decoded state of the data obvious (perhaps
> painfully so).

The MIME parser currently internally uses binary strings when parsing, which tends to place similar demands on much of the utility methods. I have it as a "todo" to rewrite the parser to use uint8array internally, but that is no small undertaking and requires some more gathering of objectives to come up with a suitable replacement for _splitRegex.

> @@ +581,5 @@
> > +      return false;
> > +    }
> > +
> > +    // Make the buffer be a typed array for what follows
> > +    buffer = mimeutils.stringToTypedArray(buffer);
> 
> Why not have mimeutils.decode_*() return Uint8Array? Similar to the earlier
> comment, this makes it obvious we're handling raw octets rather than
> meaningful JS strings.

mimeutils.decode_* is designed mostly for the MIME parser and are so constrained to the same effects that are needed in the MIME parser.

> ES6 allows you to declare the default value:
> 
> function parseAddressingHeader(header, doRFC2047 = true) {

As indicated much earlier, I'm slightly suspicious of ES6 features that aren't dramatically useful to the working of code. (Generators, Map, and for-of are different from, say, spread operators or default arguments in this regard).

> but as with the earlier comment, I'm not a fan of flags that substantially
> change API behaviour. Does the real world ever require doRFC2047===false? I
> only see it in your test cases.

Have you not seen the Thunderbird APIs that require doRFC2047 = false? I'm quite sure you've reviewed some of them yourself.

> @@ +989,5 @@
> > + *
> > + * @param {String} value The RFC 2231-encoded string to decode.
> > + * @return The Unicode version of the string.
> > + */
> > +function decode2231Value(value) {
> 
> I don't like that this routine is passed a parameter that's part String
> (outside the quotes) and part octets (inside the quotes), though it would
> take quite a bit of restructuring and maybe flag-driven-behaviour removal
> from the caller to resolve this.

I'm not sure entirely what you mean by this. The purpose of this method is take the result of a 2231-encoded parameter (which is used in two places, due to RFC 2231 also letting you split up parameters and therefore needing to reassemble them, etc., etc., rant-against-RFC 2231), so it's not clear there's any benefit to be had by tweaking the API to this--it just invites more code duplication.

> @@ +1448,5 @@
> > +
> > +/**
> > + * An equivalent of Map.@@iterator, applied to the structured header
> > + * representations. This is the function that makes
> > + * for (let [header, key] of headers) work properly.
> 
> Iterator should return [key, value] rather than [value, key] (oddly, the
> code below looks to me like it actually returns [key, value] but I see
> callers that expect [value, key], so I'm not sure what's going on)

*peers into the code* Oh, I see what I did. The Map.forEach is stupidly specified and calls fn(value, key, map). When I looked into the .forEach method, I probably wrote it the wrong way first and then switched the let-destructured variables, so that 'key' refers to the value instead of the other way around. It was probably late at night when I wrote the documentation....

> @@ +1543,5 @@
> > + *    strformat: one of {binarystring, unicode, typedarray} [default=binarystring]
> > + *      How to treat output strings:
> > + *        binarystring: Data is a JS string with chars in the range [\x00-\xff]
> > + *        unicode: Data for text parts is converted to UTF-16; data for other
> > + *          parts is a typed array buffer, akin to typedarray.
> 
> Can we get away with only supporting your 'unicode' option here, rather than
> requiring callers to figure out which one to use?

Conversion to unicode is dangerous in some cases (fakeservers, e.g.), which is why I give the option to use binary strings or typedarrays instead of full autoconversion. As for the default, I'm not fully sure what the best way is, largely because the most common use case that wants unicode wants the body/attachment delineation that happens in code-not-yet-written, and it looks like that code needs to force specific parser options anyways.

> @@ +2269,5 @@
> > + *     not including the final CRLF pair. If this count would be exceeded, then
> > + *     an error will be thrown and encoding will not be possible.
> > + *   @param [options.useASCII=true] {Boolean}
> > + *     If true, then RFC 2047 and RFC 2231 encoding of headers will be performed
> > + *     as needed to retain headers as ASCII.
> 
> Do we ever *not* want to fully encode headers to wire format?

Yes. EAI support prefers not to use RFC 2047 or RFC 2231, and there are still cases in Thunderbird where we pass around addresses as strings instead of arrays.

> ::: mailnews/mime/jsmime/test/test_header_emitter.js
> @@ +51,5 @@
> > +      [[{name: "", email: "hi![e-mail]"}], "\"hi!bad\"@all.com"],
> > +      [[{name: "", email: "\"hi!bad\"@all.com"}], "\"hi!bad\"@all.com"],
> > +      [[{name: "Doe, John", email: "[e-mail]"}], "\"Doe, John\" <[e-mail]>"],
> > +      [[{name: "A really, really long name to quote", email: "[e-mail]"}],
> > +        "A \"really,\" really long name\r\n to quote <[e-mail]>"],
> 
> This is a bit odd - while it may be legal, it looks better to quote the
> entire name field if any part of it needs to be quoted.

The name exceeds the line limits specified in the test. In that scenario, I judged it to better to quote minimally to fit in the line limit than to exceed the line limit. That's what the test is testing, in fact (note the presence of the \r\n).

Comment 6

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8359887 [details] [diff] [review]
Import JSMime 0.2 into comm-central

Review of attachment 8359887 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Joshua Cranmer [:jcranmer] from comment #5)

(lots of sections left out where I accept your explanation and withdraw my objection)

> > Iterator should return [key, value] rather than [value, key] (oddly, the
> > code below looks to me like it actually returns [key, value] but I see
> > callers that expect [value, key], so I'm not sure what's going on)
> 
> *peers into the code* Oh, I see what I did. The Map.forEach is stupidly
> specified and calls fn(value, key, map). When I looked into the .forEach
> method, I probably wrote it the wrong way first and then switched the
> let-destructured variables, so that 'key' refers to the value instead of the
> other way around. It was probably late at night when I wrote the
> documentation....

OK, just make sure the naming and documentation are cleaned up.

> > ::: mailnews/mime/jsmime/test/test_header_emitter.js
> > @@ +51,5 @@
> > > +      [[{name: "", email: "hi![e-mail]"}], "\"hi!bad\"@all.com"],
> > > +      [[{name: "", email: "\"hi!bad\"@all.com"}], "\"hi!bad\"@all.com"],
> > > +      [[{name: "Doe, John", email: "[e-mail]"}], "\"Doe, John\" <[e-mail]>"],
> > > +      [[{name: "A really, really long name to quote", email: "[e-mail]"}],
> > > +        "A \"really,\" really long name\r\n to quote <[e-mail]>"],
> > 
> > This is a bit odd - while it may be legal, it looks better to quote the
> > entire name field if any part of it needs to be quoted.
> 
> The name exceeds the line limits specified in the test. In that scenario, I
> judged it to better to quote minimally to fit in the line limit than to
> exceed the line limit. That's what the test is testing, in fact (note the
> presence of the \r\n).

Ah, OK. Maybe a comment or change the text to "A name, that needs to be quoted and split across two lines" so it's obvious what you're testing - in general, I found it quite mind-numbing to try and reverse engineer the goals of your test cases from just the inputs and outputs; comments or other guidance would have made that task (and would make the task of analyzing test failures in future) easier.

Comment 7

[Ludovic Hirlimann [:Usul]]

when will this land ?

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Import updated JSMime

Some testing and use identified a few changes that would be extremely useful. The most notable change is switching the structured header map to normalizing to "preferred" spellings (e.g., To, Cc, MIME-Version) instead of all lower-case. I also changed the test suite to loading files asynchronously after Firefox started complaining about sync XHR, which turns out to have been much easier than I was expecting.

The JSMime changes themselves can be seen at this link: <https://github.com/jcranmer/jsmime/compare/389cae6c60...master>.

Comment 9

[Joshua Cranmer [:jcranmer]]

Attachment: Use charset decoders instead of TextDecoder in a few cases

So I earlier said that jsmime.jsm may need to have some polyfills... well, here's polyfill #1: my exploration of charsets indicates that we need to support UTF-7 and x-mac-croatian in RFC 2047 encoded headers. Since I don't have time to write a decoder for these right now, I'm using the builtin nsIUnicodeDecoder instead.

Or I would be if I could. All I can do in JS is nsIScriptableUnicodeConverter, which lacks much of the flexibility that is needed to polyfill TextDecoder. I threw in some tests to make sure JSMime can handle the UTF-7 decoding (and x-mac-croatian, for good measure).

Comment 10

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8392211 [details] [diff] [review]
Import updated JSMime

Review of attachment 8392211 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to get the Promise usage in the tests cleaned up a bit, but otherwise I'm happy.

::: mailnews/mime/jsmime/test/head_xpcshell_glue.js
@@ +33,5 @@
> +
> +    let promise = Promise.resolve(filename);
> +    promise.then(do_get_file).then(function (file) {
> +      return OS.File.read(file.path);
> +    }).then(function (contents) {

Are you Promise-wrapping this to get the errors caught? I'd probably just do that part synchronous.
Also, I like how ES6 arrow functions look with Promises (since this code is Mozilla-specific), but tastes differ...

let file;
try {
  file = do_get_file(filename);
catch(e) {
  callback(e);
}
OS.File.read(file.path))
  .then(contents => callback(undefined, contents),
        callback);      // or error => callback(error), if you prefer how that looks

@@ +109,5 @@
> +MochaSuite.prototype.runSuite = function () {
> +  return Task.spawn(this._runSuite.bind(this));
> +};
> +
> +/// Run the given function, ending when it is done.

It's nice to document functions that return promises e.g.
/// Run the given function asynchronously
// returns Promise{null}: Resolves with no value when fn() completes

@@ +116,5 @@
> +    function onEnd() {
> +      resolve();
> +    }
> +    if (fn.length == 1) {
> +      fn(onEnd);

why define function onEnd()? fn(resolve) should work.

::: mailnews/mime/jsmime/test/test_mime_tree.js
@@ +29,5 @@
> +/// A file cache for read_file.
> +var file_cache = {};
> +
> +/**
> + * Read a file into a string (all line endings become CRLF).

* @return Promise{string} resolves with a string containing the requested range of lines from the file.

@@ +39,5 @@
> +        if (err) reject(err);
> +        else resolve(data);
> +      });
> +    });
> +    var loader = realFile.then(function (realFile) {

I'd prefer you not shadow the variable name of the promise in the function body of the handler (realFile, in this case); I've been known to use the somewhat Hungarian pWhatever for the promise, unless I can think of a better name:

let readFile = ... make promise ...
let loader = readFile.then(contents => { ... });

@@ +119,5 @@
> +    message = Promise.resolve(message);
> +  }
> +  if (!(results instanceof Promise)) {
> +    results = Promise.resolve(results);
> +  }

Promise.all([message, results]) will do the right thing with non-Promise arguments, so you don't need to wrap them. Aside from that, instanceof is problematic; it's better to duck type by looking for a .then() function.

@@ +208,5 @@
> +    if (!checkingHeaders)
> +      assert.equal(0, uncheckedValues.length);
> +    else
> +      assert.deepEqual({}, uncheckedValues);
> +  }).then(callback, callback);

- Most of the Toolkit code that uses promises doesn't cuddle the .then onto the closing parens.
- If you can end the promise chain with a standalone exception catcher that causes a test failure, that would be best. In xpcshell terms,

...})
.then(callback)
.then(null, do_report_unexpected_exception);

Taking a quick look at the Mocha docs, it looks like you can also return promises instead of using done() callbacks, so if you can change your polyfill to support that it would be even better.

@@ +484,5 @@
> +        tree.get('1').headers.charset = 'Shift-JIS';
> +        assert.equal(tree.get('1').headers.charset, 'Shift-JIS');
> +        assert.equal(tree.get('1').headers.get('Content-Description'),
> +          '\u30b1\u30c4\u30a1\u30eb\u30b3\u30a2\u30c8\u30eb');
> +      }).then(done, done);

Again, would prefer
})
.then(done)
.then(null, function_taking_exception_and_logging_test_failure);

here and in all similar places if possible (among other things, this logs a clear error if the done() callback throws rather than silently discarding the exception), or just return the promise if the framework can handle it.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8359854 [details]
JSMime 0.2

Clearing the review request against me since I think as discussed on IRC a week or two ago I think I've already provided the high level feedback desired over time on IRC and the Gaia e-mail adoption of the library is going to come in the form of new bugs being filed against this existing library as opposed to needing to be in the form of a big review.

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to :Irving Reid from comment #10)
> Are you Promise-wrapping this to get the errors caught? I'd probably just do
> that part synchronous.

It's to make it closer to the node.js rules, where all the errors are thrown asynchronously. (Of course, the throw new Error above kind of ruins that point...). Given that most of my development is done in browser-land, I generally don't expect to see any errors anyways--there's certainly no attempt to specify how the error message looks in the shims.

> Also, I like how ES6 arrow functions look with Promises (since this code is
> Mozilla-specific), but tastes differ...

I've generally held off on "unnecessary" ES6 features because I want to get JSMime working on Node.js... except I need to wait for v8 to get a non-broken Map implementation since I badly need iteration support. head_xpcshell_glue.js code doesn't have that issue--indeed, I could even play with using Tasks.jsm here. :-)

> @@ +116,5 @@
> > +    function onEnd() {
> > +      resolve();
> > +    }
> > +    if (fn.length == 1) {
> > +      fn(onEnd);
> 
> why define function onEnd()? fn(resolve) should work.

Mocha abuses the onEnd function here. onEnd(non-undefined) means fail, onEnd(undefined) means pass. On closer inspection, I never coded up the code to handle fails properly. (casualty of how I test the code, probably).

> - Most of the Toolkit code that uses promises doesn't cuddle the .then onto
> the closing parens.

My inclination is to prefer:
return promise.then(function(blah) {
  {blah blah}
}).then(function(blah) {
  blah blah
});

Although I can see prefer .then being its own line if it's not a function header.

> Taking a quick look at the Mocha docs, it looks like you can also return
> promises instead of using done() callbacks, so if you can change your
> polyfill to support that it would be even better.

Looking back at the docs, I missed this feature. I so much prefer promises over callbacks that I've ripped out the callbacks, which invalidates a few of the other comments here.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

The mocha promises support is fairly new.  March 13th:
https://github.com/visionmedia/mocha/blob/master/History.md#1180--2014-03-13

Comment 14

[Joshua Cranmer [:jcranmer]]

Attachment: Import updated JSMime

Comment 15

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8399686 [details] [diff] [review]
Import updated JSMime

Review of attachment 8399686 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/mime/jsmime/test/head_xpcshell_glue.js
@@ +132,5 @@
> +      if (promise !== undefined && 'then' in promise &&
> +          typeof promise.then === "function")
> +        promise.then(resolve, reject);
> +      else
> +        resolve();

resolve() does the right thing whether you pass it undefined, a value, or a promise, so you can support function-optionally-returning-promise with just

resolve(fn());

Comment 16

[Joshua Cranmer [:jcranmer]]

Pushed, finally:
remote:   https://hg.mozilla.org/comm-central/rev/2ad8960eb396
remote:   https://hg.mozilla.org/comm-central/rev/c7154af5cfaa

Comment 17

[Phil Lacy]

Is this going to be the 'new' backend for bugs such as this bug 1018606

Comment 18

[Joshua Cranmer [:jcranmer]]

(In reply to Phil Lacy from comment #17)
> Is this going to be the 'new' backend for bugs such as this bug 1018606

Eventually, but the logic that's written, let alone landed, does not affect that bug. (I've mostly stuck to header logic for the moment; the work on bodies is hung up on some much-delayed specification work).