1470716 - Remove ExtensibleStringBundle support

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[Kris Maglione [:kmag]]

These aren't used by any in-tree code anymore.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1470716: Remove unused ExtensibleStringBundle support.

Review commit: https://reviewboard.mozilla.org/r/252582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252582/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8987325 [details]
Bug 1470716: Remove unused ExtensibleStringBundle support.

https://reviewboard.mozilla.org/r/252582/#review259438

Saying our goodbyes to 1999 code feels good. lgtm! :)

Comment 3

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b1c959442d70238ded401204860504eb3a601f
Bug 1470716: Remove unused ExtensibleStringBundle support. r=gandalf

Comment 4

[Kris Maglione [:kmag]]

Jorg, there's one no-op use of this in comm-central that you'll probably want to just remove.

Comment 5

[Jorg K (CEST = GMT+2)]

Hmm, you're referring to:
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/intl/nsCharsetConverterManager.cpp#54
return sbServ->CreateExtensibleBundle(aCategory, aResult);

Why is that a no-op? That's not so obvious to me, what am I missing?

Comment 6

[Kris Maglione [:kmag]]

(In reply to Jorg K (GMT+2) from comment #5)
> Hmm, you're referring to:
> https://dxr.mozilla.org/comm-central/rev/
> 51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/intl/
> nsCharsetConverterManager.cpp#54
> return sbServ->CreateExtensibleBundle(aCategory, aResult);
> 
> Why is that a no-op? That's not so obvious to me, what am I missing?

Extensible string bundles only work if there are category entries registered to add string bundles to them. comm-central doesn't have any.

It's possible that some extensions may provide entries for that extensible bundle. If that's the case and you want to continue supporting it, you could probably move the old extensible string bundle code to a separate service, or reimplement it in JS. I don't really have an easy way of finding whether any add-ons do use it, but I couldn't find any by googling the category name, and it seems rather unlikely.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1470716-c-c-part.patch

Hmm, I'm not really sure what the replacement should be, maybe just CreateBundle(). This compiles and the charset menu appears to work. Sorry about the ignorance, I can't really know everything.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8989023 [details] [diff] [review]
1470716-c-c-part.patch

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

I believe you'll need to replace the *_CATEGORY constants with these URLs:

https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/build/nsMailModule.cpp#1321-1322

and then remove the category registrations/original category constants.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1470716-c-c-part.patch

How about this one then. I wasn't sure about that category stuff.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8989025 [details] [diff] [review]
1470716-c-c-part.patch

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

lgtm

::: mailnews/intl/nsCharsetConverterManager.cpp
@@ +42,5 @@
>    NS_IF_RELEASE(sTitleBundle);
>  }
>  
>  static
> +nsresult LoadBundle(const char* aCategory,

Should rename this to something like aURL. Also, please fix indentation of the next line.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1470716-c-c-part.patch

Thanks, Kris!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/67e3d766d515
C-C part: Replace call to LoadExtensibleBundle(). r=kmag DONTBUILD

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f6b1c959442d