Closed Bug 1414390 Opened 7 years ago Closed 7 years ago

Introduce a pref to store BCP47 locale list

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.
59 bytes, text/x-review-board-request
jfkthame
: review+
mossop
: review+
Details 
As we're closing down on the remaining uses of general.useragent.locale, it's time to start planning its replacement.

I'd like to introduce a new pref which will:

 - store a list of locales
 - preferably as BCP47 language tags
 - probably using `intl.locale` pref namespace

The first idea is to use `intl.locale.requested`, but I'm open to discuss other ideas. One reason to look for something more specific is that if in the future we'll want to introduce multiple requested lists (say, requested locales for Firefox, separate for requested locales for DevTools) we may have to figure out how to make it more specific.

Maybe:
 - intl.locale.requested
 - intl.locale.requested.devtools

is enough?
Blocks: 1346877
Priority: -- → P3
Pike, Jonathan - do you have any preference?
Flags: needinfo?(l10n)
Flags: needinfo?(jfkthame)
Depends on: 1410736
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> Maybe:
>  - intl.locale.requested
>  - intl.locale.requested.devtools
> 
> is enough?

That seems reasonable to me. It provides a global/default value, but can easily be extended with more-specific overrides for .devtools and for any other components that might someday need to be configured separately.

Maybe should be "intl.locales.requested" (note the "s") to reinforce the fact that it is a list, not just a single locale.
Flags: needinfo?(jfkthame)
Related question first, how do you anticipate this to work with matchOS?
Flags: needinfo?(l10n) → needinfo?(gandalf)
Depends on: 1414899
Flags: needinfo?(gandalf)
Flags: needinfo?(gandalf)
> Related question first, how do you anticipate this to work with matchOS?

Not sure yet.

One option would be to do what Jonathan suggested when we designed LocaleService - empty requested means "use OS".
Alternatively, we could keep the matchOS as is, and consider adding `intl.locale.matchOS.devtools` etc.

Or reverse it and do:

intl.locale.devtools.requested
intl.locale.devtools.matchOS

I'm not sure which way to go here, just brainstorming.

> Maybe should be "intl.locales.requested" (note the "s") to reinforce the fact that it is a list, not just a single locale.

I thought we're aiming to make the namespace of preferences be "tree-like", as in "intl.*" is all Intl related, "intl.locale.*" is all intl/locale related?

If that's the case then having `intl.locales.requested` and `intl.locale.matchOS` would be confusing, no?
Flags: needinfo?(gandalf)
Do you have a suggestion?
Flags: needinfo?(l10n)
Empty being matchOS sounds good to me.
We'll also need to handle users, i.e., junk being manually entered in about:config.
Flags: needinfo?(l10n)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8930189 [details]
Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.

Here's the WIP for this patch.

Jonathan, Axel: can you skim through to verify that the code looks reasonable?

Few things noteworthy:
 - I am removing general.useragent.locale|intl.locale.matchOS - originally I thought about trying to handle both, but with landing of bug 1337078 in 58, we no longer have any place where codebase would operate via manipulating the old pref, and this is much cleaner now.

 - I removed the intl.preferences override. I looked through all values localizers set there, and it should all be correctly captured by the language negotiation and LikelySubtags.
The only exception is the `ja-JP-mac` which I provided a workaround for by turning it into `ja-JP-macos`.
It would be nice to change that in our naming structure ("ja-JP-mac" -> "ja-JP-macos") which would allow us to remove all the specialcasing, but for now I believe it'll work well.
Attachment #8930189 - Flags: feedback?(l10n)
Attachment #8930189 - Flags: feedback?(jfkthame)
Comment on attachment 8930189 [details]
Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.

https://reviewboard.mozilla.org/r/201346/#review206472


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: intl/locale/LocaleService.cpp:988
(Diff revision 2)
> -    Preferences::ClearUser(SELECTED_LOCALE_PREF);
> -  } else {
> -    Preferences::SetCString(SELECTED_LOCALE_PREF, aRequested[0]);
> +    nsAutoCString locale(aRequested[i]);
> +    SanitizeForBCP47(locale);
> +    if (locale.EqualsLiteral("und")) {
> +      NS_ERROR("Invalid language tag provided to SetRequestedLocales!");
> +      return NS_ERROR_INVALID_ARG;
> +    } else if (locale.EqualsLiteral("ja-JP-x-lvariant-mac")) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Attachment #8930189 - Flags: feedback?(l10n)
Attachment #8930189 - Flags: feedback?(jfkthame)
Comment on attachment 8930189 [details]
Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.

Seems reasonable to me. It would be nice to have some actual tests for how this works with multi-locale builds, etc., which I don't see here; but maybe we're not really set up to be able to do that at this point.
Attachment #8930189 - Flags: feedback?(jfkthame) → feedback+
> Seems reasonable to me. It would be nice to have some actual tests for how this works with multi-locale builds, etc., which I don't see here; but maybe we're not really set up to be able to do that at this point.

So, this patch doesn't directly influence our builds. It only liberates us to store multiple requested locales, rather than one. In bug 1410923 I started brainstorming ways to further extend this to build system, and even with that, we're still not talking about multilocale as in `MOZ_CHROME_MULTILOCALE="de it" ./mach package` yet, just that if you do `./mach build installers-sr-RU` we can package more than just sr-RU at build time.

So, with this patch landing all that changes is that:
a) we now will be able to work on the build side to store list of locales in `intl.locale.requested`
b) at runtime LocaleService::SetRequestedLocales can set a fallback list

