1385416 - Remove Locale.jsm

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

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

toolkit/modules/Locale.jsm is unnecessary now. We have Services.locale.negotiateLanguages for that.

Comment 1

[Marco Castelluccio [:marco]]

Are you taking care of this? I had a patch to remove the Preferences.jsm import from Locale.jsm, but if you are removing it then it isn't needed.

Comment 2

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

Yeah! :) patch will come in a couple minutes.

Comment 3

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

Attachment: Bug 1385416 - Remove Locale.jsm.

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

Comment 4

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162622/diff/1-2/

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

https://reviewboard.mozilla.org/r/162622/#review168000

r=me to land in 57, or to land the changes other than the Locale.jsm removal in 56 and the actual removal in 57. But a lot of extensions still use Locale.jsm, so we can't remove it before then.

::: toolkit/components/extensions/Extension.jsm:1058
(Diff revision 2)
>        });
>  
> -      let match = Locale.findClosestLocale(localeList);
> -      locale = match ? match.name : this.defaultLocale;
> +      let locale = Services.locale.negotiateLanguages(
> +        Service.locales.getAppLocalesAsLangTags(),
> +        localeList,
> +        this.defaultLocale || Services.locale.defaultLocale,

No need for the `||`. `this.defaultLocale` is guaranteed to be defined.

::: toolkit/components/extensions/ExtensionCommon.jsm:1349
(Diff revision 2)
>  
>  
>    get uiLocale() {
>      // Return the browser locale, but convert it to a Chrome-style
>      // locale code.
> -    return Locale.getLocale().replace(/-/g, "_");
> +    return Services.locale.getAppLocaleAsLangTag().replace(/-/g, "_");

I'm pretty sure we want to keep using BCP47 for this.

Comment 6

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162622/diff/2-3/

Comment 7

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

https://reviewboard.mozilla.org/r/162622/#review168000

> I'm pretty sure we want to keep using BCP47 for this.

I am surprised. BCP47 means locale code like "ja-JP-x-lvariant-mac", while LangTag for it is "ja-JP-mac".

But, this is out of scope for this bug now :)

Comment 8

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162622/diff/3-4/

Comment 9

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162622/diff/4-5/

Comment 10

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Geez, I'm ashamed to admit how long it took me to understand this code.

And I apologies for not fixing the naming, but since my starting point is `this.locales`, which, by no means is a list of locale codes, I don't feel in the right position to rename it.

I left the docstrings to help any lost soul who'll ever try to touch this code.

Comment 11

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

p.s.

I'm pretty sure that in the Extension.jsm you want LangTags, and that you want the Service.locales.langNegStrategyLookup, to stop on the first result.

Comment 12

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162622/diff/5-6/

Comment 13

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

ugh, more tests to fix

Comment 14

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162622/diff/6-7/

Comment 15

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Ok, now it seems review ready. Sorry for the bugmail noise.

Comment 16

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162622/diff/7-8/

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

https://reviewboard.mozilla.org/r/162622/#review170086

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4819
(Diff revision 8)
> +    let locales = this.locales
> +      .map(loc => loc.locales)
> +      .reduce((a, b) => a.concat(b), []);

Nit: `let locales = [].concat(...this.locales.map(loc => loc.locales))`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4828
(Diff revision 8)
> +    let requestedLocales = Services.locale.getRequestedLocales();
> +
> +    /**
> +     * If en is not the top locale, add "en-US" to the list.
> +     */
> +    if (requestedLocales[0].substring(0, 3) != "en-") {

Nit: `if (!requestedLocales[0].startsWith("en-"))`

Also, can this ever be just "en", or not all-lower-case?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4853
(Diff revision 8)
> +      for (let locale of this.locales) {
> +        if (locale.locales.includes(bestLocale)) {
> +          this._selectedLocale = locale;
> +          break;
> +        }
> +      }

Nit: `this._selectedLocale = this.locales.find(loc => loc.locales.includes(bestLocale))`

Comment 18

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

https://reviewboard.mozilla.org/r/162622/#review170086

> Nit: `if (!requestedLocales[0].startsWith("en-"))`
> 
> Also, can this ever be just "en", or not all-lower-case?

Nope, we normalize cases in LocaleService.

Comment 19

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

Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162622/diff/8-9/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26e6fd4d9d31
Remove Locale.jsm. r=kmag

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/26e6fd4d9d31