998191 - Introduce the structured header concept to nsIMsgCompFields

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Joshua Cranmer [:jcranmer]]

I've got an extensively long patch queue that starts with making nsIMsgCompFields inherit from msgIWritableStructuredHeaders and ends with (well, once I finish them) the structured headers surviving until nsMsgSendPart is told to write them out to the temporary file.

This bug should contain pretty much the patches up to the removal of nsIMsgCompFields.randomOtherHeaders. It's worth noting that there's comments in the code suggesting that we should have added the idea of compose fields being a header map years ago. :-)

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Part 4±1: Add a header builder interface

I'm attaching this patch of the queue before I finalize the rest of the set of patches because this subtly impacts almost the entire rest of the queue. This may also influence comments on bug 998189.

The problem I have is that I need to convert a structured header object into a buffer that I can append to the output from the current C++ compose code. The exact place this happens ends up switching several times over the course of incremental patches--first nsMsgCompUtils, then nsMsgSend, then finally nsMsgSendPart.

The code that does the emission is an intrinsic JS call in jsmime--it's even simpler than presented in this patch, I could have just called jsmime.headeremitter.emitStructuredHeaders. The interface I present in this patch is the result of constant redesign from an interface that predated structured headers, which is why the process of converting a structured header object to a mime header block seems so convoluted:
nsCOMPtr<msgIMimeHeaderBuilder> headerBuilder =
  do_CreateInstance(NS_IMIMEHEADERBUILDER_CONTRACTID);
headerBuilder->AddStructuredHeaders(finalHeaders);
nsCString moreHeaders;
headerBuilder->GetBuffer(moreHeaders);

In the long run, what I want to expose is basically a JS-implemented XPIDL API for nsMsgSendPart, where you present a structured header object and the body contents and get out the full MIME message in response. That wouldn't need this interface, but I need this first to have any hope of getting there without making one really, really, really monolithic patch (my patch queue has 19 patches *after* this one to get nsMsgSendPart to a state where I can think about starting this and likely another 5 or 6 to actually get it there).

So requesting feedback from eventual reviewers for their thoughts.


[Note to self: full queue for this bug and not later ones will be up to more-strucuredheaders. Stopping at kill-orh doesn't make much sense.]

Comment 2

[[e-mail]]

(In reply to Joshua Cranmer from comment #1)
> nsCOMPtr<msgIMimeHeaderBuilder> headerBuilder =
>   do_CreateInstance(NS_IMIMEHEADERBUILDER_CONTRACTID);
> headerBuilder->AddStructuredHeaders(finalHeaders);
> nsCString moreHeaders;
> headerBuilder->GetBuffer(moreHeaders);

So why don't you want to write finalHeaders->GetBuffer(moreHeaders); ?

Comment 3

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] from comment #2)
> (In reply to Joshua Cranmer from comment #1)
> > nsCOMPtr<msgIMimeHeaderBuilder> headerBuilder =
> >   do_CreateInstance(NS_IMIMEHEADERBUILDER_CONTRACTID);
> > headerBuilder->AddStructuredHeaders(finalHeaders);
> > nsCString moreHeaders;
> > headerBuilder->GetBuffer(moreHeaders);
> 
> So why don't you want to write finalHeaders->GetBuffer(moreHeaders); ?

I thought about that for a bit. msgIStructuredHeaders has two distinct versions, one provided by the StructuredHeaders bit in jsmime (used for things we read) (and used for nsIMimeHeaders), and another which is used to provide the underlying functionality for (eventually) nsIMsgCompFields and nsIMsgDBHdr. Internally, these two implementations work slightly differently: nsIMimeHeaders is essentially built on binary strings, or nsACString, while the other versions use full Unicode strings, or nsAUTF8String/nsAString in XPIDL parlance.

The other issue is that building a MIME message requires setting options, particularly once we want to support EAI (and being able to say "no, don't use RFC 2047, just use UTF-8!"), so I don't want to particularly encourage people using this sort of method.

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Part 4±1 alternative idea

Posting a rough idea of how Neil's suggestions would look when implemented, for comparison.

Comment 5

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

Comment on attachment 8488964 [details] [diff] [review]
Part 4±1: Add a header builder interface

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

More feedback coming on the alternative patch.

::: mailnews/compose/src/msgMimeBuilder.js
@@ +19,5 @@
> +  classID: Components.ID("4dfaf4bd-bdb3-44af-a014-465aaf90ba95"),
> +  QueryInterface: XPCOMUtils.generateQI([
> +    Components.interfaces.msgIMimeHeaderBuilder
> +  ]),
> +  

white space

@@ +21,5 @@
> +    Components.interfaces.msgIMimeHeaderBuilder
> +  ]),
> +  
> +  addStructuredHeaders: function (headers) {
> +    let enumerator = headers.headerNames;

Can you make the headerNames enumerator JS-compatible, so that you can for..of loop on it?

Comment 6

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

Comment on attachment 8496654 [details] [diff] [review]
Part 4±1 alternative idea

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

I don't know how it affects the rest of your changes, but just looking at the two alternatives, this one seems a little more straightforward. I know how likely that is to break down when it comes in contact with the rest of the code base, so do what you need to make the bigger picture work.

::: mailnews/mime/public/msgIStructuredHeaders.idl
@@ +107,5 @@
> +   * This attribute is provided mainly for the benefit of C++ consumers of this
> +   * interface, since the JSMime headeremitter functionality allows more
> +   * fine-grained customization of the results.
> +   */
> +   readonly attribute AUTF8String asMimeText;

I'd prefer this to be a method, to indicate that we're doing the work of serializing; that also gives us a place to pass options in future when we want to specify how to serialize.

::: mailnews/mime/public/nsIMimeHeaders.idl
@@ +40,1 @@
>    readonly attribute ACString allHeaders;

This exposes a difference between reading messages, where we may want an API *somewhere* that gives us the raw version of what we got from the sender, and writing messages where we serialize them in our preferred representation. I wonder if we want to make the difference between those two use cases more explicit (e.g. a rawText attribute vs. a toMimeText() method, and I have no particular attachment to those specific names)

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Use an array of nsCString instead of char*.

It's a lot easier to muck with this class if we use real string classes instead of C strings.

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Make nsIMsgCompFields support msgIWritableStructuredHeaders

Comment 9

[Joshua Cranmer [:jcranmer]]

Attachment: Part 3: Copy all headers instead of each one manually

This eliminates some of the header "fixing", but that's okay because if we internally use structured headers, then retrieving them naturally fixes the headers anyways.

Comment 10

[Joshua Cranmer [:jcranmer]]

Attachment: Part 4: Add msgIStructuredHeaders::buildMimeText

I ended up using this version in lieu of the header builder interface. I'm still a little uneasy adding this kind of interface when it's something I don't want to support long-term, but temporary hacks have a way of hanging around, and this turns out to work a little better for a lot of later code anyways.

Comment 11

[Joshua Cranmer [:jcranmer]]

Attachment: Part 5: Emit all headers instead of a select few

Technically, this patch fulfills the contract of the bug. However, we can do a lot more cleanup on this method, so the patches continue after this.

Comment 12

[Joshua Cranmer [:jcranmer]]

Attachment: Part 6: Add extra headers via real headers, not otherRandomHeaders

Code can be much simpler when you use a map of headers rather than a single large block of MIME text.

Comment 13

[Joshua Cranmer [:jcranmer]]

Attachment: Part 7: Kill off nsIMsgCompFields::otherRandomHeaders completely

*stares at mimedrft.cpp changes*

Comment 14

[Joshua Cranmer [:jcranmer]]

Attachment: Part 8: Simplify mime_generate_headers via implicit JSMime usage

The next several patches are where I started ripping out "unnecessary" work in mime_generate_headers. So this is where the code changes start getting a little ugly and messy, and where I really loved having that test_messageHeaders.js I added a while back.

Comment 15

[Joshua Cranmer [:jcranmer]]

Attachment: Part 9: Simplify mime_generate_headers by using structured addresses

Comment 16

[Joshua Cranmer [:jcranmer]]

Attachment: Part 10: Simplify mime_generate_headers by using JSMime for newsgroup headers

Fun fact: I just fixed a case of undefined behavior in this patch. Bonus points if you can figure out what it is.

Oh, and if you see a test fail if you apply only some of this queue but not including the patch, it may be due to said undefined behavior. Drove me mad until I figured out that we were relying on undefined behavior. >_>

Comment 17

[Joshua Cranmer [:jcranmer]]

Attachment: Part 11: Simplify mime_generate_headers by fixing remaining headers

Comment 18

[Joshua Cranmer [:jcranmer]]

Attachment: Part 12: Deliver the result of mime_generate_headers as msgIStructuredHeaders

And some warnings fixes to this file.

And with this, I've completed all the patches for this bug!

Now I can go work on preparing the next dozen patches for review. :-)

