1394215 - Remove remaining use if nsIAtom in mailnews after bug 1334834

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Jorg K (CEST = GMT+2)]

In bug 1392883 M-C might remove the COM interface to nsIAtom.

After removing most usage of nsIAtom in bug 1334834 we should look at what's left:
mailnews/intl/nsCharsetConverterManager.cpp
mailnews/mime/src/mimemoz2.cpp

The removal there is trivial, we just need to remove
*aResult = NS_Atomize(langGroup).take();
and
rv = langGroupAtom->ToUTF8String(fontLang);
and switch the interface to strings.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1394215-atoms-nsICharsetConverterManager.patch (v1)

One more patch to come.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1394215-atoms-nsICharsetConverterManager.patch (v1b) with ACString

Oops, restored accidentally removed line.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1394215-atoms-nsMsgDBView.patch (v1)

nsIAtom† in mailnews: R.I.P.

Comment 4

[:aceman]

Comment on attachment 8901587 [details] [diff] [review]
1394215-atoms-nsICharsetConverterManager.patch (v1b) with ACString

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

::: mailnews/intl/nsCharsetConverterManager.cpp
@@ +189,5 @@
>        aCharset, u".LangGroup", langGroup);
>  
>    if (NS_SUCCEEDED(rv)) {
> +    ToLowerCase(langGroup); // use lowercase for all language groups
> +    aResult = NS_ConvertUTF16toUTF8(langGroup);

So what values can langGroup get (from nsCharsetConverterManager::GetCharsetData) ? Can there be UTF16 characters? Will converting it to nsCString (and passing out to JS) work?

::: mailnews/intl/nsICharsetConverterManager.idl
@@ +57,5 @@
>       * @throws if aCharset is an unknown charset.
>       * @return the language code for the character encoding.
>       */
> +    ACString getCharsetLangGroup(in string aCharset);
> +    ACString getCharsetLangGroupRaw(in string aCharset);

How does passing CString out to JS work? Will it be automatically converted to JS string?

Comment 5

[Jorg K (CEST = GMT+2)]

Show me a JS caller and we discuss the question further. As far as I know, it's all ASCII, but I can s/ACString/AUTF8String/. Would you prefer that?

Comment 6

[:aceman]

Comment on attachment 8901589 [details] [diff] [review]
1394215-atoms-nsMsgDBView.patch (v1)

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

Strange that this unused leftovers are here.

Comment 7

[:aceman]

(In reply to Jorg K (GMT+2) from comment #5)
> Show me a JS caller and we discuss the question further. As far as I know,
> it's all ASCII, but I can s/ACString/AUTF8String/. Would you prefer that?

I don't know. No addon uses this. I don't know how this works and what is the difference to AUTF8String. I think JS only has one string type and that is UTF-16. So how is nsCString automatically converted to JS string when JS accesses an interface like this? Can we as someone?

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #7)
Given that we pass ASCII strings and there is no JS use, neither in TB nor in add-ons, the discussion is purely academic.

However, here is the documentation:
https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL
AUTF8String 	const nsACString& 	nsACString& 	string 	Full Unicode set permitted (translated to UTF-8)
ACString 	const nsACString& 	nsACString& 	string 	Only chars in range \u0000-\u00ff permitted

Note that Nicholas used AUTF8String here:
https://hg.mozilla.org/comm-central/rev/351c858c276c#l5.14

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1394215-atoms-nsICharsetConverterManager.patch (v2) with AUTF8String

Nick, which of the two should we use? (v1b) or (v2)?

Comment 10

[:aceman]

Then also AString would be a possibility when nsCharsetConverterManager::GetCharsetData() returns nsString and we just pass it on.

Comment 11

[Jorg K (CEST = GMT+2)]

Purely academic discussion since the only consumer wants UTF-8. But I can attach a third patch to chose from ;-)

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1394215-atoms-nsICharsetConverterManager.patch (v3) with AString

Aceman, take your pick. Since there are no JS callers and we pass ASCII data, all patches will do the same thing.

Comment 13

[Nicholas Nethercote [inactive]]

AUTF8String is probably best.

aceman, which string type you use doesn't affect how JS code is written, but it does affect how the C++ code is written.

XPConnect is responsible for the necessary conversions across the language barrier. The two functions that do it (one for each direction) are here:

http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#103
http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#393

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4cdc9415f29b
remove use of nsIAtom in nsICharsetConverterManager.idl. r=njn
https://hg.mozilla.org/comm-central/rev/799ad314120f
remove unused traces of nsIAtom from nsMsgDBView.cpp/h. r=aceman