+++ This bug was initially created as a clone of Bug #1410736 +++ As per bug 1410736 comment #1 we should move away from using pref general.useragent.locale. There are a few uses in C-C: https://dxr.mozilla.org/comm-central/search?q=general.useragent.locale&redirect=true

Since bug 1410736 is in the process of landing, I'm looking at this now. The replacement for let locale = Services.prefs.getCharPref("general.useragent.locale"); is let originalReqLocales = Services.locale.getRequestedLocales(); I guess you return an array and therefore we need to use originalReqLocales[0], right? We also have C++ code prefs->GetComplexValue("general.useragent.locale", ... prefs->GetCharPref("general.useragent.locale", locale); So the replacement would be mozilla::intl::LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale); Could you please take a look at https://dxr.mozilla.org/comm-central/rev/952762964f408e33d060dd1110cac0626ae5fa51/mail/components/shell/DirectoryProvider.cpp#112-123 and advise me.

(In reply to Jorg K (GMT+2) from comment #1) > Since bug 1410736 is in the process of landing, I'm looking at this now. > > The replacement for > let locale = Services.prefs.getCharPref("general.useragent.locale"); > is > let originalReqLocales = Services.locale.getRequestedLocales(); > > I guess you return an array and therefore we need to use > originalReqLocales[0], right? That's not exactly right. What you quoted is from tests, and tests often do: ``` Services.prefs.setCharPref("general.useragent.locale", "ab-CD"); (...) Services.prefs.resetCharPref("general.useragent.locale"); ``` LocaleService does not have a good way to express "resetCharPref" because you're not operating on a char pref. We could probably do "LocaleService.getOriginalRequestedLocales" but that would be confusing and only really needed in tests. So, instead we do: ``` let originalReqLocales = Services.locale.getRequestedLocales(); Services.locale.setRequestedLocales(["ab-CD"]); (...) Services.locale.setRequestedLocales(originalReqLocales); ``` This allows the test to test some specific conditions and then reset to the conditions from before the test. If you're looking for a direct equivalent of: `let locale = Services.prefs.getCharPref("general.useragent.locale");`, then do `let locale = Services.locale.getRequestedLocale();` We offer both, singular and plural forms. Whenever possible you should operate on locale fallback lists, but for many old APIs and tests, singular version will work just fine. > > We also have C++ code > prefs->GetComplexValue("general.useragent.locale", ... > prefs->GetCharPref("general.useragent.locale", locale); > > So the replacement would be > mozilla::intl::LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale); Nope, `GetAppLocaleAsLangTag` returns a resolved locale (via language negotiation), while the original tests tests just for the requested locale (via pref). The difference is that you can freely change the requested between, say, `de`, `fr` and `en-US`, but if the only locale that is installed is `en-US` then `GetAppLocaleAsLangTag` will always return `en-US`. So, the direct equivalent of: ``` prefs->GetComplexValue("general.useragent.locale"); ``` is: ``` AutoTArray<nsCString, 10> requestedLocales; LocaleService::GetInstance()->GetRequestedLocales(requestedLocales); let loc = requestedLocales[0]; ``` (C++ API does not have the singular form, so you need to take an array and pick the first element) and an equivalent of: ``` prefs->GetCharPref("general.useragent.locale", locale); ``` is: ``` AutoTArray<nsCString, 10> requestedLocales; LocaleService::GetInstance()->GetRequestedLocales(requestedLocales); let loc = requestedLocales->Length() > 0 ? requestedLocales[0] : locale; ``` although I believe that GetRequestedLocales always returns a value, so maybe there is no need to try for the fallback one (`locale` in this case) and you can just use the same code as in the previous example. > Could you please take a look at > https://dxr.mozilla.org/comm-central/rev/ > 952762964f408e33d060dd1110cac0626ae5fa51/mail/components/shell/ > DirectoryProvider.cpp#112-123 > and advise me. This code does a lot of magic around "special" values of the pref. Fortunately, we do that magic inside GetRequestedLocales now, so you just need to do a regular: ``` nsAutoCString locale; AutoTArray<nsCString, 10> requestedLocales; LocaleService::GetInstance()->GetRequestedLocales(requestedLocales); nsAutoCString locale(requestedLocales[0]); ``` Hope that helps!

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2) > Hope that helps! Sure does. I'm dealing with a bit of bustage right now, so I'll get back to this mid-week with f?zibi ;-)

no rush. I'll start moving us away from `general.useragent.locale` only after 59 opens.

This should do it. I'm not sure what Services.locale.setRequestedLocales(["foo"]); will do. But I'm sure Zibi knows ;-)

