Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Description • 7 years ago

      No description provided.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 2 • 7 years ago

Comment on attachment 8847940 [details]
Bug 1347306 - Hand over language negotiation from ChromeRegistry to LocaleService.

This is the first of the more complex patches. What it does is:

1) Remove the UILocale command line option*
2) Migrate the language negotiation to LocaleService
3) Migrate the control over reacting to update to requested locales to LocaleService
4) Hook ChromeRegistry to react to intl:app-locales-changed from LocaleService
5) Hook ChromeRegistry registering locale packages to fire LocaleService::Refresh
6) Remove `selected-locale-has-changed` event that noone was listening to
7) Remove retrieving SelectedLocale in Init because nobody asks for it until after first packages are loaded (I'll have to init it to default locale maybe?)
8) Separate out language negotiation in Chrome and Content processes

It does not yet:
1) Remove the negotiation step from ChromeRegistry.

--------------

This is how far I feel I can go now and it allows me to plug L10nRegistry as an alternative registrar.

There are two challenges to consider in follow up bugs:

1) Initiation order is wrong (bug 1344355). Basically, when ChromeRegistry is initialized, the prefs are not yet, so we can't get the user selected locale. We can only get the "default" value for the pref. Then, JS Context is created after we loaded the bundled package ('en-US' in my case) but before we loaded langpack package ('pl').
That means that the only locale I can return here is 'en-US', despite that the locale should be 'pl'.

The patch makes it so that ChromeRegistry and LocaleService react to those initialization events as they come, so once the pref for the locale is initialized, LocaleService reacts, and once the 'pl' package is registered, ChromeRegistry reacts.
All that happen before any XUL window is created, which is why it worked so far and it continues working, but it'd be nice to fix it. I'll take it to bug 1344355

2) Content process has no access to ChromeRegistry availableLocales and JS context asks for a locale to early for prefs to be ready.

That's an interesting challenge which may also require a separate bug. For now, I just return hardcoded "defaultLocale" - 'en-US' - in content process.
The way it worked before was that JS Context was retrieving OS locale, so it didn't care.

Overall I think the patch is in a good shape for feedback and is pretty much complete barring test updates and docs.

*) I posted my intention about it a week ago to .dev.platform with no response https://groups.google.com/d/msg/mozilla.dev.platform/_-Hb__StGrQ/SYkO-IP1AwAJ

	Jonathan Kew [:jfkthame]
	Comment 3 • 7 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> 6) Remove `selected-locale-has-changed` event that noone was listening to

Are we sure? Could there be add-ons that listen to it and will be disrupted by its removal?

	Jonathan Kew [:jfkthame]
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8847940 [details]
Bug 1347306 - Hand over language negotiation from ChromeRegistry to LocaleService.

https://reviewboard.mozilla.org/r/120868/#review125458

Generally this looks reasonable, modulo a few questions (and probably needs a rebase by now).

::: chrome/nsChromeRegistry.h:94
(Diff revision 1)
>                                          const nsCString& aProvider,
>                                          const nsCString& aPath) = 0;
>    virtual nsresult GetFlagsFromPackage(const nsCString& aPackage,
>                                         uint32_t* aFlags) = 0;
>  
> -  nsresult SelectLocaleFromPref(nsIPrefBranch* prefs);
> +  nsresult UpdateSelectedLocale();

Does this need to be here, or is it nsChromeRegistryChrome-only? Currently AFAICS there's no impl of it in nsChromeRegistryContent, and none of the "common" code in nsChromeRegistry calls it.

::: chrome/nsChromeRegistryChrome.cpp:139
(Diff revision 1)
>      nsXPIDLCString provider;
>      rv = prefs->GetCharPref(SELECTED_SKIN_PREF, getter_Copies(provider));
>      if (NS_SUCCEEDED(rv))
>        mSelectedSkin = provider;
>  
> -    SelectLocaleFromPref(prefs);
> +    //UpdateSelectedLocale();

Hmm, so what should be happening here...?

::: chrome/nsChromeRegistryChrome.cpp:292
(Diff revision 1)
>      mProfileLoaded = true;
>    }
> +  else if (!strcmp("intl:app-locales-changed", aTopic)) {
> +    UpdateSelectedLocale();
> +    if (mProfileLoaded)
> +      FlushAllCaches();

braces please (gecko style)

::: intl/locale/LocaleService.cpp:85
(Diff revision 1)
> -static void
> -ReadAppLocales(nsTArray<nsCString>& aRetVal)
> +void
> +LocaleService::NegotiateAppLocales(nsTArray<nsCString>& aRetVal)
>  {
> -  nsAutoCString uaLangTag;
> -  nsCOMPtr<nsIToolkitChromeRegistry> cr =
> -    mozilla::services::GetToolkitChromeRegistryService();
> +  if (XRE_IsParentProcess()) {
> +    AutoTArray<nsCString, 100> availableLocales;
> +    AutoTArray<nsCString, 100> requestedLocales;

Make `requestedLocales` a smaller autoarray, to reduce stack frame size; I'd suggest something like 10. (Currently, IIUC we only return 1 entry, but even once we allow the user to specify several locales, the number isn't likely to be very large.)

::: intl/locale/LocaleService.cpp:86
(Diff revision 1)
> -ReadAppLocales(nsTArray<nsCString>& aRetVal)
> +LocaleService::NegotiateAppLocales(nsTArray<nsCString>& aRetVal)
>  {
> -  nsAutoCString uaLangTag;
> -  nsCOMPtr<nsIToolkitChromeRegistry> cr =
> -    mozilla::services::GetToolkitChromeRegistryService();
> -  if (cr) {
> +  if (XRE_IsParentProcess()) {
> +    AutoTArray<nsCString, 100> availableLocales;
> +    AutoTArray<nsCString, 100> requestedLocales;
> +    nsAutoCString defaultLocale(NS_LITERAL_CSTRING("en-US"));

Let's use `GetDefaultLocale` to get this string rather than repeating the hard-coded constant here.

::: intl/locale/LocaleService.cpp:87
(Diff revision 1)
> -    // We don't want to canonicalize the locale from ChromeRegistry into
> -    // it's BCP47 form because we will use it for direct language
> +    LocaleService::LangNegStrategy strategy =
> +      LocaleService::LangNegStrategy::Filtering;

No need to declare a `strategy` variable here, just put `LangNegStrategy::Filtering` directly in the `NegotiateLanguages` call. (It shouldn't need the `LocaleService::` prefix, as we're within a LocaleService method.)

::: intl/locale/LocaleService.cpp:94
(Diff revision 1)
> -  if (!uaLangTag.IsEmpty()) {
> -    aRetVal.AppendElement(uaLangTag);
> -  }
>  
> -  if (!uaLangTag.EqualsLiteral("en-US")) {
> +    NegotiateLanguages(requestedLocales, availableLocales, defaultLocale,
> +      strategy, aRetVal);

indent to match the start of the arguments above

::: intl/locale/LocaleService.cpp:96
(Diff revision 1)
> -  }
>  
> -  if (!uaLangTag.EqualsLiteral("en-US")) {
> +    NegotiateLanguages(requestedLocales, availableLocales, defaultLocale,
> +      strategy, aRetVal);
> +  } else {
>      aRetVal.AppendElement(NS_LITERAL_CSTRING("en-US"));

Here also, let's use `GetDefaultLocale`, so that the hardcoded "en-US" is isolated to a single place. (Ideally, at least... not sure if we've got them all yet?)

::: intl/locale/LocaleService.cpp:206
(Diff revision 1)
> +  if (mAppLocales.IsEmpty()) {
> +    return;
> +  }

IIUC, this makes sense because `mAppLocales` will only be empty if nobody has requested the app locales yet, not because there was a valid empty list that might be changing to non-empty. Please add a comment to say something like

    // if mAppLocales has not been initialized yet, just return

to remind us what's happening here.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 6 • 7 years ago

Thanks! I applied your feedback and thought more about what exactly `mSelectedLocale` is in ChromeRegistry.

I ended up removing it because it basically was representing a wrong concept. It carried not the negotiated locale (between requested and available), but requested only and then it was negotiated in GetSelectedLocale.

On top of that, it was misused because it was sent to child processes without negotiation etc.

With those changes, we now have:
 - full negotiation and cache invalidation in LocaleService
 - ChromeRegistry triggers LocaleService::Refresh when available languages changes
 - ChromeRegistry clears its cache on intl::app-locales-changed
 - ChromeRegistry::GetSelectedLocale is only useful for non-global packages and should be deprecated
 - I brought back `selected-locale-has-changed` for compatibility reasons. I'd like to deprecate it soon

I think it's review ready.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 9 • 7 years ago

There's a crash in try build at
[task 2017-03-23T23:31:02.185068Z] 23:31:02     INFO -  Assertion failure: IsInitialized(), at /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5092

(...)

[task 2017-03-23T23:31:02.189497Z] 23:31:02     INFO -  mozpack.errors.ErrorMessage: Error: Error while running startup cache precompilation

but i cant reproduce it by just building and launching the app. not sure how to get the startup cache precompilation.

	Jonathan Kew [:jfkthame]
	Comment 10 • 7 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> There's a crash in try build at
> [task 2017-03-23T23:31:02.185068Z] 23:31:02     INFO -  Assertion failure:
> IsInitialized(), at
> /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5092
> 
> (...)
> 
> [task 2017-03-23T23:31:02.189497Z] 23:31:02     INFO - 
> mozpack.errors.ErrorMessage: Error: Error while running startup cache
> precompilation
> 
> but i cant reproduce it by just building and launching the app. not sure how
> to get the startup cache precompilation.

If you do a DEBUG build, and then run ./mach package, you should be able to reproduce the assertion.

It looks like we'd need to make sure nsContentUtils is initialized prior to any call to Preferences::GetLocalizedCString (which occurs when LocaleService::GetRequestedLocales is used while trying to negotiate app locales); as things stand, we end up trying to do this before nsContentUtils has been initialized, and that breaks.

However, I'm not sure we can actually call nsContentUtils::Init() that early in startup; I expect there are a bunch of interdependencies that determine when it can/should be called, and just moving it arbitrarily early during startup isn't an option. Someone like :smaug may have an idea what is or isn't possible here.

Alternatively, can you avoid the need for Preferences::GetLocalizedCString there? Regular Preferences::GetCString seems to be OK to call without the ContentUtils dependency.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 12 • 7 years ago

Apparently we have some things like:
 - https://dxr.mozilla.org/mozilla-beta/source/addon-sdk/source/lib/sdk/l10n/locale.js#46
 - http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4351

which say that we need it here :(

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 13 • 7 years ago

Maybe I can just put it in an `if(nsIContent.IsInitialized())` and skip the LocalizedCPref if it's not and cross my fingers and hope for the best.

I don't fully understand it, but it seems that during regular startup we don't ask for the pref before nsICOntent is ready, just during the prepackaging, so maybe it doesn't matter?

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 14 • 7 years ago

Ok, :pike says that since those are the only places in the code which check for it, and he wants it to go away, we can skip it for crash reporter and check with the owner of the addonsdk about cutting it in the bug about moving addonsdk locale.js to use getRequestedLocales.

So here I will remove the getlocalizedcstring and we'll continue in that bug.

	Axel Hecht [:Pike]
	Comment 15 • 7 years ago

Crashreporter should use the locale service, and avoid the prefs completely if it can.

locale.js has a ton of code that does things that might be OK or not (like getting the OS locale for matchOS and then getting the localized pref, which should be kinda the same thing). Given the addon-sdk is deprecated, I think we should get permission to limit the impact that code has on our current work, and maybe just get the UI locale, the OS locale, and en-US?

	Jonathan Kew [:jfkthame]
	Comment 16 • 7 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #13)
> Maybe I can just put it in an `if(nsIContent.IsInitialized())` and skip the
> LocalizedCPref if it's not and cross my fingers and hope for the best.

(s/nsIContent/nsContentUtils/, I think)

Well, that would avoid the assertion, but offhand I don't know what the implications would be...

> 
> I don't fully understand it, but it seems that during regular startup we
> don't ask for the pref before nsICOntent is ready, just during the
> prepackaging, so maybe it doesn't matter?

The try run looks like it's failing all kinds of tests (not just the build/package job) on OS X Debug because of that assertion. I don't know why it's different there from other platforms.

Probably should check with someone who knows about the packaging step to find out what that involves and whether it cares about this.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 18 • 7 years ago

Ok, I removed it from this patch, and asked :mossop in bug 1346616 comment 14 if it's ok to regress when we land there.

If he'll say "no", we'll just not move that file to use LocaleService and leave it as is.

At least it should not block us here anymore. Started new try build and waiting for review :)

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 19 • 7 years ago

Oh, and I reverted the change to ChromeRegistryChrome::GetProvider - there's a limited amount of will power I have to rework ChromeRegistry and this is a bit more complex than I originally thought because of some optimizations between overlays, skins and locales.

I wanted to rewire it to use LanguageNegotiation, but left it as is.

The bulk of the value from this move stays - for "global" package we're gaining full LanguageNegotiation so for example if we have "fr-FR" langpack and user asks for "fr", we'll match.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 21 • 7 years ago

Here's the performance comparison for startup (tpaint ts_paint): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=75ed2e765fab6c13d9f28495ddf38bec62125038&newProject=try&newRevision=cd1a5206a675d90616ad529014a876d6d27349e6&framework=1&showOnlyImportant=0

Nothing major, but it seems like a small perf win (around 10ms); and that's before we started caching in LocaleService more aggressively (bug 1345527).

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 22 • 7 years ago

:m_kato showed me how to search through addons - https://dxr.mozilla.org/addons/search?q=selected-locale-has-changed&redirect=false - seems like noone is using `selected-locale-has-changed` :)

Up to you if you want me to keep it there in case of some dark matter.

	Jonathan Kew [:jfkthame]
	Comment 23 • 7 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #22)
> :m_kato showed me how to search through addons -
> https://dxr.mozilla.org/addons/search?q=selected-locale-has-
> changed&redirect=false - seems like noone is using
> `selected-locale-has-changed` :)

Cool. :) On further thought, what I'd suggest is that for the purpose of this bug, we keep it here; but feel free to file a separate bug about removing it, perhaps after posting to dev.platform about the intention. That way the decision will be appropriately recorded, and the removal can be tracked in its own right (and potentially reverted if something unexpected comes up).

	Jonathan Kew [:jfkthame]
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8847940 [details]
Bug 1347306 - Hand over language negotiation from ChromeRegistry to LocaleService.

https://reviewboard.mozilla.org/r/120868/#review126452

This basically seems fine, just a couple of questions.

::: chrome/nsChromeRegistryChrome.cpp:244
(Diff revision 7)
> -  aLocale = entry->locales.GetSelected(mSelectedLocale, nsProviderArray::LOCALE);
> +  aLocale = entry->locales.GetSelected(reqLocale, nsProviderArray::LOCALE);
>    if (aLocale.IsEmpty())
>      return NS_ERROR_FAILURE;
>  
>    if (aAsBCP47) {
> -    SanitizeForBCP47(aLocale);
> +    SanitizeForBCP47(reqLocale);

This can't be right -- `reqLocale` is a temporary variable that's about to go away. Presumably this should remain `aLocale`, the thing you're about to return.

::: intl/locale/LocaleService.cpp:95
(Diff revision 7)
> +    //XXX: We need to get appLocales from parent process
> +    //     See bug 1348042.
> +    aRetVal.AppendElement(defaultLocale);

Are we OK with landing this patch by itself in this state (leaving things such that the content process always thinks the app locale is en-US), or should we wait until bug 1348042 is also ready?

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 26 • 7 years ago

> This can't be right

It was not... Thanks for catching it!

> Are we OK with landing this patch by itself in this state (leaving things such that the content process always thinks the app locale is en-US), or should we wait until bug 1348042 is also ready?

Good question. Fortunately, we don't have to go for this duality. I updated the patch to just do the old behavior (taking locale from ChromeRegistry) in the Content process (which works because we didn't touch ChromeRegistryContent!).

Hope that's enough to get this landed without waiting :)

re-requesting review.

	Jonathan Kew [:jfkthame]
	Comment 27 • 7 years ago
mozreview-review

Comment on attachment 8847940 [details]
Bug 1347306 - Hand over language negotiation from ChromeRegistry to LocaleService.

https://reviewboard.mozilla.org/r/120868/#review126708

OK, sounds good.

	Pulsebot
	Comment 28 • 7 years ago

Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/850cf5d6d37d
Hand over language negotiation from ChromeRegistry to LocaleService. r=jfkthame

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/850cf5d6d37d

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 30 • 6 years ago

Is it still possible to launch a profile with a selected locale? Use case here: Have one profile which gets cloned and the locale gets to what is needed. Thank you in advance.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 31 • 6 years ago

> Is it still possible to launch a profile with a selected locale? Use case here: Have one profile which gets cloned and the locale gets to what is needed. Thank you in advance.

Sure, if you have Firefox with multiple locales (say, Firefox has ab-CD packaged and ef-GH langpack installed), then all you need is to change the `intl.locale.requested` between two profiles.

For example, in one profile use `intl.locale-requested = "ab-CD,ef-GH"` and in the other use `intl.locale-requested = "ef-GH,ab-CD"`. Does it help? If not, you may want to file a new bug with a particular scenario you'd like to get to.