Closed Bug 1346616 Opened 7 years ago Closed 7 years ago

Migrate JS callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales

Categories

(Core :: Internationalization, enhancement)

Product:
Core
Component:
Internationalization
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1346616 - Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales.
59 bytes, text/x-review-board-request
Pike
: review+
jfkthame
: review+
Details 
Part of the bug 1334772 is to migrate all callsites.

In this bug I'll deal with JS callsites.
Drive-by question: What's the deal with preference intl.locale.matchOS and general.useragent.locale. Looks like the patch is pretty much removing them, right?
Currently, I'm unifying the callsites so that instead of each one of them coming up with its own implementation of "getRequestedLocales", they all use the LocaleService's one.

In the future I'd like to turn from a single locale pref ('general.useragent.locale') to a fallback chain of locales (see bug 1325870 and the UI mock there).
But that's pretty far away from now, and there'll be a long grace period and even once I switch, the code will handle old `general.useragent.locale` as a fallback.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
in order to narrow down the scope of this patch, I left most of the tests unaffected. Most of them will be switched to `setRequestedLocales` in a separate patch.
Also, similarly to my previous refactors. I'm trying to keep the code intact wherever I feel like I can't just trivially fix it.

For example, I believe that the transition from getRequestedLocales to getAppLocales should not be in the scope of this bug, since it requires quite drastic changes to the tests.

I'd like to keep this bug focused on eliminating `general.useragent.locale` and `matchOS` pref calls from the codebase since the patch is already getting big :)
Comment on attachment 8846384 [details]
Bug 1346616 - Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales.

https://reviewboard.mozilla.org/r/119438/#review123826

There seem to be a lot of call-sites that do

```
locales = GetRequestedLocales();
return locales.length > 1 ? locales[0] : en-US-or-null
```

Should we just have a dedicated API for that, instead of having that code all over the place?

::: browser/components/extensions/test/browser/head_pageAction.js:8
(Diff revision 4)
>  "use strict";
>  
>  /* exported runTests */
>  /* globals getListStyleImage */
>  
> +const { Services } = Cu.import('resource://gre/modules/Services.jsm', {});

Do we need this import?

::: browser/components/search/test/head.js:5
(Diff revision 4)
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  Cu.import("resource://gre/modules/Promise.jsm");
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});

As this file uses Services already, I don't think we need to import it?

::: browser/modules/DirectoryLinksProvider.jsm
(Diff revision 4)
>    get _observedPrefs() {
>      return Object.freeze({
>        enhanced: PREF_NEWTAB_ENHANCED,
>        linksURL: PREF_DIRECTORY_SOURCE,
> -      matchOSLocale: PREF_MATCH_OS_LOCALE,
> -      prefSelectedLocale: PREF_SELECTED_LOCALE,

Should this file observe something new instead of the locale prefs now?

::: browser/modules/test/unit/test_DirectoryLinksProvider.js
(Diff revision 4)
>  
>    // Teardown.
>    do_register_cleanup(function() {
>      server.stop(function() { });
>      DirectoryLinksProvider.reset();
> -    Services.prefs.clearUserPref(kLocalePref);

In another test, you explicitly set the requestedLocales back to the original value.

Should you do that here, too?

::: dom/encoding/FallbackEncoding.cpp
(Diff revision 4)
>    Preferences::RegisterCallback(FallbackEncoding::PrefChanged,
>                                  "intl.charset.fallback.override",
>                                  nullptr);
> -  Preferences::RegisterCallback(FallbackEncoding::PrefChanged,
> -                                "general.useragent.locale",
> -                                nullptr);

Same comment as on a previous piece of code, should this observe something else now?

::: toolkit/components/downloads/ApplicationReputation.cpp:1323
(Diff revision 4)
>    // We have no way of knowing whether or not a user initiated the
>    // download. Set it to true to lessen the chance of false positives.
>    mRequest.set_user_initiated(true);
>  
>    nsCString locale;
> -  NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_GENERAL_LOCALE, &locale),
> +  LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale);

Do we need to do something with the nsresult here?

I know that it's always NS_OK, but maybe an unused or so? I'd want a c++ helper on that question.
Attachment #8846384 - Flags: review?(l10n) → review-
Thanks! Applied all the feedback and rerequesting review.
Comment on attachment 8846384 [details]
Bug 1346616 - Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales.

https://reviewboard.mozilla.org/r/119438/#review124108

I just have nits at this point, but I strongly rely on jfkthame to review the C++ bits. In particular the fallback encoding stuff I didn't really look in to.

So this is an r=me with nits and whatever-jfkthame-says.

::: toolkit/components/downloads/ApplicationReputation.cpp:1323
(Diff revisions 4 - 6)
>    // We have no way of knowing whether or not a user initiated the
>    // download. Set it to true to lessen the chance of false positives.
>    mRequest.set_user_initiated(true);
>  
>    nsCString locale;
> -  LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale);
> +  rv = LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale);

<froydnj>	Pike: probably should NS_ENSURE_SUCCESS(rv, rv) just for paranoia's sake

::: intl/locale/tests/unit/test_localeService.js:117
(Diff revision 6)
> +add_test(function test_getRequestedLocale() {
> +  Services.prefs.setBoolPref(PREF_MATCH_OS_LOCALE, false);
> +  Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "ar-IR");
> +
> +  let requestedLocale = localeService.getRequestedLocale();
> +  do_check_true(requestedLocale === "ar-IR", "requestedLocale returns the right value");

What's happening to this test if someone runs this with their OS set to ar-IR?

::: intl/locale/tests/unit/test_localeService.js:124
(Diff revision 6)
> +  Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "");
> +
> +  requestedLocale = localeService.getRequestedLocale();
> +  do_check_true(requestedLocale === "", "requestedLocale returns empty value value");
> +
> +  Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "en-US");

