1005413 - Improvements to structured headers in JSMime

Status: ASSIGNED

(MailNews Core :: MIME, defect)

Description

[Joshua Cranmer [:jcranmer]]

[landing pad for review requests]

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Date header support

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Date header support

Oops, borked this up.

Comment 3

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

Comment on attachment 8416833 [details] [review]
Date header support

Not my most thorough review, but nothing jumped out at me as wrong.

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: JSMime patches for review

Comment 5

[Joshua Cranmer [:jcranmer]]

Attachment: RFC 2231/Content-Description headers

This can't land on comm-central yet without breaking tests thanks to bug 1074049 (insert rant about non-reflexivity of NaN comparisons here). Hence why this is structured as two pull requests rather than a single one.

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: RFC 2231 import into comm-central

(I manually commented out the problematic test to make local tests pass).

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: Import the Date and getHeaderToken improvements into comm-central

This one may be useful to look at, since a few changes were made due to strict JS controls that didn't feed back into jsmime upstream.

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Fix structured header test code failure caused by new JSMime update

Comment 9

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8497223 [details] [diff] [review]
Fix structured header test code failure caused by new JSMime update

Oops, forgot that this was a change to a test in my patch queue and not a change to a committed test. :-[

Comment 10

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8497223 [details] [diff] [review]
Fix structured header test code failure caused by new JSMime update

Actually, I'm going to revive this patch. It's going to end up folded with the structured headers patch before landing, though.

Comment 11

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

Comment on attachment 8497215 [details] [review]
JSMime patches for review

Minor nits.

Comment 12

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

Comment on attachment 8497223 [details] [diff] [review]
Fix structured header test code failure caused by new JSMime update

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

Is this test going to fail if the clock rolls over a second boundary between when the message is created and the value is checked? If so, that will be an annoying random failure. Might be worth adding a fudge factor (within 5 or 10 seconds of the same, maybe?)

Comment 13

[Joshua Cranmer [:jcranmer]]

(In reply to :Irving Reid from comment #12)
> Is this test going to fail if the clock rolls over a second boundary between
> when the message is created and the value is checked? If so, that will be an
> annoying random failure. Might be worth adding a fudge factor (within 5 or
> 10 seconds of the same, maybe?)

The test uses only one day value (which isn't even a new Date()), so it's not affected by the clock like the compose test I added a while back was.

Comment 14

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/comm-central/rev/3f8c1a00b76a

I'm only landing the Date header stuff for now. The RFC 2231 stuff requires fixing mozilla-central Assert.jsm first or commenting the test out in TB temporarily. Since I won't need it for a while, I feel safe leaving it out for now.

Comment 15

[Joshua Cranmer [:jcranmer]]

Attachment: RFC 2231 import into comm-central

Comment 16

[Joshua Cranmer [:jcranmer]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1f3d5aadda3e is a try run (on top of another, albeit hopefully unnecessary patch that does some compose cleanup).

Comment 17

[Kent James (:rkent)]

Comment on attachment 8666426 [details] [diff] [review]
RFC 2231 import into comm-central

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

General comments on review and next step:

As you can see through the comments that I have made so far, you are using a very unusual coding pattern in this file, but that pattern is almost completely undocumented here. That makes it impossible for me to easily figure out even what is calling what, and the meanings of basic function calls. If I am going to be able to understand this and review it, I need much better internal documentation about how the basic structure is supposed to work. I'm several hours into this, and I can't figure out how it works.

If there are other people who can review this who can more easily understand this, feel free to direct the review to them. But I really think that you need to take my comments and confusion, and use that as a guide to adding comments so that this code can be understood. I can't review it without a more basic map of how it is supposed to be structured.

General comments on code:

1) The file jsmime.js needs 1) license builderplate, 2) a brief description of the purpose of this file. Actually, all functions in this file need a one-line description of what they are supposed to do and the expected types of any parameters.

2) The header style "(function (root, fn) {this, function() {" is unusual, and sufficiently rare in Mozilla code that it deserves a comment pointing out that it follows the UMD pattern, and is usable in CommonJS, AMD, or as a global. ("fn" is always "factory" in the references I have seen, which helps obscure the pattern.)

3) The boilerplate that you added at the beginning of the file includes the undocumented functions req() and def(). I need a description of what these are supposed to do.

::: mailnews/mime/jsmime/jsmime.js
@@ +125,5 @@
> +    }
> +  }
> +
> +  if (needsQuote)
> +    text = '"' + text.replace(/["\\]/g, "\\$&") + '"';

It was not immediately obvious to me what the purpose of the test.replace regex was, so this needs a comment. I understand it now, but I should not have to think and analyze the regex to understand it.

@@ +133,1 @@
>  return {

It was not apparent to me what this is being returned to, without having to analyze several levels of code and understand the (undocumented) def function. I don't want to work this hard, this deserves a comment.

@@ +138,4 @@
>    stringToTypedArray: stringToTypedArray,
>    typedArrayToString: typedArrayToString,
>  };
>  });

Nit: you should really have a blank line after major divisions in the file such as this one.

