Closed Bug 1006498 Opened 7 years ago Closed 7 years ago

Import nsCharsetConverterManager into c-c

Categories

(MailNews Core :: Internationalization, defect)

defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 32.0

People

(Reporter: hsivonen, Assigned: jcranmer)

References

Details

Attachments

(3 files, 3 obsolete files)

Bug 943268 will remove nsCharsetConverterManager from m-c. It needs to be imported into c-c to avoid c-c burning.
Attached patch Patch to get you started (obsolete) — Splinter Review
Problems with this patch:

 * charsetalias.properties.h is included in the patch. The build system should generate it from charsetalias.properties using props2arrays.py (in m-c), but the Makefile.in incantation that works for m-c doesn't work for me in c-c.

 * The resource paths for charsetData.properties and charsetTitles.properties are placeholders and jar manifest entries are missing.

 * A bunch of this stuff is dead code or in need of refactoring. See bug 943268 comment 15.
Duplicate of this bug: 1008077
(In reply to Henri Sivonen (:hsivonen) from comment #1)
> Created attachment 8418009 [details] [diff] [review]
> Patch to get you started
> 
> Problems with this patch:
[...]

Well, at least it fixes the bustage for my SM Linux x86_64. ;)
(In reply to Hartmut Figge from comment #3)
> (In reply to Henri Sivonen (:hsivonen) from comment #1)
> > Created attachment 8418009 [details] [diff] [review]
> > Patch to get you started
> > 
> > Problems with this patch:
> [...]
> 
> Well, at least it fixes the bustage for my SM Linux x86_64. ;)

It fixes the *build* bustage. The resulting app doesn't actually *work* due to "The resource paths for charsetData.properties and charsetTitles.properties are placeholders and jar manifest entries are missing."
(In reply to Henri Sivonen (:hsivonen) from comment #1)
> Created attachment 8418009 [details] [diff] [review]
> Patch to get you started
> 
> Problems with this patch:
> 
>  * charsetalias.properties.h is included in the patch. The build system
> should generate it from charsetalias.properties using props2arrays.py (in
> m-c), but the Makefile.in incantation that works for m-c doesn't work for me
> in c-c.

Maybe because you never bothered to include a proper path to props2arrays.py?
I tried 4 or 5 different paths locally.
Attached patch WIP patch (not working) (obsolete) — Splinter Review
This is what I have so far. It's failing with:

> processing c:/t1/hg/comm-central/mailnews/intl/jar.mn
> Traceback (most recent call last):
>   File "c:\DEV\mozilla-build\python\Lib\runpy.py", line 162, in _run_module_as_main
>     "__main__", fname, loader, pkg_name)
>   File "c:\DEV\mozilla-build\python\Lib\runpy.py", line 72, in _run_code
>     exec code in run_globals
>   File "c:\t1\hg\comm-central\mozilla\python\mozbuild\mozbuild\action\jar_maker.py", line 15, in <module>
>     sys.exit(main(sys.argv[1:]))
>   File "c:\t1\hg\comm-central\mozilla\python\mozbuild\mozbuild\action\jar_maker.py", line 11, in main
>     return mozbuild.jar.main(args)
>   File "c:\t1\hg\comm-central\mozilla\python\mozbuild\mozbuild\jar.py", line 531, in main
>     jm.makeJar(infile, options.j)
>   File "c:\t1\hg\comm-central\mozilla\python\mozbuild\mozbuild\jar.py", line 249, in makeJar
>     raise RuntimeError(l)
> RuntimeError: @AB_CD@.jar:
> c:/t1/hg/comm-central/config/rules.mk:1274: recipe for target 'libs' failed
> mozmake[1]: *** [libs] Error 1
> mozmake[1]: Leaving directory 'c:/t1/hg/objdir-sm/mailnews/intl'
> c:/t1/hg/comm-central/config/rules.mk:512: recipe for target 'all' failed
> mozmake: *** [all] Error 2
> mozmake: Leaving directory 'c:/t1/hg/objdir-sm/mailnews/intl'
This compiles. I can save a message in "multibyte" charsets, and the old charset selector decides that they get the ?B? bit in 2047 encoding. So I assume that the code works.

I can't vouch for tests due to bug 1004098 breaking us horribly.
Attachment #8418009 - Attachment is obsolete: true
Attachment #8420293 - Attachment is obsolete: true
Attachment #8420366 - Flags: review?(neil)
Oh geez. We can't actually use nsIScriptableUConv anymore with our own charsets because that was broken by mozilla-central.
Comment on attachment 8420366 [details] [diff] [review]
Barely-tested port [checked in]

>+LOCAL_INCLUDES += [
>+    '/mozilla/intl/locale/src',
>+]
This definitely works does it? (I don't think UNIFIED_SOURCES likes it for instance.)

>+++ b/mailnews/intl/jar.mn
>@@ -0,0 +1,6 @@
>+# 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/.
>+
>+toolkit.jar:
>+    res/charsetData.properties                         (charsetData.properties)
[I found the bug that created this and still don't see how it works.]
Attachment #8420366 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #10)
> Comment on attachment 8420366 [details] [diff] [review]
> Barely-tested port
> 
> >+LOCAL_INCLUDES += [
> >+    '/mozilla/intl/locale/src',
> >+]
> This definitely works does it? (I don't think UNIFIED_SOURCES likes it for
> instance.)

It produces -I$(topsrcdir)/mozilla/intl/locale/src in backend.mk. The main reason I never ported local includes to moz.build in comm-central was because I didn't want to have to adjust the c-c/m-c merge script to handle /mozilla includes, not because they didn't work.

> >+++ b/mailnews/intl/jar.mn
> >@@ -0,0 +1,6 @@
> >+# 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/.
> >+
> >+toolkit.jar:
> >+    res/charsetData.properties                         (charsetData.properties)
> [I found the bug that created this and still don't see how it works.]

What do you mean? The problem in earlier versions was the localization setup. Take out the localization portion, and the jar.mn doesn't have any problems...

FWIW, I've requested backout of bug 943268 due to not being able to pick up non-EncodingUtils scriptable converters, which breaks three of our tests and some of our code. So I'm holding off on landing this patch until that is resolved. (It also seems that bug 943268 broke some Firefox OS stuff, so maybe it will get resolved much sooner).
(In reply to Joshua Cranmer [:jcranmer] from comment #9)
> Oh geez. We can't actually use nsIScriptableUConv anymore with our own
> charsets because that was broken by mozilla-central.

You can if you set isInternal to true on the nsIScriptableUConv and use the Gecko-canonical name.
(In reply to Joshua Cranmer [:jcranmer] from comment #11)
> So I'm holding off on landing this patch until that is
> resolved.

I don't intend to back out. Instead (subject to review), I intend to land bug 1008832.

(Before landing bug 943268, I reviewed c-c uses of nsIConverterInput stream to make sure those don't need non-Encoding Standard encodings.)
Depends on: 1008832
Comment on attachment 8420366 [details] [diff] [review]
Barely-tested port [checked in]

I checked in the first patch:
https://hg.mozilla.org/comm-central/rev/e41e714750eb

The tree is still closed until I can sort out test failures.
Attachment #8420366 - Attachment description: Barely-tested port → Barely-tested port [checked in]
Attached patch Fix charset alias (obsolete) — Splinter Review
I'll happily take whomever gets to review this first.

The removal of test_bug718500.js is due to later bustage by Henri removing the x-johab and TI-6.820 encoders. The test's value isn't enough for me to want to keep resetting it every time a decoder gets killed from mozilla-central. We only use one of the functions tested, and quite frankly, even there, we could probably kill it.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #8421494 - Flags: review?(standard8)
Attachment #8421494 - Flags: review?(neil)
Attachment #8421494 - Flags: review?(irving)
I suggest marking UTF-7 no longer internal now that the Web parsing code no longer uses  this code. By exposing all internal encodings to email, you expose too much--more than just UTF-7, which seems to be what you are after by changing to GetPreferredInternal.
Also, the catch blocks should say windows-1252 instead of iso-8859-1.
> Tue May 13 2014 21:58:41
> Error: NS_ERROR_XPC_BAD_IID: Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]
> Source file: chrome://messenger/content/messengercompose/MsgComposeCommands.js
> Line: 108

> gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager);

This is the SeaMonkey equivalent for Thunderbird comm-central changeset ac9aa13e8d1c
This moves UTF-7 to be non-internal, per hsivonen's advice.

I didn't change the charsets in the catch blocks because those are tests and those lines arguably shouldn't be reached anyways.
Attachment #8421494 - Attachment is obsolete: true
Attachment #8421494 - Flags: review?(standard8)
Attachment #8421494 - Flags: review?(neil)
Attachment #8421494 - Flags: review?(irving)
Attachment #8421870 - Flags: review?(standard8)
Attachment #8421870 - Flags: review?(neil)
Attachment #8421870 - Flags: review?(irving)
Comment on attachment 8421870 [details] [diff] [review]
Fix charset alias

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

Looks good to me r=Standard8
Attachment #8421870 - Flags: review?(standard8)
Attachment #8421870 - Flags: review?(neil)
Attachment #8421870 - Flags: review?(irving)
Attachment #8421870 - Flags: review+
https://hg.mozilla.org/comm-central/rev/d7b54421771a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 32.0
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.