Should this reset the prefs via clearUserPref?
Attachment #8846384 - Flags: review?(l10n) → review+
:mossop, you've been pointed out by :pike as a person who can make the decision here.

Basically, we have north of 20 places in the code where we retrieve user requested locale. It always goes more or less through matchOS first, and then falls back on retrieving `general.useragent.locale`.
Of course each one of those 20 places did a slightly different variation of the logic.

I added LocaleService::GetRequestLocales and in this bug want to centralize the callsites, which will enable us to cache the result and in the future move from a single request locale to a fallback chain list of locales that the user requests.
While building the algorithm I tried to collect all the different use cases and put them together in the most sensible way.

There are two places in our code, where instead of calling for the pref `general.useragent.locale`, we're doing some sort of:

```
  // In some cases, mainly on Fennec and on Linux version,
  // `general.useragent.locale` is a special 'localized' value, like:
  // "chrome://global/locale/intl.properties"
  if (!NS_SUCCEEDED(Preferences::GetLocalizedCString(SELECTED_LOCALE_PREF, &locale))) {
    // If not, we can attempt to retrieve it as a simple string value.
    if (!NS_SUCCEEDED(Preferences::GetCString(SELECTED_LOCALE_PREF, &locale))) {
      return false;
    }
  }
```

Unfortunately, when I tried to rewire our locale selection to hand it over from ChromeRegistry to LocaleService I encountered a bug that was caused by GetLocalizedCString requiring nsIConentUtils to be initialized and the GetRequestedLocales being called to early.
That happened not in runtime, but in packaging step.

Now, I can try to workaround it, but after consulting with :pike and :jfkthame, we would like to consider dropping the `GetLocalizedCString` part, and doing just `GetCString` on `general.useragent.locale`.

The two places where this would change the logic are:
 - http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4351
 - http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/l10n/locale.js#45

We believe it's ok to not take the localizedpref in CrashReporter, but would like to get your go-ahead to remove the behavior in AddonSDK.
Is that ok with you?
Flags: needinfo?(dtownsend)
Kris, based on bug 1350646, maybe you can help me wrt. comment 14?
Flags: needinfo?(kmaglione+bmo)
I'd rather we migrate that module to use getLocale() from Locale.jsm, and not worry about precisely what the previous logic was doing. There's no reason that code needs to behave differently from the rest of the locale code in the tree.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(dtownsend)
Excellent :)

Locale.jsm is also going away, as all is getting centralized in mozILocaleService, so I'll just migrate the module to use it here.

Thanks!
Updated the patch to :pike's feedback and according to :kmag recommendation.
Also, the try build failure is fixed in bug 1347306 so setting it as a dependency.
Depends on: 1347306
Attachment #8846384 - Flags: review?(jfkthame)
Pike, assuming the try run is green, are you ok with me landing this or should I get more reviews?
Flags: needinfo?(l10n)
Ok, I'll need a .cpp review on this once I fix the try :)
Flags: needinfo?(l10n)
Comment on attachment 8846384 [details]
Bug 1346616 - Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales.

:jfkthame - can I get your r? on the C++ bits here? I'd like to land it soon after the merge day next week to maximize the time for any regressions.