@@ +140,5 @@
>  };
>  });
>  /**
>   * This file implements knowledge of how to encode or decode structured headers
>   * for several key headers. It is not meant to be used externally to jsmime.

OK this is the missing file description that I asked for earlier. Does it have to be buried 144 lines into the code like this? Hard to find it.

In looking at this file, the compatibility with different module systems must be a factor (since you are using 'require') and should also be mentioned here.

@@ +143,5 @@
>   * This file implements knowledge of how to encode or decode structured headers
>   * for several key headers. It is not meant to be used externally to jsmime.
>   */
>  
>  def('structuredHeaders', function (require) {

Why are you using these odd def() functions? I assume it must have something to do with compatibility with different module types, but this deserves a comment.

@@ +238,5 @@
>      structure.set(name.toLowerCase(), value);
>    });
>    return structure;
>  }
> +function emitContentType(contentType) {

Nit: can you place a blank line before function definitions? This function also needs a comment with a brief description of its purpose, and the types of its parameters.

@@ +241,5 @@
>  }
> +function emitContentType(contentType) {
> +  // Be forgiving and accept string variants of the content-type.
> +  if (typeof contentType == "string")
> +    contentType = parseContentType.call(headerparser(), [contentType]);

I'm totally confused by the proper parameter types for parseContentType, and that is because:

1) Nowhere do you really document its parameter type.
2) You are passing in an array with a .call which looks suspiciously like you meant to use .apply instead.
3) In the related definition of parseParameterHeader, you convert value to value[0], and pass it into a this. version of the same function. Is that the same function, or a different function with the same name? I suppose I should understand namespacing in JS well enough to know, but why should I have to? Or does the function take both array and non-array values? Any tricky business like this needs clear comments, and they are not there. I don't want to have to look at the code that hard to understand it.

@@ +244,5 @@
> +  if (typeof contentType == "string")
> +    contentType = parseContentType.call(headerparser(), [contentType]);
> +
> +  this.addText(contentType.type, false);
> +  this.addParameters(contentType);

With all of the aforementioned confusion about the module boilerplate and function definitions, I am now totally confused about what "this" is. I should not have to be an expert of UMD modules to understand your code. I need some supporting comments, please.

This is also complicated because you are not strictly following indentation rules for the functions defined in the boilerplate. That's OK in general, but I need some supporting comments then to know when one I enter and exit key outer objects.

@@ +246,5 @@
> +
> +  this.addText(contentType.type, false);
> +  this.addParameters(contentType);
> +}
> +addHeader("Content-Type", parseContentType, emitContentType);

Blank lines before and after this to separate it from the functions around it.

@@ +248,5 @@
> +  this.addParameters(contentType);
> +}
> +addHeader("Content-Type", parseContentType, emitContentType);
> +// RFC 2183
> +function parseDisposition(value) {

This function needs a comment describing its purpose, and a description of its parameters. No need for a full Doxygen treatment, but a single line comment should be sufficient. Make sure you describe the expected type of the parameter.

@@ +249,5 @@
> +}
> +addHeader("Content-Type", parseContentType, emitContentType);
> +// RFC 2183
> +function parseDisposition(value) {
> +  let headerparser = this;

It's very confusing to be passing a reference to a set of methods (headerparser) as the "this" to a function. Why are you using this confusing architecture? I can see how it works a few lines later in parseDisposition.call(headerparser(), ... but what about later, when you use it in addHeader?

If you absolutely must use this confusing architecture, then you must document it very clearly, which I don't see.

@@ +265,5 @@
> +    structure.set(name, value);
> +  });
> +  return structure;
> +}
> +function emitDisposition(value) {

blank line before the function, please. And perhaps you can guess by now that I would like a short description of the function, and the type of its parameters?

@@ +271,5 @@
> +  if (typeof value == "string")
> +    value = parseDisposition.call(headerparser(), [value]);
> +
> +  this.addText(value.isAttachment ? "attachment" : "inline", false);
> +  let emitter = this;

So I assume that somehow this function is being called in a context where "this" is an emitter object. But beats the heck out of me how or why this happens, it seems to be buried deeply in addHeader(), then into structuredEncoders, then a return through an object that renames it as encoders, and by now I am completely lost.

::: mailnews/mime/jsmime/test/test_header_emitter.js
@@ +320,5 @@
> +      deliverEOF: function () {
> +        assert.equal(this.output, this.expected + '\r\n');
> +      }
> +    };
> +    let header_tests = [

I really think that the expectation should be that each test has a simple description of what it is about, one per test. That rule does not have to be hard and fast, but no-comment should be the exception rather than the rule.

Look at the level of comments in test_MIME_params.js as an example.

::: mailnews/mime/jsmime/test/test_structured_headers.js
@@ +134,5 @@
> +      })],
> +    // Testcases following copied from http://greenbytes.de/tech/tc2231/. Some
> +    // of the rules (e.g., refusing RFC 2047) don't apply to us since we're not
> +    // a browser. We also don't ignore the header on parse errors but rather try
> +    // to extract some sense from it.

The reference that you gave gives brief descriptions of what is being tested in each case. Since the reference is not a primary document (unlike say an RFC) you need to copy those reasons as comments to the tests.

Comment 18

[Wayne Mery (:wsmwk)]

Joshua. is there a path forward for all or part of this?