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

As a backbone of the proposal from bug 1325870, we'll introduce a new LocaleService API that will control language selection in Gecko.

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

Ok, I took a first stab to try to turn bug 1325870 into code, but boy I need help.

The WIP works in that it does create a new service accessible from JS, I can load it in chrome and it does expose three functions, and I can set the right preferences and depending on them I'll retrieve the locales.

There's a lot of work left, but I'd like to first get this mock to look the way we'll want it to look.
I need help with:

1) How to make it work for C++? I feel like the way I wrote it it can only be called from JS?
2) How to store a list of strings in kosher Mozilla C++?
3) How to write the code to retrieve the list of strings (requestedLocales) either from system locales or from Prefs (in GetAppLocales)
4) How to split a string returned by the prefs (that will look like this: "de,fr,en-US") into an array?
5) other bad-c++ things?

:jfkthame, can you help me clean this up? My C++ is very limited and in particular I'm lost with strings and string arrays which is exactly what this API will have to operate on. :(

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

OK, I took your patch and reworked things a bit, and filled in some more of the code... see what you think. At this point, I think it mostly does what you were aiming for, if I understood correctly; missing pieces include code to get the list of available app locales (where do we find that?), and system-specific code to get the list of preferred languages/locales from the OS (I only did a macOS version of this). Oh, and I haven't looked into the question of calling this from C++; so far, only tested it with JS in the browser console.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 4 • 8 years ago

Comment on attachment 8829161 [details] [diff] [review]
Introduce mozILocaleService

This is awesome! Thank you!

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

I took your code from the POC, and your code from bug 1308329 and created two APIs:

 - OSPreferences
 - LocaleService

Right now OSPreferences just has GetSystemLocales, but later we'll add the region preferences to the same API.

LocaleService right now only returns app locales and doesn't do any negotiation yet. I'd like to look at how much ICU can offload that part (it definitely has the getParentLocales and case-insensitive comparisons).

I also separated the C++ API from the JS API based on your code.

It's still pretty much a mock, but I think it's start shaping up properly.

My next steps:


 - Look into how to copy current behavior into this API (so that OSPreferences::GetSystemLocales matches nsLocaleService::GetSystemLocale and LocaleService::GetAppLocales matches nsChromeRegistry::GetSelectedLocale).
 - Write tests
 - Add observers to OSPreferences for system locales changing and LocaleService for app locales changing

I have some questions:

1) I see that you tend to use ns*, while during my work on mozIntl I was told that moz* is the preferred prefix now. It also helps me avoid duplication between nsILocaleService and the new mozILocaleService. Are you ok with moz* or would you prefer me to switch to ns*?

2) If I understand correctly, nsChromeRegistry::GetSelectedLocale is similar to my intended LocaleService::GetAppLocales - it does the logic of checking user preferences, following OS locales, or negotiating between available and requested locales.

I believe LocaleService is a better place for it, especially that we were asked to move away from ChromeRegistry.
I see the separation between the new L10nRegistry that we'll be using for l10n resources and the LocaleService in that the registry only knows which locales are available, loads/adds/removes/updates language resources.
The LocaleService negotiates app locale fallback chain.

Does it sound reasonable to you?

3) What is the use case of LocaleService::GetApplicationLocale?

It seems to be loading the locale from the OS, not from the user request or negotiated, so if I understand correctly if the OS has "fr" as application locale, and Gecko has language resources only in "de", the GetSelectedLocale will return "de", but GetApplicationLocale will return "fr".
That seems like potential inconsistency. I'd like to get everything in Gecko to use LocaleService::GetAppLocales fallback chain.

Does it sound good?

4) "Everything" == L10n + ICU?

Is there anything else beyond I18n (collation, date/time formatting etc.) and L10n that we should have a language for?
In my vision LocaleService::GetAppLocales returns the fallback chain whose first locale has l10n resources loaded, and ICU can just do its language negotiation and pick the closest in the fallback chain.

If that's it, do we still need `categories`?

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

I got some stub for systemlocaleschanged observer. I'm driving blind a bit since I've never worked on event driven code in C++ so I'm not sure how it's supposed to work, but I started from:

var osp = Components.classes["@mozilla.org/intl/ospreferences;1"].getService(Components.interfaces.mozIIntlOSPreferences);

osp.GetSystemLocales(); // ["gtk", "en-US"] - dummy locales to check that it's working

and now I'd like it to do:

function onSystemLocalesChanged() {
  console.log("system locales changed!");
}