This is the last use of nsLocaleService in Firefox code!
It also paves the way to migrate from general.useragent.locale to general.useragent.locales ;)
Attachment #8846384 - Flags: review?(jfkthame)
Comment on attachment 8846384 [details]
Bug 1346616 - Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales.

https://reviewboard.mozilla.org/r/119438/#review125518

::: browser/components/dirprovider/DirectoryProvider.cpp:131
(Diff revision 9)
>          }
>        }
>      }
>  
>      // we didn't have a defaultLocale, use the user agent locale
>      nsCString locale;

While you're here, please make this nsAutoCString.

::: browser/components/search/test/head.js:24
(Diff revision 9)
>      }
>    }
>  }
>  
>  function getLocale() {
> -  const localePref = "general.useragent.locale";
> +  return Services.locale.getRequestedLocale();

Doesn't this give a different result (empty string vs `undefined`?) if the pref is missing?

::: intl/locale/mozILocaleService.idl:149
(Diff revision 9)
>                             [retval, array, size_is(aCount)] out string aLocales);
>  
>    /**
> +   * Returns the top-requested locale from the user, or null if none is set.
> +   */
> +  ACString getRequestedLocale();

s/null/empty string/, right?

I wonder, though, if this is a good thing. Might it be better to explicitly return `undefined` if there is no locale set? That will stay closer to existing behavior for the callsites (I've noted a couple of places where I think it could have an effect).

::: mobile/android/chrome/content/browser.js:1650
(Diff revision 9)
>        }
>      }, 500);
>    },
>  
>    getUALocalePref: function () {
> -    try {
> +    return Services.locale.getRequestedLocale();

This will change behavior here for the case where the locale is missing; previously, we returned `undefined`, but this API will return an empty string  instead. Might that matter to callers?
Comment on attachment 8846384 [details]
Bug 1346616 - Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales.

https://reviewboard.mozilla.org/r/119438/#review125518

> s/null/empty string/, right?
> 
> I wonder, though, if this is a good thing. Might it be better to explicitly return `undefined` if there is no locale set? That will stay closer to existing behavior for the callsites (I've noted a couple of places where I think it could have an effect).

I'm a bit torn on this. On one hand,  I agree that returning undefined would be more aligned with what the current code does, but we'd introduce a misalignment between what getRequestedLocale returns and getAppLocale* (and OSPreferences.systemLocale ?).

Do you think it's worth it? I also don't know how to mark an xpidl method to return string or undefined?
Oh, and since nightly doesn't get merged anywhere this time (because of Dawn), I'd like to land it as soon as I get an r+ :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #28)
> Comment on attachment 8846384 [details]
> Bug 1346616 - Migrate callsites that are retrieving requested locale from
> pref, to use LocaleService::GetRequestedLocales.
> 
> https://reviewboard.mozilla.org/r/119438/#review125518
> 
> > s/null/empty string/, right?
> > 
> > I wonder, though, if this is a good thing. Might it be better to explicitly return `undefined` if there is no locale set? That will stay closer to existing behavior for the callsites (I've noted a couple of places where I think it could have an effect).
> 
> I'm a bit torn on this. On one hand,  I agree that returning undefined would
> be more aligned with what the current code does, but we'd introduce a
> misalignment between what getRequestedLocale returns and getAppLocale* (and
> OSPreferences.systemLocale ?).

Hmmm. OK, I guess we can leave this as is, though it'd be good to test that nothing breaks too badly if the requested locale is set to an empty string. Maybe when you migrate us from a pref for a single requested locale to a list of locales, that would be a good time to include a test for the degenerate case where that list is empty.
> Maybe when you migrate us from a pref for a single requested locale to a list of locales, that would be a good time to include a test for the degenerate case where that list is empty.

Yeah, good point.
My plan is to add edge case tests in bug 1346877 right after this and bug 1348042 land (I want to unblock the Fennec team to work on DLC integration first).
Comment on attachment 8846384 [details]
Bug 1346616 - Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales.

https://reviewboard.mozilla.org/r/119438/#review133508
Attachment #8846384 - Flags: review?(jfkthame) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60d72c2dd49d
Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales. r=jfkthame,Pike
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39a903b099a6
Migrate callsites that are retrieving requested locale from pref, to use LocaleService::GetRequestedLocales. r=jfkthame,Pike
Sorry for that :aryx, my try run from 2 days ago was green - https://treeherder.mozilla.org/#/jobs?repo=try&revision=30de1bad4343 :(
Flags: needinfo?(gandalf)
https://hg.mozilla.org/mozilla-central/rev/39a903b099a6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1373061
Depends on: 1395355
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: