1456001 - Write test for bug 1454257 - Treatment of folded headers in nsParseMailbox.cpp

Status: RESOLVED FIXED

(MailNews Core :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1454257 +++

Now that this is nicely working we should turn the test message in bug 1454257 into a test.

That could be easily done in Mozmill, just import the message and then check msgHdr.author, msgHdr.recipients and msgHdr.subject.

A little more challenging as Xpcshell, but any test using MailServices.copy.CopyFileMessage to copy a file to a folder should be useful as a template, for example mailnews/base/test/unit/test_bccInDatabase.js which loads a message and checks the headers.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1456001-header-folding-test.patch

During testing it turned out that JS Mime does NOT compress inter-word space. It looks like this in the header pane, but when you use reply, you can see the inter-word spaces.

The C++ part of this patch takes care of that. It re-establishes the back-tracking we had in an earlier patch of bug 1454257 and removes the overall compression.

Messy business.

Comment 2

[Jorg K (CEST = GMT+2)]

Actually, with the message provided here you see three different behaviours:

On reply, the white-space around the folds is removed.
On forward, the white-space around the folds is maintained, but only in the resulting subject. In the quoted message multiple spaces are collapsed.
In the header pane, all multiple spaces in the subject are compressed, but not in the recipient.
Copy/paste of the subject behaves like forward, so you get all white-space around the fold.

This is clearly a mess. I think the patch proposed here is a compromise which mirrors the reply behaviour. Further reading: Bug 1259430.

We should keep in mind that bug 1454257 addressed the issue of headers being glued together. That's fixed now, so it's a secondary issue how we want to treat multiple spaces which is already inconsistent.

Comment 3

[:aceman]

Comment on attachment 8970124 [details] [diff] [review]
1456001-header-folding-test.patch

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

::: mailnews/base/test/unit/test_headerFoldingInDatabase.js
@@ +2,5 @@
> + * 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/. */
> +
> +/*
> + * Testing of bcc in message summary file added in bug 481667

This is probably copied... Please reference bug 1454257.

@@ +35,5 @@
> +function continueTest()
> +{
> +  Assert.equal(hdr.author, "[e-mail]");
> +  Assert.equal(hdr.recipients, "\"Recipient  with  spaces\" <[e-mail]>");
> +  Assert.equal(hdr.subject, "Badly folded headers, one line with   space   between   To and From");

Why is there 1 space in 'one line'? There were multiple continuation lines. Do they only contribute at most one space? Is that according to the RFC?

Comment 4

[Jorg K (CEST = GMT+2)]

As I said, please refer to bug 1259430 comment #6 for further reading.

https://tools.ietf.org/html/rfc5322#section-3.2.2 says (last paragraph):
   Runs of FWS, comment, or CFWS that occur between lexical tokens in a
   structured header field are semantically interpreted as a single
   space character.

So I think compacting white-space "around a fold" as proposed here is the right thing to do.

I can fix the comment at check-in time.

Comment 5

[Jorg K (CEST = GMT+2)]

Besides, the old code had the back-tracking code as well:
https://dxr.mozilla.org/comm-central/rev/8d636ad5810b5586433294c16cb2376d826fcca8/mailnews/local/src/nsParseMailbox.cpp#1083
      // locate the proper location for a folded space by eliminating any
      // leading spaces before the end-of-line character
      char* foldedSpace = buf;
      while (*(foldedSpace - 1) == ' ' || *(foldedSpace - 1) == '\t')
        foldedSpace--;

Only that it's not "leading" but "trailing" spaces.

So all we do with this patch is reverting to the previous behaviour while maintaining the clean-up and bug fix.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1456001-header-folding-test.patch (v1b)

Fixed comment.

Comment 7

[:aceman]

Comment on attachment 8970303 [details] [diff] [review]
1456001-header-folding-test.patch (v1b)

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

Works for me, thanks.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8970303 [details] [diff] [review]
1456001-header-folding-test.patch (v1b)

[Triage Comment]

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1504b9ba5c76
Bug 1454257 follow-up: fix inter-word space compression and add test for header folding in database. r=aceman

Comment 10

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 5):
https://hg.mozilla.org/releases/comm-beta/rev/2ed7c90c8ab0e9c3bc5fe621300092f1d15d0e28