Currently, we use Chrome Registry's `global` locale package to identify available locales [0]. As we move away from Chrome Registry we also want to move away from using it as a reference location for available locales data. The new source of truth is L10nRegistry and its data is a combination of multilocale.json registered locales (package)[1] and language packs[2]. One of the benefits of this move is that we can vastly reduce the number of language negotiation states we go through during the bootstrap. At the moment we hit at least 5: 1) no locales available, defaultLocale requested 2) PreferencesService reads default prefs.js, new requested locales 3) ChromeRegistry registers packages, new available locales 4) prefs.js reads user's profile prefs.js, new requested locales 5) Extensions are registered, new available locales But in reality the number of states is much higher, since *each* time we read a new locale, ChromeRegistry registers it, triggers `available-locales-changed` and language negotiation. I'd like to vastly simplify it. The PreferencesService rewrite should handle the (2)+(4) state, and I believe that multilocale.json can handle the (1)+(3) state, leaving us with: 1) LocaleService reads multilocale.json - available locales ready 2) PreferencesService reads default prefs.js, requested locales ready 3) Extensions are registered, new available locales [0] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/intl/locale/LocaleService.cpp#119 [1] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/browser/components/nsBrowserGlue.js#675 [2] https://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1834

Comment on attachment 8945711 [details] Bug 1431260 - Migrate LocaleService::AvailableLocales from ChromeRegistry to multilocale.txt. This is pretty early, but I think the patch shapes up really nicely. ChromeRegistry stops informing LocaleService available locale selection, instead we get the list of packaged locales from multilocale.json, and then we allow L10nRegistry to override it when new sources are registered. In result we have all packaged available locales at once, and then we just add the langpacks. This should reduce the number of app-locales changes during bootstrap quite nicely. Jonathan - does it look good to you? I could also use a bit of help in getting the JSON parsing in C++ (for multilocale.json)

Ping? :)

Comment on attachment 8945711 [details] Bug 1431260 - Migrate LocaleService::AvailableLocales from ChromeRegistry to multilocale.txt. https://reviewboard.mozilla.org/r/215828/#review225168 ::: intl/locale/LocaleService.cpp:584 (Diff revision 1) > + /* RefPtr<nsZipArchive> zip = Omnijar::GetReader(Omnijar::GRE); */ > + /* if (zip) { */ > + /* nsZipItemPtr<char> item(zip, "res/multilocale.json"); */ > + /* } */ So this will find a JSON file in omnijar, and then you want to parse it and set `mPackagedLocales` from its contents, right? AFAIK, the only JSON parsing code we have in the tree is what's in spidermonkey. So I think you'd need to call JS_ParseJSON, which will give you the result as a JS::MutableHandleValue, and then extract the values from that result to stash in the C++ array here. That all sounds a bit tedious, actually. What is this "multilocale.json" going to look like, and how's it generated? If all it contains is an array of strings that represent the locale codes, then maybe JSON is overkill here (and makes it more work than necessary to read the resource)? Maybe simpler is better?

Comment on attachment 8945711 [details] Bug 1431260 - Migrate LocaleService::AvailableLocales from ChromeRegistry to multilocale.txt. I started to play with the idea of using JSAPI to get JSON, but then realized that we need to be able to read the packaged locales in order to negotiate languages in order to set locale when creating JS Context. Sooo... trying to use JSON which requires js context there would be a no-go. Instead I switched to .txt file which has the same syntax as intl.locale.requested - "ab-CD,ef,gh-IJ". I will split the patch later to get appropriate reviewers for fennec and build system, but initially, I kept it together to make it easier to test. I think the LocaleService.cpp part needs just one more thing and that's to read the file in non-packaged scenario. Jonathan - can you help me with this piece? Also, does the patch look good otherwise?

