Closed Bug 1394215 Opened 2 years ago Closed 2 years ago

Remove remaining use if nsIAtom in mailnews after bug 1334834

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: jorgk, Assigned: jorgk)

Details

Attachments

(2 files, 3 obsolete files)

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.
One more patch to come.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8901586 - Flags: review?(acelists)
Oops, restored accidentally removed line.
Attachment #8901586 - Attachment is obsolete: true
Attachment #8901586 - Flags: review?(acelists)
Attachment #8901587 - Flags: review?(acelists)
nsIAtom† in mailnews: R.I.P.
Attachment #8901589 - Flags: review?(acelists)
Summary: Investigate the remaining use if nsIAtom in mailnews after bug 1334834 → Remove remaining use if nsIAtom in mailnews after bug 1334834
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?
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 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.
Attachment #8901589 - Flags: review?(acelists) → review+
(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?
(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
Nick, which of the two should we use? (v1b) or (v2)?
Flags: needinfo?(n.nethercote)
Then also AString would be a possibility when nsCharsetConverterManager::GetCharsetData() returns nsString and we just pass it on.
Purely academic discussion since the only consumer wants UTF-8. But I can attach a third patch to chose from ;-)
Aceman, take your pick. Since there are no JS callers and we pass ASCII data, all patches will do the same thing.
Attachment #8901587 - Attachment description: 1394215-atoms-nsICharsetConverterManager.patch (v1b) → 1394215-atoms-nsICharsetConverterManager.patch (v1b) with ACString
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
Flags: needinfo?(n.nethercote)
Attachment #8901587 - Attachment is obsolete: true
Attachment #8901587 - Flags: review?(acelists)
Attachment #8901594 - Attachment is obsolete: true
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.