Introduce the structured header concept to nsIMsgCompFields

RESOLVED FIXED in Thunderbird 37.0

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Blocks 3 bugs)

Trunk
Thunderbird 37.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(25 attachments, 5 obsolete attachments)

6.10 KB, patch
Irving
: review+
Details | Diff | Splinter Review
8.25 KB, patch
Irving
: review+
Details | Diff | Splinter Review
3.00 KB, patch
Irving
: review+
Details | Diff | Splinter Review
8.15 KB, patch
Irving
: feedback+
Details | Diff | Splinter Review
19.15 KB, patch
Irving
: review+
Details | Diff | Splinter Review
11.46 KB, patch
Irving
: review+
Details | Diff | Splinter Review
14.80 KB, patch
Irving
: review+
Details | Diff | Splinter Review
10.68 KB, patch
Irving
: feedback+
Details | Diff | Splinter Review
10.85 KB, patch
Irving
: feedback+
Details | Diff | Splinter Review
13.57 KB, patch
Irving
: feedback+
Details | Diff | Splinter Review
3.53 KB, patch
neil
: review+
Details | Diff | Splinter Review
20.62 KB, patch
neil
: review+
Details | Diff | Splinter Review
24.94 KB, image/png
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
39 bytes, text/x-review-board-request
Irving
: review+
Details
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. :-)
Blocks: 998192
Blocks: 108261
Depends on: 1020696
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.]
Attachment #8488964 - Flags: feedback?(neil)
Attachment #8488964 - Flags: feedback?(irving)
(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); ?
(In reply to neil@parkwaycc.co.uk 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.
Posted patch Part 4±1 alternative idea (obsolete) — Splinter Review
Posting a rough idea of how Neil's suggestions would look when implemented, for comparison.
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?
Attachment #8488964 - Flags: feedback?(irving) → feedback+
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)
It's a lot easier to muck with this class if we use real string classes instead of C strings.
Attachment #8488964 - Attachment is obsolete: true
Attachment #8496654 - Attachment is obsolete: true
Attachment #8488964 - Flags: feedback?(neil)
Attachment #8505236 - Flags: review?(neil)
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.
Attachment #8505238 - Flags: review?(irving)
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.
Attachment #8505239 - Flags: superreview?(neil)
Attachment #8505239 - Flags: review?(irving)
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.
Attachment #8505240 - Flags: review?(irving)
Code can be much simpler when you use a map of headers rather than a single large block of MIME text.
Attachment #8505242 - Flags: review?(irving)
*stares at mimedrft.cpp changes*
Attachment #8505243 - Flags: review?(irving)
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.
Attachment #8505246 - Flags: review?(irving)
Attachment #8505246 - Attachment is patch: true
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. >_>
Attachment #8505249 - Flags: review?(irving)
Attachment #8505249 - Attachment is patch: true
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. :-)
Attachment #8505252 - Flags: review?(irving)
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];?
Attachment #8505236 - Flags: review?(neil) → review-
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?
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 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.
OS: Linux → All
Hardware: x86_64 → All
Attachment #8505236 - Attachment is obsolete: true
Attachment #8509907 - Flags: review?(neil)
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).
Attachment #8509908 - Flags: review?(neil)
Attachment #8505238 - Flags: review?(irving) → review+
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).
Attachment #8505239 - Flags: review?(irving) → review+
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?
Attachment #8505240 - Flags: review?(irving) → review+
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)
Attachment #8505242 - Flags: review?(irving) → feedback+
Attachment #8505243 - Flags: review?(irving) → review+
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...
Attachment #8505246 - Flags: review?(irving) → review+
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 <test@tinderbox.invalid"}, 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.
Attachment #8505248 - Flags: review?(irving) → review+
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.
Attachment #8505249 - Flags: review?(irving) → feedback+
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
Attachment #8505250 - Flags: review?(irving) → feedback+
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
Attachment #8505252 - Flags: review?(irving) → feedback+
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?
Attachment #8509907 - Flags: review?(neil) → review+
Attachment #8509908 - Flags: review?(neil) → review+
(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 neil@parkwaycc.co.uk 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...
(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.
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.
Attachment #8532867 - Flags: review?(irving)
/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 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.
Attachment #8532867 - Flags: superreview?(neil)
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?
Attachment #8505237 - Attachment is obsolete: true
Attachment #8505237 - Flags: review?(neil)
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...
Attachment #8505239 - Flags: superreview?(neil) → superreview+
Attachment #8532867 - Flags: superreview?(neil)
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?
Flags: needinfo?(Pidgeot18)
(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.
Flags: needinfo?(Pidgeot18)
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.
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.
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?
Attachment #8532867 - Flags: review?(irving) → review+
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.
Posted image 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.
(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.
(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
(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>.
WFM
Attachment #8532867 - Attachment is obsolete: true
Attachment #8618124 - Flags: review+
Attachment #8618125 - Flags: review+
Attachment #8618126 - Flags: review+
Attachment #8618127 - Flags: review+
Attachment #8618128 - Flags: review+
Attachment #8618129 - Flags: review+
Attachment #8618130 - Flags: review+
Attachment #8618131 - Flags: review+
Attachment #8618132 - Flags: review+
Attachment #8618133 - Flags: review+
Attachment #8618134 - Flags: review+
Attachment #8618135 - Flags: review+
You need to log in before you can comment on or make changes to this bug.