Comment on attachment 8945711 [details] Bug 1431260 - Migrate LocaleService::AvailableLocales from ChromeRegistry to multilocale.txt. https://reviewboard.mozilla.org/r/215828/#review225970 ::: intl/locale/LocaleService.cpp:587 (Diff revision 2) > + //XXX: We probably should support non-packaged builds here, > + // and once we do, we may as well abstract it to handle update.locale below. Does the `multilocale.*` file even exist in a non-packaged build? (I kinda thought it would be generated during the packaging process...) Where would it be located, then? ::: intl/locale/LocaleService.cpp:592 (Diff revision 2) > + //XXX: We probably should support non-packaged builds here, > + // and once we do, we may as well abstract it to handle update.locale below. > + RefPtr<nsZipArchive> zip = Omnijar::GetReader(Omnijar::GRE); > + if (zip) { > + nsAutoCString localesString; > + nsZipItemPtr<char> item(zip, "res/multilocale.json"); We'd better change the name of this file, if it no longer uses JSON format! ::: intl/locale/LocaleService.cpp:593 (Diff revision 2) > + size_t len = item.Length(); > + // Ignore any trailing spaces, newlines, etc. > + while (len > 0 && item.Buffer()[len - 1] <= ' ') { > + len--; > + } > + localesString.Assign(item.Buffer(), len); Rather than manually trimming trailing space here, I'd suggest just doing localesString.Assign(item.Buffer(), item.Length()); localesString.Trim(" \t\n\r"); (which would trim both leading and trailing whitespace, if present).

> Does the `multilocale.*` file even exist in a non-packaged build? (I kinda thought it would be generated during the packaging process...) Where would it be located, then? ▶ cat obj-x86_64-pc-linux-gnu-dbg/dist/bin/res/multilocale.txt en-US,it ▶ cat obj-x86_64-pc-linux-gnu-dbg/dist/bin/update.locale en-US

Comment on attachment 8945711 [details] Bug 1431260 - Migrate LocaleService::AvailableLocales from ChromeRegistry to multilocale.txt. `hg split` is amazing :) Updated the patch to your comments.

Looking pretty good, I think. Here's a suggested update (applies on top of your patches) to handle reading the locales in non-packaged builds. (Make sure to test this before incorporating it -- I checked that it compiles, but that's as far as I went.)

Comment on attachment 8950979 [details] Bug 1431260 - Switch Android code to read multilocale.txt. https://reviewboard.mozilla.org/r/220236/#review226400 ::: mobile/android/base/java/org/mozilla/gecko/BrowserLocaleManager.java:431 (Diff revision 1) > try { > - final JSONObject multilocale = new JSONObject(contents); > - final JSONArray locales = multilocale.getJSONArray("locales"); > + String[] values = contents.split(","); > + final Set<String> out = new HashSet<String>(Arrays.asList(values)); > - if (locales == null) { > - Log.e(LOG_TAG, "No 'locales' array in multilocales.json!"); > - return null; > - } > - > - final Set<String> out = new HashSet<String>(locales.length()); > - for (int i = 0; i < locales.length(); ++i) { > - // If any item in the array is invalid, this will throw, > - // and the entire clause will fail, being caught below > - // and returning null. > - out.add(locales.getString(i)); > - } > > return out; > } catch (JSONException e) { > Log.e(LOG_TAG, "Unable to parse multilocale.json.", e); > return null; > } Looks like the try/catch here is obsolete.

Hoah! I annotated the whole LocaleService to analyze the startup, the lines starting with "I" denote XPCOM interface methods. Some methods have two log lines - first line describing the call start, second describing the return value at the end. The investigation led me to a couple small optimizations, but I think it overall looks good! I'm attaching a log file with the *worst* case scenario - non-default locale using a langpack. This is not optimal yet, because langpack is registered late, and requested locale from the profile is also coming late. The latter will be soon resolved by the Prefs revmap hopefully, the former would require some more eager caching and early startup in WE which is hard because the code in WE uses JS and we would like to have the locale set before the first JS Context is even created. Here are things I noticed while reading this log: - There are *a lot* of `I GetAppLocaleAsLangTag`. I suspect vast majority of them come from ChromeRegistry which now reads locale for each manifest entry to be registered. Fortunately, the method is fast, and I don't think it makes sense to cache it in ChromeRegistry because then we'd have to invalidate that cache, but maybe :jfkthame will suggest it? - We now do exactly two language renegotiation in that scenario: - One after Gecko registers user's prefs.js and intl.locale.requested changes - Another when Addons code registers the langpack - In the best scenario (default locale, no langpacks) we do zero language renegotiations during startup! - There's a long chain of NegotiateLanguages coming from somewhere. Maybe a bug, but not in our code.

Now that I think about it - Jonathan, would it be faster/better if ChromeRegistry (C++) called a regular GetAppLocaleAsLangTag (which we'd have to add) rather than NS_IMETHOD or is there no difference?

This is ready for review! :jfkthame - I cleaned up, documented the patch, added tests mostly for new things, but also for several red spots from [0], and added several minor changes (like client process not reading update.locale, or not triggering LocalesChanged from SetAvailableLocales if the locales are the same) that should help with performance. :gps - This is pretty trivial change from multilocale.json to multilocale.txt across the build system. We need txt because in Gecko we want to read this file earlier than any JS context is created, so we can't use JSON parser. :rnewman - Simple change for Android, to help Gecko with reading the file early. [0] https://codecov.io/gh/marco-c/gecko-dev/src/master/intl/locale/LocaleService.cpp

Hmmm, I'm not sure what's going on. The current try is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91e0e9dec8d2706a00e7808adafedb33429dd554 It fails on all platforms in staging with: ``` 0:08.73 ERROR: The following duplicated files are not allowed: 0:08.73 update.locale 0:08.73 res/multilocale.txt 0:08.76 make[3]: *** [/home/zbraniecki/projects/mozilla-unified/toolkit/mozapps/installer/packager.mk:37: stage-package] Error 1 0:08.76 make[3]: Leaving directory '/home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/browser/installer' 0:08.76 make[2]: *** [/home/zbraniecki/projects/mozilla-unified/toolkit/mozapps/installer/packager.mk:109: make-package] Error 2 ``` I'm a bit lost because I'm sure that a) I didn't touch `update.locale` and I did change `multilocale.json` to `multilocale.txt` in every place in mozilla-central I could find. May need help with debugging that :(

The reason it was reporting it is because in the default build now those two files have the same content. I added both to allowed-dupes files. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f42042413461f43cf22710830a34a7a9bdfb44

The latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba6d8a69b1ddadb24e7c9f4b00176a7d3ac220b It has one pending test failure in the browser/components/extensions/test/browser/browser_ext_pageAction_context.js The reason for the failure is that the test registers locales using chrome registry, which is no longer supported[0]. This test has been added in bug 1357902 by :kmag. Kris, can you advise on how should I update the test infra in browser/components/extensions/test/browser/ to register locales via L10nRegistry rather than ChromeRegistry? If you need to just mock it, we can now use `Services.locale.setAvailableLocales(['es-ES']);` which should negotiate down to es-ES if requested is es-ES as well, and then extensions should get it. [0] https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/components/extensions/test/browser/locale/chrome.manifest#1

Alternatively, you can use `L10nRegistry` to register a new FileSource for es-ES just like langpacks do.

Seems like the same issue is with https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_startup_cache.js - it also tries to register locale via chrome.manifest.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20) > Now that I think about it - Jonathan, would it be faster/better if > ChromeRegistry (C++) called a regular GetAppLocaleAsLangTag (which we'd have > to add) rather than NS_IMETHOD or is there no difference? What we should do, actually, is just declare LocaleService as a 'final' class; we're not intending anyone to inherit from it. Then callers like ChromeRegistry, when they call a (virtual, because NS_IMETHOD) method via LocaleService::GetInstance()->foo will be able to call the specific implementation directly, rather than dispatching through the vtable. So that'll make such calls fractionally cheaper.

Comment on attachment 8945711 [details] Bug 1431260 - Migrate LocaleService::AvailableLocales from ChromeRegistry to multilocale.txt. https://reviewboard.mozilla.org/r/215828/#review226758 ::: intl/locale/LocaleService.cpp:955 (Diff revision 8) > + AutoTArray<nsCString, 100> packagedLocales; > + GetPackagedLocales(packagedLocales); > + > + *aCount = packagedLocales.Length(); > + *aOutArray = CreateOutArray(packagedLocales); This ends up copying `mPackagedLocales` to a temporary local array, and then building the output array from that, which seems unfortunate. We could just do *aCount = mPackagedLocales.Length(); *aOutArray = CreateOutArray(mPackagedLocales); directly, except for the one-time initialization case when `mPackagedLocales` gets populated. So I'd suggest factoring out the code to populate `mPackagedLocales` into a helper, so that here we can do if (mPackagedLocales.IsEmpty()) { InitPackagedLocales(); MOZ_ASSERT(!mPackagedLocales.IsEmpty()); } *aCount = .... and avoid the extra array copy.

Green try! https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa47186c9699b854f93a34149173b090d41de9a2

Comment on attachment 8945711 [details] Bug 1431260 - Migrate LocaleService::AvailableLocales from ChromeRegistry to multilocale.txt. https://reviewboard.mozilla.org/r/215828/#review227508 LGTM! :)

Comment on attachment 8951826 [details] Bug 1431260 - Migrate Extensions tests to mock locale availability. https://reviewboard.mozilla.org/r/221120/#review227580

Comment on attachment 8950979 [details] Bug 1431260 - Switch Android code to read multilocale.txt. https://reviewboard.mozilla.org/r/220236/#review227622 ::: mobile/android/base/java/org/mozilla/gecko/BrowserLocaleManager.java:429 (Diff revision 8) > if (contents == null) { > // GeckoJarReader logs and swallows exceptions. > return null; > } > > - try { > + String[] values = contents.split(","); Are you sure this file will never have spaces? Do you have a build step that fails the build if it's malformed? Any of these: ``` "" "en-US, be, ca" "en-US, be," "en-US,be, " ``` will all return junk from this function: respectively, a single empty entry, lang tags with leading spaces, a final empty entry, and a final entry that's just a space.

Comment on attachment 8950979 [details] Bug 1431260 - Switch Android code to read multilocale.txt. https://reviewboard.mozilla.org/r/220236/#review227676

Comment on attachment 8950978 [details] Bug 1431260 - Switch multilocale.json to multilocale.txt in the build system. https://reviewboard.mozilla.org/r/220234/#review228014 This seems like a (mostly) relatively straightforward search and replace. The changes to allowed-dupes.mn don't seem to belong in this commit. I assume they are part of the larger refactor (which I didn't look at at all).

> The changes to allowed-dupes.mn don't seem to belong in this commit. I assume they are part of the larger refactor (which I didn't look at at all). The change is part of this commit. The build system was rejecting the fact that now `update.locale` and `multilocale.txt` may have the same value - "en-US" for example.

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f0e06a115f0f Migrate LocaleService::AvailableLocales from ChromeRegistry to multilocale.txt. r=jfkthame https://hg.mozilla.org/integration/autoland/rev/5dae26ba399f Switch multilocale.json to multilocale.txt in the build system. r=gps https://hg.mozilla.org/integration/autoland/rev/dc3f1141af95 Switch Android code to read multilocale.txt. r=rnewman https://hg.mozilla.org/integration/autoland/rev/f0a6945be22c Migrate Extensions tests to mock locale availability. r=aswan

Jorg - you may want to take a look at this from the Tb/Sm perspective. We basically move away from using Chrome Registry for registering available locales to use L10nRegistry. I know you don't use Fluent in Tb/Sm yet, but the solution is still simple - you just need to register a source in L10nRegistry with the packaged locales. Basically an equivalent of this: https://hg.mozilla.org/integration/autoland/file/tip/browser/components/nsBrowserGlue.js#l719 somewhere in your startup code. Sorry for late notice!

So you want me to put these there lines + let locales = Services.locale.getPackagedLocales(); + const appSource = new FileSource("app", locales, "resource://app/localization/{locale}/"); + L10nRegistry.registerSource(appSource); Where do you think would be a good spot? https://dxr.mozilla.org/comm-central/rev/91311b1d6aa3bddd427858cb7f8c149a81c71a92/mail/components/mailGlue.js#54 What will happen without that code? Oh, and where does 'L10nRegistry' come from?

Probably safest is to do what browser does - `final-ui-startup` - https://dxr.mozilla.org/comm-central/rev/91311b1d6aa3bddd427858cb7f8c149a81c71a92/mail/components/mailGlue.js#58 > What will happen without that code? That's a great question! This will likely mess up a scenario where the user is using a langpack, because we now let L10nRegistry "set" the list of available locales which is then used to negotiate languages for the app. So, a scenario would be like this: 1) User installs Thunderbird in Italian 2) LocaleService fetches AvailableLocales from PackagedLocales // ["it"] 3) User installs language pack for French 4) L10nRegistry calls `LocaleService.setAvailableLocales(L10nRegistry.getAvailableLocales());` and since you didn't register `it` into L10nRegistry, now it sets the LocaleService avialable locales to ["fr"] 5) Italian becomes unavailable in the build I know it's an edge scenario involving langpack installation and trying to use the packaged locale, but still :) > Oh, and where does 'L10nRegistry' come from? const { L10nRegistry, FileSource } = ChromeUtils.import("resource://gre/modules/L10nRegistry.jsm", {});

https://hg.mozilla.org/mozilla-central/rev/f0e06a115f0f https://hg.mozilla.org/mozilla-central/rev/5dae26ba399f https://hg.mozilla.org/mozilla-central/rev/dc3f1141af95 https://hg.mozilla.org/mozilla-central/rev/f0a6945be22c