Comment on attachment 8925387 [details] [diff] [review] 1410738-general.useragent.locale.patch (v1) Review of attachment 8925387 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: mail/base/test/unit/test_mailGlue_distribution.js @@ +121,5 @@ > do_check_eq(value, Services.prefs.getCharPref(key)); > } > } > + > + Services.locale.setRequestedLocales(originalReqLocales); I don't think you need to clean up in an xpcshell test. ::: mail/test/mozmill/newmailaccount/test-newmailaccount.js @@ +1126,5 @@ > function test_search_button_disabled_if_no_lang_support() { > // Set the user's supported language to something ridiculous (caching the > // old one so we can put it back later). > + let originalReqLocales = Services.locale.getRequestedLocales(); > + Services.locale.setRequestedLocales(["foo"]); // XXX: Will that work?? The test intends to set a bogus locale. Do you ask Zibi if setRequestedLocales won't reject such a locale? @@ +1165,5 @@ > * is not set to "*". > */ > function test_provider_language_wildcard() { > + let originalReqLocales = Services.locale.getRequestedLocales(); > + Services.locale.setRequestedLocales(["foo"]); // XXX: Will that work?? The test had "foo-bar" as locale value, why do you change it to "foo" only?

(In reply to :aceman from comment #6) > I don't think you need to clean up in an xpcshell test. OK. > The test intends to set a bogus locale. Do you ask Zibi if > setRequestedLocales won't reject such a locale? Yes, also in comment #5. > The test had "foo-bar" as locale value, why do you change it to "foo" only? Makes, no difference, does it?

Comment on attachment 8925387 [details] [diff] [review] 1410738-general.useragent.locale.patch (v1) Review of attachment 8925387 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/modules/distribution.js @@ -138,5 @@ > XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_locale", > function TBDistCustomizer_get__locale() { > let locale; > try { > - locale = Services.prefs.getCharPref("general.useragent.locale"); You don't need the try/catch anymore. We fallback on en-US automatically and the method is infallible. ::: mail/base/test/unit/test_mailGlue_distribution.js @@ +121,5 @@ > do_check_eq(value, Services.prefs.getCharPref(key)); > } > } > + > + Services.locale.setRequestedLocales(originalReqLocales); you definitely do need to clean up in an xpcshell test :) We're removing `general.useragent.locale`. ::: mail/test/mozmill/newmailaccount/test-newmailaccount.js @@ +1126,5 @@ > function test_search_button_disabled_if_no_lang_support() { > // Set the user's supported language to something ridiculous (caching the > // old one so we can put it back later). > + let originalReqLocales = Services.locale.getRequestedLocales(); > + Services.locale.setRequestedLocales(["foo"]); // XXX: Will that work?? "foo" is not a bogus locale from the BCP47 perspective. We may not have data for it (most likely we don't ;)), but it's an ok language tag. Depending on what you want to test, you may keep using it, or the test may not make sense anymore, since we do canonicalize entry values, so you will not be able to set an incorrect value here. @@ +1165,5 @@ > * is not set to "*". > */ > function test_provider_language_wildcard() { > + let originalReqLocales = Services.locale.getRequestedLocales(); > + Services.locale.setRequestedLocales(["foo"]); // XXX: Will that work?? "foo-bar" is also a correct language tag according to BCP47. :)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8) > ::: mail/base/test/unit/test_mailGlue_distribution.js > @@ +121,5 @@ > > do_check_eq(value, Services.prefs.getCharPref(key)); > > } > > } > > + > > + Services.locale.setRequestedLocales(originalReqLocales); > > you definitely do need to clean up in an xpcshell test :) We're removing > `general.useragent.locale`. I mean you do not need to set the value back at the end of the test. No other test does reset the values it has changed. I think all tests start with a clean initial state (that is why they can run in parallel). Which is in contrast to mozmill, where tests run one after another in the same session (one TB startup) so they need to cleanup after themselves.

Addressed comments: 1) Removed Xpcshell clean-up 2) Removed try/catch 3) Restored foo-bar. I also ran mozmake SOLO_TEST=newmailaccount/test-newmailaccount.js mozmill-one and it passes.

Comment on attachment 8925434 [details] [diff] [review] 1410738-general.useragent.locale.patch (v2) Review of attachment 8925434 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r+ for the /mail part. ::: mail/base/modules/distribution.js @@ +134,5 @@ > } > return ini; > }); > > XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_locale", I wonder what magic this XPCOMUtils.defineLazyGetter does and if we cold just unroll the (now gutted) "_locale" member. Any JS expert?

Comment on attachment 8925434 [details] [diff] [review] 1410738-general.useragent.locale.patch (v2) Review of attachment 8925434 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good. r+ for the calendar part @aceman: it's basically a caching mechanism - see also https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/XPCOMUtils.jsm#defineLazyGetter()

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/1c87503f677b Replace uses of pref general.useragent.locale in C-C. r=MakeMyDay,aceman