osp.AddEventListener("systemlocaleschanged", onSystemLocalesChanged);
osp.RemoveEventListener("systemlocaleschanged", onSystemLocalesChanged);

and some equivalent of that from C++.

:jfkthame, do you know how it should work?

	Axel Hecht [:Pike]
	Comment 9 • 8 years ago

I have some general concerns about the path here.

I think that the problem we try to fix here comes from base-level APIs that you can use to put together the right thing, with accept-lang prefs, and localized prefs, and chrome registry, etc.

And then our dear developer friends use some of them, put them together as they see fit, and create wrong code paths.

Now, this patch does again add a bunch of APIs on which developers could implement the right thing. Or anything else.

The code paths here might be good, I didn't check that in detail. But I think they should be private to the service exposing the actual API that we expose to developers. Let's remove footguns, not build more sophisticated ones.

Practical experience with pref values coming from telemetry shows that whatever you store in prefs will be tempered with, and will be set to junk, at times. Whenever you get data from prefs, assume they've been tempered with and replaced with unuseful data. Might call for less preferences? Then you have less to validate and make consistent.

Final comment on the role of the chrome registry. For as long as much of our UI localization content is coming through the chrome registry, it defines our app locales to [chromereg.getSelectedLocale('global'), "en-US"]. You can strip the second "en-US" if the first is en-US, of course.

IMHO, we should implement it like that, and once the chrome registry isn't important anymore, we can change where and how that's stored.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 10 • 8 years ago

> Practical experience with pref values coming from telemetry shows that whatever you store in prefs will be tempered with, and will be set to junk, at times. Whenever you get data from prefs, assume they've been tempered with and replaced with unuseful data.

Good point. I do agree and my plan is to treat this data as third-party non-safe. It'll be canonicalized and validated before use.

> Might call for less preferences? Then you have less to validate and make consistent.

a) I'm not sure what would be the alternative to prefs to store that data in Gecko
b) I'm a little bit worried about closing the system. One of the nice things about prefs is that it allows more hackability.

> Final comment on the role of the chrome registry. For as long as much of our UI localization content is coming through the chrome registry, it defines our app locales to [chromereg.getSelectedLocale('global'), "en-US"]. You can strip the second "en-US" if the first is en-US, of course.
> IMHO, we should implement it like that, and once the chrome registry isn't important anymore, we can change where and how that's stored.

Totally agree. I want to get the new API that can handle all that we want out of it in the future, but match the functionality of today for now.

Once we migrate away from Chrome Registry we'll be able to start sending more complete fallback.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 11 • 8 years ago

Ok, I got some pointers from :pike and :smaug and got the nsIObserver hooked into this.

I'll separate OSPreferences API back to bug 1308329, and first land that, and then work on this.

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

POC for the future reference

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

Comment on attachment 8828241 [details]
Bug 1332207 - Introduce mozilla::intl::LocaleService.

I updated the patch to represent the minimal version of the patch.

It currently does exactly what :pike suggested in comment 9.

I see it as a stepping stone - I'd like to land it, expose to JS and then migrate all callsites in Gecko to use it.

Then, over time, we can start improving the logic inside GetAppLocales to do real language negotiation, use OSPreferences::GetSystemLocales and sync it with the upcoming L10nRegistry.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 15 • 8 years ago

I'd also want to start migrating mozIntl to use GetAppLocales as the default locale fallback for its functions.

	Axel Hecht [:Pike]
	Comment 17 • 8 years ago

As for tests, https://dxr.mozilla.org/mozilla-central/rev/fbdfcecf0c774d2221f11aed5a504d5591c774e0/chrome/test/unit/test_bug519468.js might be a good inspiration. Not about the locale stub service, but registring the fake chrome urls, and then set the pref. That way, you should be able to test ['en-US'] as well as ['pl', 'en-US'] explicitly.

	Jonathan Kew [:jfkthame]
	Comment 18 • 8 years ago
mozreview-review

Comment on attachment 8828241 [details]
Bug 1332207 - Introduce mozilla::intl::LocaleService.

https://reviewboard.mozilla.org/r/105720/#review108592

This looks fine as a starting point, except that (like the OSPreferences patch) it's not going to build on Android until we've turned on ENABLE_INTL_API there.

I can't see how to give feedback+ in mozreview, but consider it given. :)

