1006498 - Import nsCharsetConverterManager into c-c

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Henri Sivonen (:hsivonen)]

Bug 943268 will remove nsCharsetConverterManager from m-c. It needs to be imported into c-c to avoid c-c burning.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: 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.

 * 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.

Comment 3

[Hartmut Figge]

(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. ;)

Comment 4

[Henri Sivonen (:hsivonen)]

(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."

Comment 5

[Joshua Cranmer [:jcranmer]]

(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?

Comment 6

[Henri Sivonen (:hsivonen)]

I tried 4 or 5 different paths locally.

Comment 7

[Philip Chee]

Attachment: WIP patch (not working)

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'

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Barely-tested port [checked in]

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.

Comment 9

[Joshua Cranmer [:jcranmer]]

Oh geez. We can't actually use nsIScriptableUConv anymore with our own charsets because that was broken by mozilla-central.

Comment 10

[[e-mail]]

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.]

Comment 11

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] 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).

Comment 12

[Henri Sivonen (:hsivonen)]

(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.

Comment 13

[Henri Sivonen (:hsivonen)]

(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.)

Comment 14

[Joshua Cranmer [:jcranmer]]

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.

Comment 15

[Joshua Cranmer [:jcranmer]]

Attachment: Fix charset alias

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.

Comment 16

[Henri Sivonen (:hsivonen)]

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.

Comment 17

[Henri Sivonen (:hsivonen)]

Also, the catch blocks should say windows-1252 instead of iso-8859-1.

Comment 18

[Philip Chee]

Attachment: SeaMonkey patch as checked-in rs=bustage fix. [checkin comment 18]

> 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

Comment 19

[Joshua Cranmer [:jcranmer]]

Attachment: Fix charset alias

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.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8421870 [details] [diff] [review]
Fix charset alias

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

Looks good to me r=Standard8

Comment 21

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/comm-central/rev/d7b54421771a