The longer lists are covered in NegotiateLanguages tests and in bug 1414899 I'll do more manual testing for scenarios such as langpack "fr", build with "it" and "en-US" which is the closest we can now get to multilocale builds.

I also added a test where I do SetRequestedLocales to a longer chain and check if GetRequestedLocales returns it. Is that enough?

I'll add more tests to verify that we remove bogus language tags (since `intl.locale.requested` is sanitized).
Comment on attachment 8930189 [details]
Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.

The whole jazz about -mac vs -x-lvariant-mac vs -macos needs more comments.

ja-JP-macos is well-formed, but not valid. No idea if that's good or bad. The comments in the code should also use strict language here, "well-formed" or "valid", not compliant (or typos thereof ;-) )

Not sure if the code actually ends up using -macos at all? I don't see it in the code.

Generally speaking, the broad idea of using one pref instead of two, and removing the localized pref is good.
I wonder, could we even remove the pref for single-locale builds from browser/firefox-l10n.js and the mobile equiv?
Attachment #8930189 - Flags: feedback?(l10n) → feedback+
Yes, we can remove them and end up with three state system:
 - missing prop means "take default locale and add last fallback"
 - empty prop means "take OS locales and add last fallback"
 - filled prop means "parse the list, sanitize it, add last fallback"

Does it sound good?

I'll document the hell out of the specialcase :)

Why do you say "ja-JP-macos" is not a valid bcp47? It matches the [5-8] alphanum variant subtag just like "ja-JP-windows"
Per https://tools.ietf.org/html/bcp47#section-2.2.9:

A tag is considered "valid" if it satisfies these conditions:

   o  The tag is well-formed.

   o  Either the tag is in the list of grandfathered tags or all of its
      primary language, extended language, script, region, and variant
      subtags appear in the IANA Language Subtag Registry as of the
      particular registry date.

   o  There are no duplicate variant subtags.

   o  There are no duplicate singleton (extension) subtags.

Given that "macos" isn't in the IANA Language Subtag Registry, ja-JP-macos is not valid. (Nor is ja-JP-windows)
Thank Axel! Ok, the patch is ready and I switched to use well-formed :)

I also added a bit of code that I'd like to keep through the 59 cycle that migrates old-style prefs to the new ones.
Notice: I am not removing the mobile/android/locales/mobile-l10n.js in this patch to keep it smaller. The file just becomes empty and we can remove it in a separate bug since it touches build system more than anything else.
Comment on attachment 8930189 [details]
Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.

https://reviewboard.mozilla.org/r/201346/#review208494

::: intl/locale/LocaleService.cpp:78
(Diff revision 6)
> -  nsAutoCString locale;
> -
> -  // First, we'll try to check if the user has `matchOS` pref selected
> +  // Temporary section to transition old style requested locales preferences
> +  // to the new model.
> +  // XXX: To be removed in Firefox 60

It seems sad to have C++ code for this; can you do it in the `_migrateUI` method of BrowserGlue?

https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/browser/components/nsBrowserGlue.js#1745

::: intl/locale/LocaleService.cpp:84
(Diff revision 6)
> -    // If he has, we'll pick the locale from the system
> -    if (OSPreferences::GetInstance()->GetSystemLocales(aRetVal)) {
> -      // If we succeeded, return.
> -      return true;
> +		nsAutoCString requestedValue;
> +		if (Preferences::GetBool(MATCH_OS_LOCALE_PREF)) {
> +      requestedValue.AssignLiteral("");
> +		} else {
> +			nsAutoCString str;
> +			if (NS_SUCCEEDED(Preferences::GetCString(SELECTED_LOCALE_PREF, str))) {
> +        if (SanitizeForBCP47(str, true)) {
> +          requestedValue.Assign(str);
> +				}
> -    }
> +      }
> -  }
> +    }
>  
> -  // Otherwise, we'll try to get the requested locale from the prefs.
> -  if (!NS_SUCCEEDED(Preferences::GetCString(SELECTED_LOCALE_PREF, locale))) {
> -    return false;
> +		Preferences::ClearUser(MATCH_OS_LOCALE_PREF);
> +		Preferences::ClearUser(SELECTED_LOCALE_PREF);
> +		Preferences::SetCString(REQUESTED_LOCALES_PREF, requestedValue);
>    }

I'm seeing some crazy-looking indentation here -- please de-tab! (And tweak your editor configuration so it never inserts hard tab characters?)

