Second part of cleanups and unification after we got rid of GetApplicationLocale uses, is to migrate all calls to ChromeRegistry::GetSelectedLocale. Should be fairly straightforward for most calls that already ask for "global" package anyway.

Comment on attachment 8847326 [details] Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. this is not yet ready for review

Ok, try seems green! ready for review :)

(I'll fix ES linting before landing)

Comment on attachment 8847326 [details] Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. https://reviewboard.mozilla.org/r/120326/#review123384 I looked at the C++ files here, but so much of this is JS/front-end code that it'd probably be better to get review from someone who speaks the language and knows better what to look out for. :) ::: intl/unicharutil/util/ICUUtils.cpp:75 (Diff revision 3) > } > > if (mCurrentFallbackIndex < 2) { > mCurrentFallbackIndex = 2; > - // Else try the user-agent's locale: > - nsCOMPtr<nsIToolkitChromeRegistry> cr = > + // Else take the app's locale: > + trailing whitespace

Comment on attachment 8847326 [details] Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. Thanks! Pike agreed to look at the JS side. Can you r+ the part you looked at?

Comment on attachment 8847326 [details] Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. https://reviewboard.mozilla.org/r/120326/#review123480 ::: commit-message-2baef:1 (Diff revision 3) > +Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. The commit message should be more descriptive of what you're changing and why. Notably, it should explain why undefined is a good substitute for locale in dates etc, and why you should AsLangTag or BCP when replacing getSelectedLocale. Doing an bz-inspired r- based on just the commit message. Happy to do the actual code-review when I can compare the changes to the intent as described in the commit message.

Comment on attachment 8847326 [details] Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. https://reviewboard.mozilla.org/r/120326/#review123494 I've got a few more nits and items to get clarified. Mostly they're existing issues, but we'll never look at them again if we don't go over them now. And yes, for the C++ code, we need jfkthame, in particular because I don't know what the intent of the locale usage is in those areas of our code. ::: browser/components/places/tests/chrome/test_treeview_date.xul:139 (Diff revision 4) > } else if (node.uri != "http://before.midnight.com/") { > // Avoid to test spurious uris, due to how the test works > // a redirecting uri could be put in the tree while we test. > break; > } > let timeStr = timeObj.toLocaleString(locale, dtOptions); Should we use `undefined` here now? And then drop getting the bcp locale above? ::: browser/experiments/Experiments.jsm:272 (Diff revision 4) > updatechannel() { > return UpdateUtils.UpdateChannel; > }, > > locale() { > - let chrome = Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry); > + return Services.locale.getAppLocaleAsLangTag(); This one I'd prefer someone to check that actually understands Experiments.jsm, and the intended behavior for ja-JP-mac. ::: browser/experiments/docs/manifest.rst:197 (Diff revision 4) > locale > (optional) Array of locale identifiers this experiment should run on. > > A locale identifier is a string like ``en-US`` or ``zh-CN`` and is > obtained by looking at > - ``nsIXULChromeRegistry.getSelectedLocale("global")``. > + ``LocaleService.getAppLocaleAsLangTag()``. ... or update the docs to be more explict? ::: browser/extensions/pocket/content/main.js:573 (Diff revision 4) > callback(); > }); > } > > function getUILocale() { > - var locale = Cc["@mozilla.org/chrome/chrome-registry;1"]. > + return Services.locale.getAppLocaleAsLangTag(); I notice that we have a `ja` localization of pocket, which we probably don't pick up on macs? OMG, so many drive-by-OMGs. Probably good to just file a bug on this one? ::: browser/extensions/pocket/content/pktApi.jsm:255 (Diff revision 4) > return false; > } > > var url = baseAPIUrl + options.path; > var data = options.data || {}; > - data.locale_lang = Cc["@mozilla.org/chrome/chrome-registry;1"]. > + data.locale_lang = Services.locale.getAppLocaleAsLangTag(); ... this one should probably be part of the pocket bug, too. ::: browser/extensions/shield-recipe-client/lib/NormandyDriver.jsm:37 (Diff revision 4) > > return { > testing: false, > > get locale() { > - return Cc["@mozilla.org/chrome/chrome-registry;1"] > + return Services.locale.getAppLocaleAsLangTag(); Can you get someone that knows this code to check this one? I have no idea where and how the value is used. ::: devtools/client/shared/doorhanger.js:29 (Diff revision 4) > * for `en-US` > */ > function shouldDevEditionPromoShow() { > return Services.prefs.getBoolPref(DEV_EDITION_PROMO_ENABLED_PREF) && > !Services.prefs.getBoolPref(DEV_EDITION_PROMO_SHOWN_PREF) && > LOCALE === "en-US"; Move that call here and make it non-const? This looks like a startup bug, which might actually show the doorhanger for non-en-US locales based on timing :-) ::: devtools/shared/system.js:135 (Diff revision 4) > // while the platform version is "1.9.3pre" > platformversion: geckoVersion, > geckoversion: geckoVersion, > > // Locale used in this build > - locale: Cc["@mozilla.org/chrome/chrome-registry;1"] > + locale: Services.locale.getAppLocaleAsLangTag(), I wonder if this is even used. And if, if it should be lazy? Feel free to put this into a follow-up, though, OTH, it'd be good to reduce our exposure and/or technical debt.

> ... or update the docs to be more explict? I updated the docs. I don't change the behavior, so #primumnonnocere. > Probably good to just file a bug on this one? Good catch, will file. > Can you get someone that knows this code to check this one? I have no idea where and how the value is used. :mythmon, can you verify that you want lang tag in NormandyDriver.jsm (so, "ja-JP-mac" which is what we have l10n resources for and what we localize to) and not BCP47 form ('ja-JP-x-lvariant-mac') which is what we use for Intl date/time formatting etc? > This looks like a startup bug, which might actually show the doorhanger for non-en-US locales based on timing :-) Great catch! Thank you Patch updated. Re-requesting review and filing follow ups

Filed bug 1348353 and bug 1348355 as follow-ups as requested.

Yes, we want the "ja-JP-mac" format in NormandyDriver.jsm.

Comment on attachment 8847326 [details] Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. https://reviewboard.mozilla.org/r/120326/#review123522 Thanks, the js pieces look good now, over to jfkthame for the c++ bites.

Comment on attachment 8847326 [details] Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. https://reviewboard.mozilla.org/r/120326/#review124850 r=me for the C++ bits.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 8e44ef4465df -d a0e6a0c4712a: rebasing 383326:8e44ef4465df "Bug 1347314 - Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. r=jfkthame,Pike" (tip) merging browser/components/feeds/FeedWriter.js merging browser/extensions/pocket/content/pktApi.jsm merging devtools/server/tests/mochitest/test_device.html merging editor/composer/nsEditorSpellCheck.cpp merging toolkit/components/urlformatter/nsURLFormatter.js merging toolkit/components/urlformatter/tests/unit/test_urlformatter.js merging toolkit/content/widgets/datetimepopup.xml merging toolkit/mozapps/downloads/DownloadUtils.jsm merging toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js merging toolkit/mozapps/extensions/content/extensions.js merging uriloader/exthandler/nsHandlerService.js warning: conflicts while merging devtools/server/tests/mochitest/test_device.html! (edit, then use 'hg resolve --mark') warning: conflicts while merging toolkit/mozapps/downloads/DownloadUtils.jsm! (edit, then use 'hg resolve --mark') warning: conflicts while merging toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cc308a73ad05 Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. r=jfkthame,Pike

Backed out for eslint failure in test_device.html: https://hg.mozilla.org/integration/autoland/rev/48e124a57b7f296c7708bb852163844dce8c6363 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cc308a73ad05ab7b11872d8640f9711a99775094&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=85676602&repo=autoland > devtools/server/tests/mochitest/test_device.html:19:9 | 'Cc' is assigned a value but never used. (no-unused-vars)

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8601c8d5b76d Migrate calls to ChromeRegistry::GetSelectedLocale to use LocaleService::GetAppLocale. r=jfkthame,Pike

Sorry for the trouble! To my defense, the try build was green. Relanding with eslint fix.

Commit pushed to master at https://github.com/mozilla/normandy https://github.com/mozilla/normandy/commit/b14f9a6fb6e77b039b1a2f305c1ed35bdf49489b recipe-client-addon: Use getAppLocaleAsLangTag for getting browser locale This is from bug 1347314.

In the future it might be good if you seek for review from appropriate peers before landing patches like that. The changes to Firefox Puppeteer completely busted our update tests. I filed bug 1355382 for that. Thanks.