Closed
Bug 937056
Opened 11 years ago
Closed 10 years ago
Move UTF-7 decoder/encoder to comm-central
Categories
(MailNews Core :: Internationalization, defect)
MailNews Core
Internationalization
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?
Assignee | ||
Comment 2•11 years ago
|
||
The sooner, the better. Avoiding Thunderbird breakage is really the only reason this code is still in m-c.
Comment 3•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
The converters are components, so you just need to ensure they're registered.
Assignee | ||
Comment 5•10 years ago
|
||
(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
Assignee | ||
Comment 6•10 years ago
|
||
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]
Updated•10 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P3]
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8406091 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
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.
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
Good point. This patch moves the code under mailnews/.
Attachment #8406107 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8406144 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
Now this should even compile. The c-c build now fails for me in a place unrelated to this patch.
Comment 14•10 years ago
|
||
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
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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.
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
m-c patch: https://tbpl.mozilla.org/?tree=Try&rev=eca53df2ec2d
Comment 20•10 years ago
|
||
(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).
Assignee | ||
Comment 21•10 years ago
|
||
(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?
Comment 22•10 years ago
|
||
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.
Assignee | ||
Comment 23•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=de3d79a49546
Attachment #8406106 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
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
Comment 25•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Attachment #8406815 -
Flags: review?(VYV03354)
Comment 26•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8406815 -
Flags: review- → review+
Assignee | ||
Comment 27•10 years ago
|
||
(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)
Assignee | ||
Comment 28•10 years ago
|
||
Oops. Forgot to hg rm some test files.
Attachment #8407353 -
Flags: review?(VYV03354)
Comment 29•10 years ago
|
||
Comment on attachment 8407353 [details] [diff] [review] hg rm more stuff Please fold two patches before landing.
Attachment #8407353 -
Flags: review?(VYV03354) → review+
Comment 30•10 years ago
|
||
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)
Assignee | ||
Comment 31•10 years ago
|
||
(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)
Assignee | ||
Comment 32•10 years ago
|
||
Folded patch for landing.
Attachment #8406815 -
Attachment is obsolete: true
Attachment #8407353 -
Attachment is obsolete: true
Attachment #8410084 -
Flags: review+
Comment 33•10 years ago
|
||
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+
Comment 34•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8410945 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 35•10 years ago
|
||
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?
Comment 36•10 years ago
|
||
(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.
Updated•10 years ago
|
Whiteboard: [MemShrink:P3] → [MemShrink:P3][needs c-c patch landing when m-i merges to m-c]
Assignee | ||
Comment 37•10 years ago
|
||
(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.
Assignee | ||
Comment 38•10 years ago
|
||
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]
Comment 39•10 years ago
|
||
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]
Comment 40•10 years ago
|
||
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
Comment 41•10 years ago
|
||
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]
Updated•10 years ago
|
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]
Updated•10 years ago
|
Attachment #8410945 -
Attachment description: Fix tests in c-c → Fix tests in c-c [checked in, comment 41]
Comment 42•10 years ago
|
||
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?
Comment 43•10 years ago
|
||
JFTR
Comment 44•9 years ago
|
||
This split creating a lot maintain burden for c-c, and only the UTF7 encoding, it's not worth that.
Comment 45•9 years ago
|
||
(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.
Description
•