::: intl/locale/LocaleService.h:17
(Diff revision 5)
> +namespace mozilla {
> +namespace intl {
> +
> +class LocaleService
> +{
> +

lose the extra blank lines at start and end of the class decl, please

	Jonathan Kew [:jfkthame]
	Comment 20 • 8 years ago
mozreview-review

Comment on attachment 8828241 [details]
Bug 1332207 - Introduce mozilla::intl::LocaleService.

https://reviewboard.mozilla.org/r/105720/#review108734

::: intl/locale/LocaleService.h:26
(Diff revisions 5 - 6)
> +
>    static LocaleService* GetInstance();
>    static void Shutdown();
>  
> -  void GetAppLocales(nsTArray<nsCString>& aRetVal);
> +  bool GetAppLocales(nsTArray<nsCString>& aRetVal);
> +  nsCString GetAppLocale();

Unless it will be awkward for callers, I'd probably favor making this

    void GetAppLocale(nsACString& aRetVal);

to allow C++ callers to pass an nsAutoCString and avoid any allocations (because the locale code will fit in the stack-allocated autostring buffer), whereas returning an nsCString will inevitably involve a heap allocation.

Oh, and I don't actually see an implementation of this. Presumably it's just going to return the first locale in the list?

::: intl/locale/LocaleService.h:33
(Diff revisions 5 - 6)
>  protected:
>    static LocaleService* sInstance;
>    nsTArray<nsCString> mAppLocales;
> -
> +private:
> +  //XXX: This fires some warnings, not sure how to fix it :(
> +  ~LocaleService() {};

Make it `virtual ~LocaleService() {};`.

::: intl/locale/LocaleService.cpp:29
(Diff revisions 5 - 6)
>  
>    if (!uaLangTag.EqualsLiteral("en-US")) {
>      aRetVal.AppendElement(NS_LITERAL_CSTRING("en-US"));
>    }
> +
> +  return true;

Why make this return a `bool` if it's always `true`? Or do you have plans that involve potentially failing here?

::: intl/locale/LocaleService.cpp:32
(Diff revisions 5 - 6)
>    }
> +
> +  return true;
>  }
>  
> +bool AreLocaleListsEqual(nsTArray<nsCString>& locales1, nsTArray<nsCString>& locales2)

I think this is redundant, you should be able to simply compare two locale lists (arrays) with `==` or `!=` as required.

::: intl/locale/LocaleService.cpp:56
(Diff revisions 5 - 6)
>  {
>    if (!sInstance) {
>      sInstance = new LocaleService();
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +      obs->AddObserver(sInstance, "selected-locale-has-changed", false);

Where is this notification going to originate? If it has one specific source, it's likely to be more efficient to have that code get the locale service and directly call a "refresh" method of some kind, rather than going via the generic notification/observer mechanism. And then this class won't need to be an nsIObserver at all, afaics.

(But if you're expecting "selected-locale-has-changed" to be generated and monitored by various places around the code, the observer service may make sense. Not clear to me at this point.)

::: intl/locale/LocaleService.cpp:79
(Diff revisions 5 - 6)
> +  bool status = true;
>    if (mAppLocales.IsEmpty()) {
> -    ReadAppLocales(mAppLocales);
> +    status = ReadAppLocales(mAppLocales);
>    }
>    aRetVal = mAppLocales;
> +  return status;

Given that `ReadAppLocales` always returns `true`, this will also do so. Which makes this return value seem redundant, too.

::: intl/locale/LocaleService.cpp:89
(Diff revisions 5 - 6)
> +{
> +  if (strcmp(aTopic, "selected-locale-has-changed")) {
> +    nsTArray<nsCString> newLocales;
> +    ReadAppLocales(newLocales);
> +
> +    if (!AreLocaleListsEqual(mAppLocales, newLocales)) {

`if (newLocales != mAppLocales) ...`

::: intl/locale/LocaleService.cpp:90
(Diff revisions 5 - 6)
> +  if (strcmp(aTopic, "selected-locale-has-changed")) {
> +    nsTArray<nsCString> newLocales;
> +    ReadAppLocales(newLocales);
> +
> +    if (!AreLocaleListsEqual(mAppLocales, newLocales)) {
> +      //XXX: Should I free memory from the old list?

No, nsTArray should take care of that when you assign a new value to it; the deleted/replaced elements will be destroyed appropriately.

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

Thanks for the feedback! :)

> Oh, and I don't actually see an implementation of this. Presumably it's just
> going to return the first locale in the list?

Yep, will add the impl.

> Why make this return a `bool` if it's always `true`? Or do you have plans
> that involve potentially failing here?

Good point. I rememember saying that bool is an option for OSpreferences and you responded by saying that it's a good idea and we should do this for LocaleService as well, but now that I think about it, GetAppLocales will always use last-resort in case nothing else works so we will never return an empty list.

So, void?
 
> I think this is redundant, you should be able to simply compare two locale
> lists (arrays) with `==` or `!=` as required.

Whoa. cool :)
 
> ::: intl/locale/LocaleService.cpp:56
> (Diff revisions 5 - 6)
> >  {
> >    if (!sInstance) {
> >      sInstance = new LocaleService();
> > +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> > +    if (obs) {
> > +      obs->AddObserver(sInstance, "selected-locale-has-changed", false);
> 
> Where is this notification going to originate? If it has one specific
> source, it's likely to be more efficient to have that code get the locale
> service and directly call a "refresh" method of some kind, rather than going
> via the generic notification/observer mechanism. And then this class won't
> need to be an nsIObserver at all, afaics.

This is fired from nsChromeRegistry and LocaleService is the only subscriber.

I'm fine plugging us directly there.

In the future, we will have more code that may trigger the refresh - L10nRegistry, AddonsManager, ChromeRegistry, user preferences.

My vision was that this code will react to any change to requestedLanguages or availableLanguages and if the result is different than the current locale chain, we will trigger our one event "intl:app-locales-changed" which all the code can listen to.

Would you prefer to skip the nsiobserver and make L10nRegistry/AddonsManager/ChromeRegistry directly fire L10nRegistry.LocalesChanged ?
We'll then need only an observer on user prefs.


> 
> (But if you're expecting "selected-locale-has-changed" to be generated and
> monitored by various places around the code, the observer service may make
> sense. Not clear to me at this point.)

No, this code is already existing and I'm just subscribing to this temporarily until we can route all l10n through L10nRegistry and we don't need ChromeRegistry for l10n anymore.

> No, nsTArray should take care of that when you assign a new value to it; the
> deleted/replaced elements will be destroyed appropriately.

sweet.

	Jonathan Kew [:jfkthame]
	Comment 22 • 8 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #21)
> So, void?

Yeah, sounds good to me.

> This is fired from nsChromeRegistry and LocaleService is the only subscriber.
> 
> I'm fine plugging us directly there.
> 
> In the future, we will have more code that may trigger the refresh -
> L10nRegistry, AddonsManager, ChromeRegistry, user preferences.
> 
> My vision was that this code will react to any change to requestedLanguages
> or availableLanguages and if the result is different than the current locale
> chain, we will trigger our one event "intl:app-locales-changed" which all
> the code can listen to.
> 
> Would you prefer to skip the nsiobserver and make
> L10nRegistry/AddonsManager/ChromeRegistry directly fire
> L10nRegistry.LocalesChanged ?
> We'll then need only an observer on user prefs.

I think so. I'd suggest factoring out the update into a method called Refresh() or something like that, which could be called directly from nsChromeRegistryChrome::UpdateSelectedLocale(), and also used from the Observe() method when it is used to handle updates to the relevant user prefs.

(We'll still want a central method here that gets the new list, checks if it has changed, and then fires the app-locales-changed message as needed, since changes to any of the various inputs -- requested locales, available locales, user prefs -- might not actually result in a changed appLocales list.)

AFAICS, the "selected-locale-has-changed" notification itself that is currently being fired by nsChromeRegistry is not actually used for anything within Firefox; the only place it's used is in a unit test (test_bug519468.js). I suspect that as you migrate things, that test may become obsolete, at which point that notification can disappear altogether.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 25 • 8 years ago

> AFAICS, the "selected-locale-has-changed" notification itself that is currently being fired by nsChromeRegistry is not actually used for anything within Firefox; the only place it's used is in a unit test (test_bug519468.js). I suspect that as you migrate things, that test may become obsolete, at which point that notification can disappear altogether.

I'll deprecate it as we'll move away from ChromeRegistry for l10n. I don't want to do this here to prevent this patch from being blocked by some third-party code relying on that event.

	Jonathan Kew [:jfkthame]
	Comment 28 • 8 years ago
mozreview-review

Comment on attachment 8828241 [details]
Bug 1332207 - Introduce mozilla::intl::LocaleService.

https://reviewboard.mozilla.org/r/105720/#review110322

r- on this version because it'll leak (see below), but once that is fixed I think we'll be ready to go here.

::: intl/locale/LocaleService.cpp:46
(Diff revision 10)
> +void LocaleService::Shutdown()
> +{
> +  delete sInstance;
> +  sInstance = nullptr;
> +}

So one thing that's still missing is any actual call to `LocaleService::Shutdown`, which means that the LocaleService (and its list of locales) will currently leak.

On looking at this again, though, I think we have a better option than inserting a call to Shutdown somewhere. If you declare `sInstance` as a `StaticAutoPtr<LocaleService>` instead of a raw pointer, you should be able to use `mozilla::ClearOnShutdown(&sInstance)` right after it is initially created, and then forget all about it. This Shutdown method will no longer be needed at all.

(Untested... I haven't used ClearOnShutdown personally, but AFAICS this is how things should work.)

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 30 • 8 years ago

Updated the code and triggered all the tests.

	Jonathan Kew [:jfkthame]
	Comment 31 • 8 years ago
mozreview-review

Comment on attachment 8828241 [details]
Bug 1332207 - Introduce mozilla::intl::LocaleService.

https://reviewboard.mozilla.org/r/105720/#review110418

LGTM. A couple of minor nits, otherwise looks like it should be good to go. Clearly ReadAppLocales() is going to become more interesting in future, but this gives us a nice clean starting point.

::: intl/locale/LocaleService.h:6
(Diff revision 11)
> +#ifndef LocaleService_h__
> +#define LocaleService_h__

Style guide says the #include guard here should be `mozilla_intl_LocaleService_h`. (Yes, lots of existing code is not consistent with this.)

::: intl/locale/LocaleService.h:28
(Diff revision 11)
> + */
> +class LocaleService
> +{
> +public:
> +  static LocaleService* GetInstance();
> +  static void Shutdown();

Remove Shutdown() from the header as well as the .cpp file.

::: intl/locale/LocaleService.h:41
(Diff revision 11)
> +   * This API always returns at least one locale.
> +   *
> +   * Example: ["en-US", "de", "pl", "sr-Cyrl", "zh-Hans-HK"]
> +   *
> +   * Usage:
> +   * nsTArray<nsCString>& appLocales;

No '&' here; the caller would typically declare the actual array (not just a reference).

::: intl/locale/LocaleService.h:60
(Diff revision 11)
> +   * not have data for the first locale in the list.
> +   *
> +   * Example: "zh-Hans-HK"
> +   *
> +   * Usage:
> +   * nsCString appLocale;

I'd suggest using nsAutoCString in the usage example here; as I expect that will usually be the optimal way to use this, assuming the caller just needs the locale as a local (stack-based) variable.

::: intl/locale/LocaleService.cpp:6
(Diff revision 11)
> +#include "LocaleService.h"
> +#include "mozilla/Services.h"
> +#include "nsIObserverService.h"
> +#include "nsIToolkitChromeRegistry.h"
> +#include "mozilla/ClearOnShutdown.h"

Sort the #includes, please.

::: intl/locale/LocaleService.cpp:20
(Diff revision 11)
> + * This function performs the actual language negotiation for the API.
> + *
> + * Currently it collects the locale ID used by nsChromeRegistry and
> + * adds hardcoded "en-US" locale as a fallback.
> + */
> +void ReadAppLocales(nsTArray<nsCString>& aRetVal)

Declare this as

    static void
    ReadAppLocales(...)

(so that it doesn't become a globally-visible function).

Which reminds me, in the .cpp file we like to write the return type (along with `static`, if present) on a separate line before the function name. So please tweak these, throughout the file.

::: intl/locale/LocaleService.cpp:70
(Diff revision 11)
> +{
> +  nsTArray<nsCString> newLocales;
> +  ReadAppLocales(newLocales);
> +
> +  if (mAppLocales != newLocales) {
> +    mAppLocales = newLocales;

I think we should use

    mAppLocales = Move(newLocales);

here, to optimize the array assignment (let mAppLocales steal the contents of newLocales instead of copying each element).

(That'll require adding an #include for mozilla/Move.h, presumably.)

::: intl/locale/moz.build:40
(Diff revision 11)
>      'nsWin32Locale.h',
>  ]
>  
> +EXPORTS.mozilla.intl += [
> +    'LocaleService.h'
> +];

No semicolon here, please. (AFAIK, it's harmless but unneeded, and inconsistent with the rest of the file.)

	Pulsebot
	Comment 33 • 8 years ago

Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70d1ad7e8c89
Introduce mozilla::intl::LocaleService. r=jfkthame

	Carsten Book [:Tomcat]
	Comment 34 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/70d1ad7e8c89

	Sebastian Zartner [:sebo]
	Comment 35 • 8 years ago

Doesn't look like this patch exposes any web-facing API, so I'm removing the dev-doc-needed flag again.

Sebastian