Comment 19

[[e-mail]]

Comment on attachment 8505236 [details] [diff] [review]
Part 1: Use an array of nsCString instead of char*.

>+  return m_headers[header].IsEmpty() ? "" : m_headers[header].get();
nsCString::get returns "" when it's empty; if you're thinking of IsVoid(), then nsCString::get still returns "" (but nsXPIDLString::get returns nullptr in that case).

>-  char*       m_headers[MSG_MAX_HEADERS];
>+  nsTArray<nsCString> m_headers;
So, why not nsCString   m_headers[MSG_MAX_HEADERS];?

Comment 20

[[e-mail]]

Comment on attachment 8505237 [details] [diff] [review]
Part 2: Make nsIMsgCompFields support msgIWritableStructuredHeaders

>   m_headers[header] = value;
>+
>+  // If we are storing this on the structured header object, we need to set the
>+  // value on that object as well. Note that the value may be null, which we'll
>+  // take as an attempt to delete the header.
>+  const char *headerName = kHeaders[header].mName;
>+  if (headerName)
>+  {
>+    if (!value || !*value)
>+      return mStructuredHeaders->DeleteHeader(headerName);
>+
>+    return mStructuredHeaders->SetRawHeader(headerName,
>+      nsDependentCString(value), "UTF-8");
>+  }
(Could do this before setting the cached element since we're going to update it when we get the header anyway.)

>+    // We may be out of sync with the structured header object.
Should CheckCharsetConversion call GetAsciiHeader to ensure it's in sync?

Comment 21

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] from comment #19)
> >-  char*       m_headers[MSG_MAX_HEADERS];
> >+  nsTArray<nsCString> m_headers;
> So, why not nsCString   m_headers[MSG_MAX_HEADERS];?

Hmm. I think I really want std::array<nsCString, MSG_MAX_HEADERS> here--statically allocated space, but still the assertion checking. Of course, given the paucity of use, I could easily just use nsCString[] instedad.

(In reply to [e-mail] from comment #20)
> >+    // We may be out of sync with the structured header object.
> Should CheckCharsetConversion call GetAsciiHeader to ensure it's in sync?

Hmm. Actually, I don't think CheckCharsetConversion need do anything anymore--we always encode the headers to UTF-8, so there's no need to check that the headers can be in non-UTF-8.

Comment 22

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Use an array of nsCString instead of char*.

Comment 23

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Make nsIMsgCompFields support msgIWritableStructuredHeaders

A little bit of scope creep here--I kill off  nsIMsgCompFields::checkCharsetConversion since that method is effectively pointless (definitely so by the end of part 11 in this bug).

Comment 24

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

Comment on attachment 8505239 [details] [diff] [review]
Part 4: Add msgIStructuredHeaders::buildMimeText

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

::: mailnews/compose/public/nsIMsgCompFields.idl
@@ +10,5 @@
>  
>  /**
>   * A collection of headers and other attributes for building a mail message.
>   */
> +[scriptable, uuid(095fd3b3-41e1-448e-8f9a-534a5f8e5407)]

I'm guessing we have a footgun around IDL interfaces derived from an interface that changes needing a UUID rev, even though the derived interface doesn't have changes of its own?

::: mailnews/mime/public/msgIStructuredHeaders.idl
@@ +103,5 @@
> +   * The header values are emitted in an ASCII form, unless internationalized
> +   * email addresses are involved. The extra CRLF indicating the end of headers
> +   * is not included in this representation.
> +   *
> +   * This attribute is provided mainly for the benefit of C++ consumers of this

s/attribute/accessor

::: mailnews/mime/src/mimeJSComponents.js
@@ +90,5 @@
>      return new StringEnumerator(this._headers.keys());
> +  },
> +
> +  buildMimeText: function () {
> +    if (this._headers.size === 0)

Mozilla style prefers == to === (and in this case, == is probably more correct), and { } around one-line if / for / whatever bodies (also below).

Comment 25

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

Comment on attachment 8505240 [details] [diff] [review]
Part 5: Emit all headers instead of a select few

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

I'm OK with it as is, or you can make changes ;-)

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +371,5 @@
> +  finalHeaders->DeleteHeader("references");
> +  finalHeaders->DeleteHeader("x-mozilla-news-host");
> +  finalHeaders->DeleteHeader("x-priority");
> +  finalHeaders->DeleteHeader("message-id");
> +  finalHeaders->DeleteHeader("x-template");

Can you put comments in the legacy code pointing here, so that when they are cleaned up this gets updated? Is there a bug filed for cleaning those places up?

@@ +749,2 @@
>  
> +  return ToNewCString(headerText);

Would it cause too much API churn to change this function API to ns*String?

Comment 26

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

Comment on attachment 8505242 [details] [diff] [review]
Part 6: Add extra headers via real headers, not otherRandomHeaders

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

I should warn you that I'm reviewing these without a working build environment, so I'm blind to whether these changes actually compile and run. I would be highly surprised if you hadn't taken care of that...

I'll switch to r+ if you talk me out of the SetRawHeader character set API change.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +2848,5 @@
>                      mCompFields->GetTo(), mCompFields->GetCc(),
>                      mCompFields->GetBcc(), mCompFields->GetFcc(),
>                      mCompFields->GetNewsgroups(), mCompFields->GetFollowupTo(),
>                      mCompFields->GetSubject(), mCompFields->GetReferences(),
> +                    mCompFields->GetOrganization(), "");

Is the final, now-empty parameter going to disappear some time or do we still need it for other consumers?

@@ +2882,5 @@
>  
>        nsCString headerVal;
>        rv = mUserIdentity->GetCharAttribute(headerName.get(), headerVal);
>        if (NS_SUCCEEDED(rv)) {
> +        int32_t colonIdx = headerVal.FindChar(':');

I was going to comment on this code using the term 'colonoscopy'. I figured out that my comment was moot, but I still wanted a chance to use that word...

@@ +2902,5 @@
>    nsresult rv;
>  
> +  // If there's already a Mail-Followup-To header, don't need to do anything.
> +  nsAutoCString mftHeader;
> +  mCompFields->GetRawHeader("Mail-Followup-To", mftHeader);

Define a constant for "Mail-Followup-To", if there isn't one already

@@ +2903,5 @@
>  
> +  // If there's already a Mail-Followup-To header, don't need to do anything.
> +  nsAutoCString mftHeader;
> +  mCompFields->GetRawHeader("Mail-Followup-To", mftHeader);
> +  if (!mftHeader.IsEmpty())

Add { } around the one-line if body while you're here

@@ +2949,5 @@
>      return NS_OK;
>  
>    // Set Mail-Followup-To
> +  return mCompFields->SetRawHeader("Mail-Followup-To", recipients,
> +    mCompFields->GetCharacterSet());

Can you make SetRawheader have a reasonable default for character set so you don't always have to pass it?

@@ +2960,5 @@
>    nsresult rv;
>  
> +  // If there's already a Mail-Reply-To header, don't need to do anything.
> +  nsAutoCString mrtHeader;
> +  mCompFields->GetRawHeader("Mail-Reply-To", mrtHeader);

Constant or macro for "Mail-Reply-To" (ditto for other header names used more than once)

Comment 27

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

Comment on attachment 8505246 [details] [diff] [review]
Part 8: Simplify mime_generate_headers via implicit JSMime usage

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

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ -369,5 @@
> -  finalHeaders->DeleteHeader("references");
> -  finalHeaders->DeleteHeader("x-mozilla-news-host");
> -  finalHeaders->DeleteHeader("x-priority");
> -  finalHeaders->DeleteHeader("message-id");
> -  finalHeaders->DeleteHeader("x-template");

When I remarked on this code in a previous patch comment, I figured I'd probably come across something like this...

Comment 28

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

Comment on attachment 8505248 [details] [diff] [review]
Part 9: Simplify mime_generate_headers by using structured addresses

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

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ -565,5 @@
>        if(ptr2 != ptr+1)
>          PL_strcpy(ptr+1, ptr2);
>      }
>  
> -    finalHeaders->SetRawHeader("Folloup-To", nsDependentCString(n2), charset);

