Closed Bug 937056 Opened 11 years ago Closed 10 years ago

Move UTF-7 decoder/encoder to comm-central

Categories

(MailNews Core :: Internationalization, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

(Keywords: memory-footprint, Whiteboard: [MemShrink:P3])

Attachments

(4 files, 11 obsolete files)

48.80 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
35.81 KB, patch
standard8
: review+
Details | Diff | Splinter Review
3.44 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
912 bytes, patch
Details | Diff | Splinter Review
We have a bunch of nsIUnicodeDecoder and nsIUnicodeEncoder implementations in mozilla-central that are not used by Firefox but are used by Thunderbird. It's useless that we keep building and shipping these with Firefox. We should move this over to comm-central.

UTF-7 and x-imap4-modified-utf7 have been unused in m-c for years and, therefore, are the natural place to start. Once we figured out how to move those over into c-c, other mail-only encoders and decoders can follow the same pattern in follow-up bugs.
Thanks for not forgetting about c-c and not removing this stuff directly into the trash :)
Do you have any time estimate when you need to get rid of this code?
The sooner, the better. Avoiding Thunderbird breakage is really the only reason this code is still in m-c.
FYI:
On Mon, Jan 13, 2014 at 2:06 PM, Philip Chee <philip.chee@gmail.com> wrote:
>> On 13/01/2014 18:16, Henri Sivonen wrote:
>>>> Would someone, please, like to volunteer to implement
>>>> https://bugzilla.mozilla.org/show_bug.cgi?id=937056 : to create a
>>>> comm-central-only analog of nsUConvModule and to move the UTF-7 code
>>>> to that module?
>>
>> I'd volunteer, unfortunately I have totally no clue how to do this. Is
>> there a dummies guide?
Probably:
 1) Make a copy of nsUConvModule.cpp in comm-central under a different name. Let's say nsCommUConvModule.cpp.
 2) Remove traces of encodings other than x-imap4-modified-utf7 and UTF-7 from nsCommUConvModule.cpp.
 3) Replace occurrences of "kUConv" with "kCommUConv" in nsCommUConvModule.cpp.
 4) Replace occurrences of "nsUConv" with "nsCommUConv" in nsCommUConvModule.cpp.
 5) Do something that makes libxul pick up the new module but only in comm-central builds. (What exactly to do isn't clear to me.)
 6) Move nsUTF7ToUnicode.*, nsMUTF7ToUnicode.*, nsUnicodeToMUTF7.* and nsUnicodeToUTF7.* to comm-central.
The converters are components, so you just need to ensure they're registered.
(In reply to Philip Chee from comment #3)
>  5) Do something that makes libxul pick up the new module but only in
> comm-central builds. (What exactly to do isn't clear to me.)

Looks like what's needed is the addition of MODULE(nsCommUConvModule) in
https://mxr.mozilla.org/comm-central/source/suite/confvars.sh#28
and
https://mxr.mozilla.org/comm-central/source/mail/confvars.sh#25
Marking [MemShrink] in case that project wants to track the opportunity to slim down libxul itself. (Once the module described in comment 3 exists, there's plenty of code besides the UTF-7 converters to move there.)
Keywords: footprint
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P3]
Attached patch Add the code to c-c (obsolete) — Splinter Review
I am unable to actually build c-c at the moment. Furthermore, this patch doesn't even try to make the test files run.

If someone who can build c-c cared to polish the c-c patch, that would be great.
The patch looks like it only adds this to /mail, i.e Thunderbird. Seamonkey (/suite) will probably want it too so it should live somewhere in /mailnews.
Good point. This patch moves the code under mailnews/.
Attachment #8406107 - Attachment is obsolete: true
Now this should even compile. The c-c build now fails for me in a place unrelated to this patch.
Thanks.
Do these lines in the patch still referencing /mail files do anything?
diff --git a/mail/intl/moz.build b/mail/intl/moz.build
new file mode 100644
diff --git a/mail/intl/nsCommUConvModule.cpp b/mail/intl/nsCommUConvModule.cpp
new file mode 100644
I can confirm the c-c tree builds with the patch but I can't confirm it links in the code and it is picked up by some internal code. I have updated the tests to be picked up by c-c xpcshell infrastructure. You may use the updated patch if you like.
But the tests still do not run yet:
TEST-INFO | test_decode_utf-7_internal.js | Test failed or timed out, will retry.
TEST-INFO | test_encode_utf-7.js | Test failed or timed out, will retry.
TEST-INFO | test_decode_utf-7.js | Test failed or timed out, will retry.
TEST-INFO | test_encode_utf-7_internal.js | Test failed or timed out, will retry.
Retrying tests that failed when run in parallel.
INFO | Following exceptions were raised:
Traceback (most recent call last):
  File "mozilla/testing/xpcshell/runxpcshelltests.py", line 166, in run
    self.run_test()
  File "mozilla/testing/xpcshell/runxpcshelltests.py", line 586, in run_test
    head_files, tail_files = self.getHeadAndTailFiles(self.test_object)
  File "mozilla/testing/xpcshell/runxpcshelltests.py", line 361, in getHeadAndTailFiles
    return (list(sanitize_list(test_object['head'], 'head')),
KeyError: 'head'

Traceback (most recent call last):
  File "mozilla/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "mozilla/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "mozilla/testing/xpcshell/runxpcshelltests.py", line 1617, in <module>
    main()
  File "mozilla/testing/xpcshell/runxpcshelltests.py", line 1613, in main
    if not xpcsh.runTests(args[0], testdirs=args[1:], **options.__dict__):
  File "mozilla/testing/xpcshell/runxpcshelltests.py", line 1498, in runTests
    raise exceptions[0]
KeyError: 'head'
make: *** [xpcshell-tests] Error 1

Also, it looks like the tests load a file CharsetConversionTests.js, that looks not to be available in the c-c tree.
Comment on attachment 8406146 [details] [diff] [review]
Add the code to c-c, under mailnews, v2

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

Brief comments from inspection:

1. Don't make a separate module for this. Dropping the module bits in mailnews/build/nsMailModule.cpp should be more than sufficient.
2. The tests don't seem to be keyed off of anything?
3. The real tests that we need are that IMAP mailbox names still work (hopefully we have some non-ascii mailbox name tests in IMAP already?) and that UTF-7 message bodies can get transcoded properly (which we probably don't need).

Also, I expect Neil to whack you if this doesn't build with external linkage :-)

::: mailnews/intl/moz.build
@@ +13,5 @@
> +]
> +
> +MSVC_ENABLE_PGO = True
> +
> +FINAL_LIBRARY = 'xul'

FINAL_LIBRARY = 'mail' here.
(In reply to Joshua Cranmer from comment #16)
> Also, I expect Neil to whack you if this doesn't build with external linkage
> :-)
> 
> ::: mailnews/intl/moz.build
> @@ +13,5 @@
> > +]
> > +
> > +MSVC_ENABLE_PGO = True
> > +
> > +FINAL_LIBRARY = 'xul'
> 
> FINAL_LIBRARY = 'mail' here.

It's never going to build under external linkage even with this change because the base classes don't get exported from libxul.
(In reply to :aceman from comment #15)
> Add the code to c-c, under mailnews, v3 proposal
...
> You may use the
> updated patch if you like.

Thanks. Non-ASCII in IMAP folder names seems to work interoperably with this patch applied.

(In reply to Joshua Cranmer [:jcranmer] from comment #16)
> 1. Don't make a separate module for this. Dropping the module bits in
> mailnews/build/nsMailModule.cpp should be more than sufficient.

Does this opinion hold even with the additional data point that there will be lots of other m-c intl/ classes that should be move to c-c? This seems to require plenty of header exports compared to adding a module. What's the downside of adding a module?

> 2. The tests don't seem to be keyed off of anything?

As noted in comment 9.

> 3. The real tests that we need are that IMAP mailbox names still work
> (hopefully we have some non-ascii mailbox name tests in IMAP already?) and
> that UTF-7 message bodies can get transcoded properly (which we probably
> don't need).

That seems out of scope of this code move bug. I tested IMAP folder names manually.

> Also, I expect Neil to whack you if this doesn't build with external linkage
> :-)

Does not building with external linkage block landing to comm-central? I thought external linkage was a total boondoggle and had been abandoned already.

> ::: mailnews/intl/moz.build
> @@ +13,5 @@
> > +]
> > +
> > +MSVC_ENABLE_PGO = True
> > +
> > +FINAL_LIBRARY = 'xul'
> 
> FINAL_LIBRARY = 'mail' here.

OK.
(In reply to Henri Sivonen (:hsivonen) from comment #18)
> Does not building with external linkage block landing to comm-central? I
> thought external linkage was a total boondoggle and had been abandoned
> already.
In comm-central extrenal linkage still works and some people (including Neil) care for it (and actively fix it when necessary).
(In reply to :aceman from comment #20)
> In comm-central extrenal linkage still works and some people (including
> Neil) care for it (and actively fix it when necessary).

So may I assume that instead of whacking me, Neil will do the work to make this work with external linkage?
Actually the way you've written it makes it look like a big job so I might just hope the guy who got external linkage into shape in the first places volunteers and I only have to review it.
Current c-c patch: still a separate module pending a response to an earlier comment. (I still think a separate module for this is cleaner than putting everything in the mail module.)
Attachment #8406146 - Attachment is obsolete: true
Attachment #8406347 - Attachment is obsolete: true
(In reply to Henri Sivonen (:hsivonen) from comment #18)
> Does this opinion hold even with the additional data point that there will
> be lots of other m-c intl/ classes that should be move to c-c? This seems to
> require plenty of header exports compared to adding a module. What's the
> downside of adding a module?

More modules means easier to screw something up. Besides, we typically don't export headers just to add them to a module: we just have a really, really fat LOCAL_INCLUDES list in mailnews/build.

> > 2. The tests don't seem to be keyed off of anything?
> 
> As noted in comment 9.

It's not even clear to me what kind of tests these should be...

> > 3. The real tests that we need are that IMAP mailbox names still work
> > (hopefully we have some non-ascii mailbox name tests in IMAP already?) and
> > that UTF-7 message bodies can get transcoded properly (which we probably
> > don't need).
> 
> That seems out of scope of this code move bug. I tested IMAP folder names
> manually.

IMAP folder names are tested in mailnews/imap/test/test_mailboxes.js already.
Comment on attachment 8406815 [details] [diff] [review]
Remove code from m-c, accommodate includes from c-c, fix a test

\o/
Attachment #8406815 - Flags: review?(VYV03354) → review-
Attachment #8406815 - Flags: review- → review+
(In reply to Joshua Cranmer [:jcranmer] from comment #25)
> (In reply to Henri Sivonen (:hsivonen) from comment #18)
> > Does this opinion hold even with the additional data point that there will
> > be lots of other m-c intl/ classes that should be move to c-c? This seems to
> > require plenty of header exports compared to adding a module. What's the
> > downside of adding a module?
> 
> More modules means easier to screw something up. Besides, we typically don't
> export headers just to add them to a module: we just have a really, really
> fat LOCAL_INCLUDES list in mailnews/build.

OK.

> > > 2. The tests don't seem to be keyed off of anything?
> > 
> > As noted in comment 9.
> 
> It's not even clear to me what kind of tests these should be...

xpcshell tests. Does Thunderbird not have a way to add those? I've left the tests disabled in this patch.

I think enabling these tests should be up to the c-c devs as a follow-up.

> > > 3. The real tests that we need are that IMAP mailbox names still work
> > > (hopefully we have some non-ascii mailbox name tests in IMAP already?) and
> > > that UTF-7 message bodies can get transcoded properly (which we probably
> > > don't need).
> > 
> > That seems out of scope of this code move bug. I tested IMAP folder names
> > manually.
> 
> IMAP folder names are tested in mailnews/imap/test/test_mailboxes.js already.

OK. Cool. Then it's not critical that the tests here are disabled.
Assignee: nobody → hsivonen
Attachment #8406818 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8407350 - Flags: review?(standard8)
Attached patch hg rm more stuff (obsolete) — Splinter Review
Oops. Forgot to hg rm some test files.
Attachment #8407353 - Flags: review?(VYV03354)
Comment on attachment 8407353 [details] [diff] [review]
hg rm more stuff

Please fold two patches before landing.
Attachment #8407353 - Flags: review?(VYV03354) → review+
Comment on attachment 8407350 [details] [diff] [review]
Add the code to c-c with tests disabled

This appears to be a reverse patch, unless I'm missing something.

I also don't see any mention of the disabled tests/imported code.
Attachment #8407350 - Flags: review?(standard8)
(In reply to Mark Banner (:standard8) (away until 22 Apr) from comment #30)
> This appears to be a reverse patch, unless I'm missing something.

Oops. Sorry. I exported the patch before folding it together with the previous one.
Attachment #8407350 - Attachment is obsolete: true
Attachment #8408165 - Flags: review?(standard8)
Folded patch for landing.
Attachment #8406815 - Attachment is obsolete: true
Attachment #8407353 - Attachment is obsolete: true
Attachment #8410084 - Flags: review+
Thanks for doing this patch. This is exactly the same patch, but with end-of-line whitespace removed (I thought now was a good opportunity).

I've also fixed the tests - I'll attach the changes for those in a follow-up patch.
Attachment #8408165 - Attachment is obsolete: true
Attachment #8408165 - Flags: review?(standard8)
Attachment #8410943 - Flags: review+
Henri, can you just take a look at this and check it makes sense?

The Makefile isn't necessary, as moz.build does everything we need here. I've fixed the xpcshell.ini not to look for the head/tail files as they aren't needed.

The main bit I wasn't sure about, was copying CharsetConversionTests.js to c-c, but this seemed the best thing to go forward maintenance wise.
Attachment #8410945 - Flags: review?(hsivonen)
Attachment #8410945 - Flags: review?(hsivonen) → review+
Thank you!

What should the landing plan be?

Now or after the ESR branching?

Simultaneous landing on m-c (not to inbound) and c-c or landing the m-c patch to inbound first and then landing the c-c patch once inbound is merged and c-c burns?
(In reply to Henri Sivonen (:hsivonen) from comment #35)
> What should the landing plan be?
> 
> Now or after the ESR branching?

I don't see any issue with doing this now (once the trees re-open). The patches are a simple move of code, so I don't see any potential issues here.

> Simultaneous landing on m-c (not to inbound) and c-c or landing the m-c
> patch to inbound first and then landing the c-c patch once inbound is merged
> and c-c burns?

I'm happy for this to land on inbound first, and land the c-c patch either at the merge, or when it burns. We can let the folks who normally watch these things know.
Whiteboard: [MemShrink:P3] → [MemShrink:P3][needs c-c patch landing when m-i merges to m-c]
(In reply to Mark Banner (:standard8) from comment #36)
> I don't see any issue with doing this now (once the trees re-open).
...
> I'm happy for this to land on inbound first

OK. Thanks.

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ba6d771ae7

Note to folks who watch c-c:
When this is merged to m-c, please land attachment 8410943 [details] [diff] [review] and 8410945 to c-c.
Oops forgot another magic linkification word: Please land attachment 8410943 [details] [diff] [review] and attachment 8410945 [details] [diff] [review] to c-c.
Attachment #8410084 - Attachment description: Remove code from m-c, folded patch → Remove code from m-c, folded patch [checked in, comment 37]
Thanks for caring about c-c :)
Keywords: checkin-needed
Whiteboard: [MemShrink:P3][needs c-c patch landing when m-i merges to m-c] → [MemShrink:P3][needs c-c patches landing when m-i merges to m-c]
https://hg.mozilla.org/mozilla-central/rev/a8ba6d771ae7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
https://hg.mozilla.org/comm-central/rev/c36c9c4b65e5
https://hg.mozilla.org/comm-central/rev/e3b89706dfb6
Whiteboard: [MemShrink:P3][needs c-c patches landing when m-i merges to m-c] → [MemShrink:P3]
Attachment #8410943 - Attachment description: Add the code to c-c with tests disabled, v3 → Add the code to c-c with tests disabled, v3 [checked in, comment 41]
Attachment #8410945 - Attachment description: Fix tests in c-c → Fix tests in c-c [checked in, comment 41]
Looks like some tests still fail:
https://tbpl.mozilla.org/php/getParsedLog.php?id=38426922&tree=Thunderbird-Trunk
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/intl/uconv/tests/unit/test_bug718500.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/intl/uconv/tests/unit/test_bug718500.js | Unknown chardet: UTF-7 - See following stack:

Can anybody fix this up quickly?
Depends on: 1001113
This split creating a lot maintain burden for c-c, and only the UTF7 encoding, it's not worth that.
(In reply to Yonggang Luo from comment #44)
> This split creating a lot maintain burden for c-c, and only the UTF7
> encoding, it's not worth that.

This decision was made over a year ago and is not likely to be reversed. It's also far from the most burdensome part of intl changes for c-c.
You need to log in before you can comment on or make changes to this bug.