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.

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 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.

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 on attachment 8891499 [details] Bug 1385416 - Remove Locale.jsm. ugh, more tests to fix

Comment on attachment 8891499 [details] Bug 1385416 - Remove Locale.jsm. Ok, now it seems review ready. Sorry for the bugmail noise.

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 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.

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/26e6fd4d9d31 Remove Locale.jsm. r=kmag