ouch!

@@ +577,5 @@
>      }
>    }
>  
> +  // We don't want to emit a Bcc header to the output.
> +  finalHeaders->DeleteHeader("bcc");

Does this remove the 'bcc' field from the copy in the Sent folder, or only in the copy serialized to SMTP?

::: mailnews/compose/test/unit/test_messageHeaders.js
@@ +495,5 @@
>        "Fcc": undefined
>      });
>      yield sendMessage({"bcc": "Somebody <[e-mail]"}, identity);
>      checkMessageHeaders(daemon.post, {
> +      "To": "undisclosed-recipients: ;"

Another ouch. Good catch.

::: mailnews/mime/src/mimeJSComponents.js
@@ +44,5 @@
> + * properties defined for 'group' that throws off jsmime. This function converts
> + * the addresses into the form that jsmime expects.
> + */
> +function fixXpconnectAddresses(addrs) {
> +  return addrs.map((addr) => {

If 'addrs' supports for..of iteration, use that instead

@@ +45,5 @@
> + * the addresses into the form that jsmime expects.
> + */
> +function fixXpconnectAddresses(addrs) {
> +  return addrs.map((addr) => {
> +    if (!('group' in addr) || addr.group === undefined || addr.group === null) {

Do we really care about anything other than !addr.group ? If so, add an explanatory comment so the next person in knows the trick.

Comment 29

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

Comment on attachment 8505249 [details] [diff] [review]
Part 10: Simplify mime_generate_headers by using JSMime for newsgroup headers

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

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +404,1 @@
>        *status = NS_ERROR_FAILURE;

Do we really want to discard 'rv' here and replace it with a generic FAILURE?

::: mailnews/mime/src/extraMimeParsers.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function parseNewsgroups(headers) {

Might be better to name this file something like 'nntpMimeParsers' if all it is likely to contain is news-related headers.

Comment 30

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

Comment on attachment 8505250 [details] [diff] [review]
Part 11: Simplify mime_generate_headers by fixing remaining headers

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

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +322,5 @@
>    if (isDraft)
>    {
> +    nsAutoCString draftInfo;
> +    draftInfo.AppendLiteral("internal/draft; ");
> +#define APPEND_BOOL(method, param) \

Define this macro outside the function body

Comment 31

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

Comment on attachment 8505252 [details] [diff] [review]
Part 12: Deliver the result of mime_generate_headers as msgIStructuredHeaders

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

Needs a proper HG patch comment

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +237,3 @@
>  
>    nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
> +  NS_ENSURE_SUCCESS(rv, rv);

We weren't hiding the return in a macro before. Stick with
if (NS_FAILED(rv)) {
  return rv;
}

::: mailnews/compose/src/nsMsgSend.cpp
@@ +763,5 @@
> +
> +    // Convert the blocks of headers into a single string for emission.
> +    nsAutoCString headers;
> +    outputHeaders->BuildMimeText(headers);
> +    

white space

Comment 32

[[e-mail]]

Comment on attachment 8509907 [details] [diff] [review]
Part 1: Use an array of nsCString instead of char*.

Maybe an nsAutoTArray<nsCString, MSG_MAX_HEADERS> would work for you?

Comment 33

[Joshua Cranmer [:jcranmer]]

(In reply to :Irving Reid (No longer working on Firefox) from comment #25)
> Comment on attachment 8505240 [details] [diff] [review]
> Part 5: Emit all headers instead of a select few
> 
> Can you put comments in the legacy code pointing here, so that when they are
> cleaned up this gets updated? Is there a bug filed for cleaning those places
> up?
> 
> Would it cause too much API churn to change this function API to ns*String?

They're cleaned up in a few parts anyways.

(In reply to :Irving Reid (No longer working on Firefox) from comment #26)
> Can you make SetRawheader have a reasonable default for character set so you
> don't always have to pass it?

It's an IDL function, so no.

(In reply to :Irving Reid (No longer working on Firefox) from comment #28)
> Comment on attachment 8505248 [details] [diff] [review]
> Part 9: Simplify mime_generate_headers by using structured addresses
> Does this remove the 'bcc' field from the copy in the Sent folder, or only
> in the copy serialized to SMTP?

BCC is re-added when doing the save to Sent here: <http://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#4606>.

(In reply to :Irving Reid (No longer working on Firefox) from comment #29)
> Comment on attachment 8505249 [details] [diff] [review]
> Part 10: Simplify mime_generate_headers by using JSMime for newsgroup headers
> 
> Do we really want to discard 'rv' here and replace it with a generic FAILURE?

See part 12.

(In reply to :Irving Reid (No longer working on Firefox) from comment #31)
> Comment on attachment 8505252 [details] [diff] [review]
> Part 12: Deliver the result of mime_generate_headers as msgIStructuredHeaders
> 
> We weren't hiding the return in a macro before. Stick with
> if (NS_FAILED(rv)) {
>   return rv;
> }

Largely because we don't have a macro for
if (NS_FAILED(rv)) {
  COMPLAIN_TO_STDOUT();
  *status = rv;
  return nullptr; // Because this returns const char* instead of nsresult
}

Now that this method returns nsresult, it can return to the mailnews style of NS_ENSURE_SUCCESS(rv, rv);

(In reply to [e-mail] from comment #32)
> Comment on attachment 8509907 [details] [diff] [review]
> Part 1: Use an array of nsCString instead of char*.
> 
> Maybe an nsAutoTArray<nsCString, MSG_MAX_HEADERS> would work for you?

Perhaps, but this patch is back to nsCString[MSG_MAX_HEADERS] and I'm not keen on spending the time it takes to go back to an nsTArray...

Comment 34

[Joshua Cranmer [:jcranmer]]

(In reply to :Irving Reid (No longer working on Firefox) from comment #26)
> ::: mailnews/compose/src/nsMsgSend.cpp
> @@ +2848,5 @@
> >                      mCompFields->GetTo(), mCompFields->GetCc(),
> >                      mCompFields->GetBcc(), mCompFields->GetFcc(),
> >                      mCompFields->GetNewsgroups(), mCompFields->GetFollowupTo(),
> >                      mCompFields->GetSubject(), mCompFields->GetReferences(),
> > +                    mCompFields->GetOrganization(), "");
> 
> Is the final, now-empty parameter going to disappear some time or do we
> still need it for other consumers?

I may have cleaned that up in one of my other patch-sets where I replaced this method with a simpler one nsMsgCompFields that said "oy, do you have non-empty from and at least one non-empty to/cc/bcc/newsgroups?" Since I did that, or at least planned to do that, I didn't particularly bother cleaning up this method call.

Comment 35

[Joshua Cranmer [:jcranmer]]

More reactions to review comments:
> If 'addrs' supports for..of iteration, use that instead

Since I'm basically replacing each element of an array with a wrapped version of that element, it feels easier and clearer to use Array.prototype.map instead of a for (let addr of addrs) { result.push(...) } loop.

> Might be better to name this file something like 'nntpMimeParsers' if all it
> is likely to contain is news-related headers.

I'm leaning towards desiring to include some of the X-Mozilla-* headers in here eventually. The Newsgroup header was added largely because I wanted to get rid of the actually-broken (not that I discovered it until after I finished the patch!) code in mime_generate_headers.

Comment 36

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: bz://998191/jcranmer

Comment 37

[Joshua Cranmer [:jcranmer]]

/r/1207 - Bug 998191, part 1: Use nsCString instead of char* in nsMsgCompFields, r=Neil
/r/1209 - Bug 998191, part 2: Make nsIMsgCompFields support msgIWritableStructuredHeaders, r=Neil
/r/1211 - Bug 998191, part 3: Copy all MIME headers in nsMsgSend, r=irving
/r/1213 - Bug 998191, part 4: Add a method to convert structured headers to MIME-encoded text
/r/1215 - Bug 998191, part 5: Emit all headers to the MIME message, r=irving
/r/1217 - Bug 998191, part 6: Use structured headers instead of otherRandomHeaders to set other random headers
/r/1219 - Bug 998191, part 7: Remove nsIMsgCompFields::otherRandomHeaders, r=irving
/r/1221 - Bug 998191, part 8: Simplify mime_generate_headers to not do tasks already done by JSMime's header emission, r=irving.
/r/1223 - Bug 998191, part 9: Use structured addresses in mime_generate_headers
/r/1225 - Bug 998191, part 10: Simplify mime_generate_headers by using JSMime for newsgroup headers
/r/1227 - Bug 998191, part 11: Simplify mime_generate_headers by using JSMime for all remaining headers
/r/1229 - Bug 998191, part 12: Simplify mime_generate_headers by returning msgIStructuredHeaders

Pull down these commits:

hg pull review -r 4588a3cabfec136a1eeaa3d38cd49e90dcae1eab

Comment 38

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8532867 [details]
MozReview Request: bz://998191/jcranmer

(Didn't trust that Neil autocompleted correctly on mozreview)

This is for the sr request on part 4. Although comments on any of the twelve patches would be appreciated if you have them.

Comment 39

[[e-mail]]

Comment on attachment 8505237 [details] [diff] [review]
Part 2: Make nsIMsgCompFields support msgIWritableStructuredHeaders

This patch was obsoleted by attachment 8509908 [details] [diff] [review] was it not?

Comment 40

[Joshua Cranmer [:jcranmer]]

Er, yes. I didn't obsolete all of the other patches because it's hard to do that when you're not adding an attachment...

Comment 41

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

Joshua, are you just looking for re-review of parts 6, 10, 11, 12 (the ones that don't already have r+), or is there something else needed here? It's a bit hard to tell, with the switch from Splinter to Review Board.

Is there an easy way to see interdiffs here?

Comment 42

[Joshua Cranmer [:jcranmer]]

(In reply to :Irving Reid (No longer working on Firefox) from comment #41)
> Joshua, are you just looking for re-review of parts 6, 10, 11, 12 (the ones
> that don't already have r+),
Yes.

> Is there an easy way to see interdiffs here?

Unfortunately, I don't think so. I switched to reviewboard because it was far easier to download the patches and actually compile and test them.

Comment 43

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

https://reviewboard.mozilla.org/r/1227/#review1177

Ship It!

Comment 44

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

https://reviewboard.mozilla.org/r/1217/#review1179

Ship It!

Comment 45

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

https://reviewboard.mozilla.org/r/1223/#review1181

::: mailnews/mime/src/mimeJSComponents.js
(Diff revision 1)
> +    if (!('group' in addr) || addr.group === undefined || addr.group === null) {

Odd; I thought truthiness checking was one of the places where JS strict mode specifically overrode the non-existent attribute warning. In any case, I'm fine with this.

Comment 46

[Joshua Cranmer [:jcranmer]]

https://reviewboard.mozilla.org/r/1223/#review1183

> Odd; I thought truthiness checking was one of the places where JS strict mode specifically overrode the non-existent attribute warning. In any case, I'm fine with this.

I suspect (without proof) that the strict mode checking is based on pattern matching, and I ended up using a pattern that the JS engine didn't match.

Comment 47

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

https://reviewboard.mozilla.org/r/1225/#review1185

::: mailnews/compose/src/nsMsgCompUtils.cpp
(Diff revision 1)
>    // TODO: sanity check other_random_headers for newline conventions

Is this still TODO or do your changes make it moot?

Comment 48

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

https://reviewboard.mozilla.org/r/1229/#review1187

Ship It!

Comment 49

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

https://reviewboard.mozilla.org/r/1205/#review1189

Ship It!

Comment 50

[Joshua Cranmer [:jcranmer]]

https://reviewboard.mozilla.org/r/1225/#review1349

> Is this still TODO or do your changes make it moot?

It's made moot by whichever patch deleted nsIMsgCompFields::OtherRandomHeaders.

Comment 51

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/comm-central/rev/feaf7e10ead5
https://hg.mozilla.org/comm-central/rev/67519bbca970
https://hg.mozilla.org/comm-central/rev/3b8fcc27ab8a
https://hg.mozilla.org/comm-central/rev/45ccfc058277
https://hg.mozilla.org/comm-central/rev/a913648f4115
https://hg.mozilla.org/comm-central/rev/a2546abffc36
https://hg.mozilla.org/comm-central/rev/e82cc8049281
https://hg.mozilla.org/comm-central/rev/81ecc4e6871f
https://hg.mozilla.org/comm-central/rev/8b9c93f10dab
https://hg.mozilla.org/comm-central/rev/920ef6f2f3c2
https://hg.mozilla.org/comm-central/rev/a9e2a1b47cdd
https://hg.mozilla.org/comm-central/rev/c6f49a08146c

Comment 52

[Hartmut Figge]

Attachment: no_to.png

I strongly suspect that the checkins are causing the following problem for SM-Trunk Linux x86_64. Trying to post to mozilla.test i get no_to.png. Newsgroup mozilla.test is missing and there is no possibility to enter a target for the posting.

Happens on a new profile too. Same for mail.

Comment 53

[Joshua Cranmer [:jcranmer]]

(In reply to Hartmut Figge from comment #52)
> Created attachment 8545626 [details]
> no_to.png
> 
> I strongly suspect that the checkins are causing the following problem for
> SM-Trunk Linux x86_64. Trying to post to mozilla.test i get no_to.png.
> Newsgroup mozilla.test is missing and there is no possibility to enter a
> target for the posting.

Is there any error in the error console? I see no issues in the Thunderbird compose window for a newsgroup.

Comment 54

[Hartmut Figge]

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

> Is there any error in the error console? I see no issues in the Thunderbird
> compose window for a newsgroup.

After selecting mozilla.test in the folder pane and pressing 'Compose' I get

Timestamp: 08.01.2015 05:12:41
Error: ReferenceError: msgRandomHeaders is not defined
Source File: chrome://messenger/content/messengercompose/addressingWidgetOverlay.js
Line: 191

Timestamp: 08.01.2015 05:12:41
Error: TypeError: element is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 1930

Comment 55

[Joshua Cranmer [:jcranmer]]

(In reply to Hartmut Figge from comment #54)
> (In reply to Joshua Cranmer [:jcranmer] from comment #53)
> 
> > Is there any error in the error console? I see no issues in the Thunderbird
> > compose window for a newsgroup.
> 
> After selecting mozilla.test in the folder pane and pressing 'Compose' I get
> 
> Timestamp: 08.01.2015 05:12:41
> Error: ReferenceError: msgRandomHeaders is not defined
> Source File:
> chrome://messenger/content/messengercompose/addressingWidgetOverlay.js
> Line: 191

Thanks the report. It looks like I lost a hunk of changes in one of several rebasings, so I reremoved the code and pushed it as <https://hg.mozilla.org/comm-central/rev/b75f54fd6121>.

Comment 56

[Hartmut Figge]

WFM

Comment 58

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 6: Use structured headers instead of otherRandomHeaders to set other random headers

Comment 59

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 7: Remove nsIMsgCompFields::otherRandomHeaders, r=irving

Comment 60

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 8: Simplify mime_generate_headers to not do tasks already done by JSMime's header emission, r=irving.

Comment 61

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 9: Use structured addresses in mime_generate_headers

Comment 62

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 10: Simplify mime_generate_headers by using JSMime for newsgroup headers

Comment 63

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 11: Simplify mime_generate_headers by using JSMime for all remaining headers

Comment 64

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 12: Simplify mime_generate_headers by returning msgIStructuredHeaders

Comment 65

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 1: Use nsCString instead of char* in nsMsgCompFields, r=Neil

Comment 66

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 2: Make nsIMsgCompFields support msgIWritableStructuredHeaders, r=Neil

Comment 67

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 3: Copy all MIME headers in nsMsgSend, r=irving

Comment 68

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 4: Add a method to convert structured headers to MIME-encoded text

Comment 69

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 998191, part 5: Emit all headers to the MIME message, r=irving