::: intl/locale/LocaleService.cpp:86
(Diff revision 6)
> -  bool matchOSLocale = Preferences::GetBool(MATCH_OS_LOCALE_PREF);
> -
> -  if (matchOSLocale) {
> -    // If he has, we'll pick the locale from the system
> -    if (OSPreferences::GetInstance()->GetSystemLocales(aRetVal)) {
> -      // If we succeeded, return.
> +  if (Preferences::HasUserValue(MATCH_OS_LOCALE_PREF) ||
> +      Preferences::HasUserValue(SELECTED_LOCALE_PREF)) {
> +
> +		nsAutoCString requestedValue;
> +		if (Preferences::GetBool(MATCH_OS_LOCALE_PREF)) {
> +      requestedValue.AssignLiteral("");

No need for this assignment, `requestedValue` will start out as an empty string anyway.

::: intl/locale/LocaleService.cpp:88
(Diff revision 6)
> +			nsAutoCString str;
> +			if (NS_SUCCEEDED(Preferences::GetCString(SELECTED_LOCALE_PREF, str))) {
> +        if (SanitizeForBCP47(str, true)) {
> +          requestedValue.Assign(str);

Using a separate string here seems like excessive copying. How about just reading the pref directly into your `requestedValue` var, and then if Sanitize says it fails, truncate it back to empty?
Moved the conversion to the nsBrowserGlue and that fixed all the following issues :)
Comment on attachment 8930189 [details]
Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.

https://reviewboard.mozilla.org/r/201346/#review210240

LGTM, though maybe you should also have a more front-end-ish person sign off on the nsBrowserGlue version of the migration code, as I don't really speak that language. :\
Attachment #8930189 - Flags: review?(jfkthame) → review+
Comment on attachment 8930189 [details]
Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.

Dave, can you review the migration code pls? (I tested it manually as well)
Attachment #8930189 - Flags: review?(dtownsend)
Comment on attachment 8930189 [details]
Bug 1414390 - Add intl.locale.requested locale list to replace general.useragent.locale.

https://reviewboard.mozilla.org/r/201346/#review211110

::: browser/components/nsBrowserGlue.js:2230
(Diff revision 7)
> +            } catch (e) { /* Don't panic if the value is not a valid locale code. */ }
> +          }
> +        }
> +        Services.prefs.clearUserPref(SELECTED_LOCALE_PREF);
> +        Services.prefs.clearUserPref(MATCHOS_LOCALE_PREF);
> +      }

Should probably notify the Thunderbird/Seamonkey folks that they need to implement something like this too.
Attachment #8930189 - Flags: review?(dtownsend) → review+
Thanks Dave!

Jorg - I'm testing the platform with the patch, but would like to land it this week to give it a proper time to bake before merge day. Let me know if you want me to delay the landing to get the migration code landed into Thunderbird/SM.
Flags: needinfo?(jorgk)
Merge day is in January. So what do we need to add, the migration code? That doesn't appear too hard.
Flags: needinfo?(jorgk)
yeah, just the migration code.

Ok, then I'll land it as soon as I'm done with testing scenarios and you can land the migration code right after that. Sounds good?
Yep. But nothing severe happens without the migration code, right?
No, just that users who're using langpack get reset to their packaged locale and need to select the langpack locale again (using the new `intl.locale.requested` pref).
Since langpacks are not usable really on Nightly, it shouldn't affect the nightly population.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d88b85d93be
Add intl.locale.requested locale list to replace general.useragent.locale. r=jfkthame,mossop
https://hg.mozilla.org/mozilla-central/rev/8d88b85d93be
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Hi folks,
thanks for making this happen.
One tought about merging this into TB + SM. Could you please fix the "intl.locale.matchOS" stuff on them too?
Thank you.
Nick
Blocks: 1423703
Depends on: 1426943
Depends on: 1432781
Since it seems there's no migration from profiles with general.useragent.locale set (i've had users losing their ui language on 59.0 and i experienced it in 60.0b3), shouldnt this have been mentioned in the relnotes ? Or something special has to be done for distributors with a distribution.ini file where langpack xpi are installed systemwide separately, like setting intl.locale.requested="" ?
(In reply to Landry Breuil (:gaston) from comment #36)
> Since it seems there's no migration from profiles with
> general.useragent.locale set (i've had users losing their ui language on
> 59.0 and i experienced it in 60.0b3), shouldnt this have been mentioned in
> the relnotes ? 

I'm starting to think this would be a good idea, even if it's quite late

SUMO article has been updated in the meantime
https://support.mozilla.org/en-US/kb/use-firefox-interface-other-languages-language-pack#w_how-to-change-the-language-of-the-user-interface
I've fixed general.useragent.locale in distribution.ini for Firefox 60 - https://bugzilla.mozilla.org/show_bug.cgi?id=1450270.

Should I handle intl.locale.matchOS as well?
Is this part of distribution.ini? I don't know what's the scope there. We do the same migration for that as we do for `general.useragent.lcoale` - https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2111
Yes, ther are Linux (and other) distros that set intl.locale.matchOS in distribution.ini. So the browser glue code wouldn't migrate it.
Ugh. I would love if any distribution style code used LocaleService API rather than setting prefs around, since that seems to be inherently flawed.

Sure, I didn't come across any bug reports about it from Linux distros, so maybe they did beta-test? Idk.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: