Closed Bug 1385416 Opened 2 years ago Closed 2 years ago

Remove Locale.jsm

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

toolkit/modules/Locale.jsm is unnecessary now. We have Services.locale.negotiateLanguages for that.
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.
Yeah! :) patch will come in a couple minutes.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
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.
Attachment #8891499 - Flags: review?(kmaglione+bmo) → review+
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 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.
Attachment #8891499 - Flags: review+ → review?(kmaglione+bmo)
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.
Attachment #8891499 - Flags: review?(kmaglione+bmo)
Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

ugh, more tests to fix
Attachment #8891499 - Flags: review?(kmaglione+bmo)
Comment on attachment 8891499 [details]
Bug 1385416 - Remove Locale.jsm.

Ok, now it seems review ready. Sorry for the bugmail noise.
Attachment #8891499 - Flags: review?(kmaglione+bmo)
Attachment #8891499 - Flags: review?(kmaglione+bmo)
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))`
Attachment #8891499 - Flags: review?(kmaglione+bmo) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/26e6fd4d9d31
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.