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

We still have places around our codebase where we use Requested rather than AppLocales.

This is likely to be wrong since it makes us pick a different locale for that component from the locale used by Firefox UI.

Example:
 - requestedLocales: ["en-US"]
 - availableLocales: ["de-DE"]
 - appLocales: ["de-DE"]

In that case, Firefox UI will be in de-DE because that's the appLocales we negotiated down to. But Activity Stream will use en-US because that's the API call it uses.

It's trivial to fix since both LocaleService::GetAppLocales and LocaleService::GetRequestedLocales share the signature.

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

Ed, can you help me here? I wrote a POC of a patch to move Activity Stream to use AppLocales, but got stuck because I don't know how to fire activity stream tests and which tests apply to which code.

I hope the POC makes it easy for someone with the right setup to complete it.

	Ed Lee :Mardak Assignee
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8935164 [details]
Bug 1423703 - Use AppLocales rather than RequestedLocales in Activity Stream.

https://reviewboard.mozilla.org/r/206014/#review211618

::: browser/components/newtab/aboutNewTabService.js:189
(Diff revision 1)
>        path = "static";
>      } else {
> -      // Use the exact match locale if it's packaged
> -      const locale = Services.locale.getRequestedLocale();
> -      if (ACTIVITY_STREAM_LOCALES.has(locale)) {
> -        path = locale;
> +      let locales = Services.locale.negotiateLanguages(
> +        Services.locale.getAppLocalesAsLangTags(),
> +        ACTIVITY_STREAM_LOCALES,
> +        Services.locale.defaultLocale

Should this `.defaultLocale` be "en-US" as it should be a locale that should be "guaranteed" to be packaged. In particular `ACTIVITY_STREAM_LOCALES` does *not* include en-US -- maybe it should?

::: browser/extensions/activity-stream/lib/ActivityStream.jsm:295
(Diff revision 1)
>        // Watch for geo changes and use a dummy value for now
>        Services.prefs.addObserver(GEO_PREF, this);
>        this.geo = "";
>      }
>  
> -    this.locale = Services.locale.getRequestedLocale();
> +    this.locale = Services.locale.getAppLocaleAsLangTag();

This `locale` is used for determining if stories / Pocket should be shown:
https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/ActivityStream.jsm#182-189

Will tha be affected in some odd ways with this change? (Separately, we could probably use `negotiateLanguages` for this Pocket selection logic…)

	Ed Lee :Mardak Assignee
	Comment 4 • 7 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> Example:
>  - requestedLocales: ["en-US"]
>  - availableLocales: ["de-DE"]
>  - appLocales: ["de-DE"]
How can I set up Firefox to get to this state to test?

	Ed Lee :Mardak Assignee
	Comment 5 • 7 years ago

Oh, on latest de Nightly:

["getRequestedLocale", "getAvailableLocales", "getAppLocalesAsLangTags"].map(m => Services.locale[m]()).join("\n")

en-US
de
de

Is this due to bug 1414390?

Compare to de 57.0.1:
de
de
de,en-US

	Ed Lee :Mardak Assignee
	Comment 6 • 7 years ago

gandalf, also, is it expected that lang repacks only set general.useragent.locale and *not* set intl.locale.requested?

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

(In reply to Ed Lee :Mardak from comment #3)
> Comment on attachment 8935164 [details]
> Bug 1423703 - Use AppLocales rather than RequestedLocales in Activity Stream.
> 
> https://reviewboard.mozilla.org/r/206014/#review211618
> 
> ::: browser/components/newtab/aboutNewTabService.js:189
> (Diff revision 1)
> >        path = "static";
> >      } else {
> > -      // Use the exact match locale if it's packaged
> > -      const locale = Services.locale.getRequestedLocale();
> > -      if (ACTIVITY_STREAM_LOCALES.has(locale)) {
> > -        path = locale;
> > +      let locales = Services.locale.negotiateLanguages(
> > +        Services.locale.getAppLocalesAsLangTags(),
> > +        ACTIVITY_STREAM_LOCALES,
> > +        Services.locale.defaultLocale
> 
> Should this `.defaultLocale` be "en-US" as it should be a locale that should
> be "guaranteed" to be packaged.

We have two different notions:
 - defaultLocale      - this is the default locale for the given build. For example "de Firefox" will have "de" as the default locale
 - lastFallbackLocale - this is the fallback locale that we use as "last resort" - if all else fail, try this locale.

Currently, lastFallback is hardcoded to be "en-US", but default depends on what packaged build you have.

Notice that in repackaged Firefox, we do not carry en-US strings, so we have to use defaultLocale here, because if the whole negotiation fails (say, you ask for 'de-DE' and only have 'fr-CA' available), we have to fallback on a locale the build is packaged for, which is defaultLocale.

> In particular `ACTIVITY_STREAM_LOCALES` does *not* include en-US -- maybe it should?

if you provide the en-US locale data, then yes, it should be in ACTIVITY_STREAM_LOCALES (which is what we called `availableLocales`).

> This `locale` is used for determining if stories / Pocket should be shown:
> https://searchfox.org/mozilla-central/source/browser/extensions/activity-
> stream/lib/ActivityStream.jsm#182-189
> 
> Will tha be affected in some odd ways with this change? (Separately, we
> could probably use `negotiateLanguages` for this Pocket selection logic…)

The only change here is that if the user asks for "french", but Firefox doesn't have it, and in result shows UI in "de", we will show the stories / Pocket for the Firefox UI locale ("de") rather than user requested locale ("fr").

The way we usually approach it is that independent software can aim to fulfill the best locale based on user requested list. So, for example a standalone Pocket can be localized into "fr-CA" even if Firefox UI cannot.
This means that it can use requestedLocales against its own available locales to get the best possible match (using the negotiateLanguages algorithm).

On the other hand, if something is a part of Firefox UI, it should be consistent with Firefox UI locale. If the user requests "fr-CA", but Firefox UI has only "fr" or not even that, parts of the Firefox UI should stay consistent with it.

That means that the correct negotiation should be between resolved Firefox UI locales and available locales for your feature.

To speak code:

Indepenent code can do:

negotiateLanguages(
  Services.locale.getRequestedLocales(),
  MY_AVAILABLE_LOCALES,
  Services.locale.defaultLocale
);

Firefox UI pieces should do:

negotiateLanguages(
  Services.locale.getAppLocalesAsLangTags(),
  MY_AVAILABLE_LOCALES,
  Services.locale.defaultLocale
);

because AppLocales is a list *already* negotiated between requested locales and FIREFOX_UI_AVAILABLE_LOCALES, so now you take that result, and try to match the best locale for Activity Stream for it.

>> Example:
>>  - requestedLocales: ["en-US"]
>>  - availableLocales: ["de-DE"]
>>  - appLocales: ["de-DE"]
> How can I set up Firefox to get to this state to test?

You can easily manipulate the requested locales using `setRequestedLocales()`, and for AvailableLocales you unfortunately need to provide the ChromeRegistry resources for it.
In the future, once we migrate to Fluent and the new L10nRegistry, we hope to enable to mock availableLocales as well (and in result, allow for testing against multiple appLocales). See bug 1358653 for that.

For now, I believe that you're already stubbing Services.locale, so you can just stub Services.locale.getAppLocalesAsLangTags to return your desired value.

Hope that helps!

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

(In reply to Ed Lee :Mardak from comment #5)
> Oh, on latest de Nightly:
> 
> ["getRequestedLocale", "getAvailableLocales",
> "getAppLocalesAsLangTags"].map(m => Services.locale[m]()).join("\n")
> 
> en-US
> de
> de

Yes!

> 
> Is this due to bug 1414390?
> 

Yes! We just fixed it in bug 1423618, but that regression allowed me to discover that the whole Firefox UI was behaving properly (because it relies on AppLocales) while Activity Stream used requestedLocales (which were wrong).

> gandalf, also, is it expected that lang repacks only set general.useragent.locale and *not* set intl.locale.requested?

We removed `general.useragent.locale` completely in bug 1414390.

And with the fix in bug 1423618 we removed the default value for `intl.locale.requested`.

So the result is that now when you install a build, either en-US or a repack, you will not have either of those prefs set at all, and LocaleService will interpret this "the user does not specify any requests for locales, let's use the default one!" and return the defaultLocale in getRequestedLocales and of course in getAppLocales as well.

I'm sorry that it's complicated :) Locale Negotiation is a matrix of possibilities against availabilities and it can be nested (like in this case, where the first negotiation is between FX_AVAILABLE and requested, and then between ACTIVITY_STREAM_AVAILABLE and FX_SELECTED).

	Ed Lee :Mardak Assignee
	Comment 9 • 7 years ago

After some more discussion on IRC, it looks like bug 1423618 should fix the problem of activity stream showing up as en-US for all nightly users, so there's less of an immediate rush to fix this particular bug, which should get some design and product feedback before prioritization.

uiwanted: The decision here is whether activity stream should match the Firefox UI's locale instead of the user's requested locale. Currently activity stream is displayed in the user's requested locale. gandalf filed this bug as "my team's position is that if it's an independent feature (say, Pocket extension) it may make sense for it to use [the requested locale], but if it's a part of Firefox UI then it should follow the UI"

I believe currently the only "UI" to set the requested locale is via about:config or browser console JS, but the plan is to increase user controls via WebExtensions bug 1352884 and directly in Firefox Preferences bug 1325870. But even with those, most users will probably have the default requested locale being the same as the Firefox UI locale.

gandalf, would it be safe to say the situations where requested locale and Firefox UI locale are different should be relatively exceptional cases? E.g., Firefox UI to pick language has a user requesting German, but Firefox cannot activate the German UI (network failure, missing files on server, explicitly removed for security)?

	Zibi Braniecki [:zbraniecki][:gandalf] Reporter
	Comment 10 • 7 years ago

> so there's less of an immediate rush to fix this particular bug, which should get some design and product feedback before prioritization.

Agree.

> uiwanted: The decision here is whether activity stream should match the Firefox UI's locale instead of the user's requested locale.

Correct.

The Firefox UI locale(s) are negotiated between requested by the user and available for Firefox.
In human terms it means "out of available languages in Firefox, pick the ones that the user wants to see in the order of his selected priorities".

The Activity Stream has two choices:

1) It can take once again the requested locales, negotiate them against its own available ones and pick the best ones.
2) It can take the already negotiated locales from Firefox, negotiate them against its own available ones and pick the best ones.

The difference is that in (1) case it is more likely that Activity Stream will end up in a different locale than Firefox UI.

My advice is that since Activity Stream is now part of Firefox UI it should follow Firefox UI locales and aim to stick to them.

If, for whatever reason, Firefox UI ends up in a different locale than requested, Activity Stream should use that locale as well.

> gandalf, would it be safe to say the situations where requested locale and Firefox UI locale are different should be relatively exceptional cases?

At the moment yes. But as we improve our ability to select and negotiate locales this may become less strict and more complex. For example, we may be able to introduce different "sets" of strategies for different components, where user could request one set of locales for Firefox UI, another for DevTools and yet another for extensions. We would then negotiate different sets of locales out of those and available locales for those components.

I would like to aim to remove any calls to requested locales in our codebase outside of LocaleService.
My take is that Requested Locales are basically only useful to select the AppLocales, and those should be used by the codebase.

Pike - what is your take on this?

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

Sorry for the lag, had to find some time to reread the bug.

My expectation is that AS follows the Firefox UI language.

(I do see the trick about reqestedlocales being cool for debugging right now, but we do have bug 1358653 for that)

	Ed Lee :Mardak Assignee
	Comment 12 • 7 years ago

design agrees that activity stream should match firefox UI.

	Ed Lee :Mardak Assignee
	Comment 13 • 7 years ago

gandalf, any thoughts on florian's concern from bug 1412930 comment 20? Basically, activity stream needs to package all locales to be able to show the appropriate html/strings when the user changes the requested locale. The change here would make it less likely that the locale will change… until the rest of Firefox UI is dynamically loading in the lang packs…?

In bug 1411452, it was decided against putting activity stream in langpacks as allowing them to package more than just strings is potentially a security issue with arbitrary <script> to steal data from activity stream content page.

So as with some other system-addons, activity stream just packages all locales as part of all Firefox builds.

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

(In reply to Ed Lee :Mardak from comment #13)
> gandalf, any thoughts on florian's concern from bug 1412930 comment 20?
> Basically, activity stream needs to package all locales to be able to show
> the appropriate html/strings when the user changes the requested locale. The
> change here would make it less likely that the locale will change… until the
> rest of Firefox UI is dynamically loading in the lang packs…?

Nothing changes here I believe. If I understand it correctly, AS is trying to maintain its own list of locales that matches the list of locales we release Firefox in.

The fact that AS packages all of those locales for each build is... unconventional.. and I hope temporary, but doesn't change the list of locales we should be keeping available for AS.
 
> In bug 1411452, it was decided against putting activity stream in langpacks
> as allowing them to package more than just strings is potentially a security
> issue with arbitrary <script> to steal data from activity stream content
> page.

We have a safe localization model via the old API - StringBundle and DTD, and with the upcoming Fluent and Fluent-React.

I believe that your ultimate target is to use Fluent-React as soon as possible. Fluent-React provide a good security characteristic for rich React/HTML localized content via DOM Overlays mechanism - https://github.com/projectfluent/fluent.js/tree/master/fluent-react

It would be great to plan this for Q1, and :stas, the author and maintainer of the Fluent-React bindings, should be able to mentor the AS team through it.
 
> So as with some other system-addons, activity stream just packages all
> locales as part of all Firefox builds.

We're planning to extend Fluent and Fluent-React to work with system addons, but we don't have bugs filed yet. I plan to work on that in Q1.

	Ed Lee :Mardak Assignee
	Comment 15 • 7 years ago

gandalf, we have mochitests that make sure every Firefox build can access any packaged locales when requested requested:
https://searchfox.org/mozilla-central/source/browser/components/newtab/tests/browser/browser_packaged_as_locales.js#48

By switching to purely getAppLocales, how would we maintain that test without bug 1358653?

I suppose we could just get rid of that test and just make sure it passes for en-US as that's the only app locale used when running these tests…?

	Zibi Braniecki [:zbraniecki][:gandalf] Reporter
	Comment 16 • 7 years ago

If I understand your code correctly all you're really testing is that [0] updates the path?

Because [1] does not attempt to access anything if I read it correctly.

So, if you want to keep the test you could mock the `Services.intl.getAppLocales*` to return the locale chain you want and see if the defaultURL path updates correctly, but I'm not sure if I see the value in the test itself :(

[0] https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutNewTabService.js#212
[1] https://searchfox.org/mozilla-central/source/browser/components/newtab/tests/browser/browser_packaged_as_locales.js#48

	Ed Lee :Mardak Assignee
	Comment 17 • 7 years ago

The test is making sure that Firefox allows accessing all of those packaged locales via about:newtab. I.e., the test fails if we package a locale but about:newtab fails to use that locale and uses en-US instead.

Additionally, it specially tests that "id" locale is packaged and accessible via about:newtab.

Is there a standard way to mock Services.* now in mochitests that change the behavior of components/modules that have already loaded Services.jsm?

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

I don't know of any standardized way (I wish we used sinon tbh). I usually just do the:

```
let originalAPI = Services.intl.API;

Services.intl.API = function mockedAPI() {};

...

Services.intl.API = originalAPI;
```

	Ed Lee :Mardak Assignee
	Comment 19 • 7 years ago

Oh. okay. ;) I believe there's been some more recent-ish discussion of standard stubbing / mocking of interfaces, so I wasn't sure if there was some new fancy already.

Looks like monkey patching won't be so easy:

Object.getOwnPropertyDescriptor(Services.locale, "getAppLocaleAsLangTag")
Object { value: getAppLocaleAsLangTag(), writable: false, enumerable: true, configurable: false }

Maybe we'll just remove the test and hope we don't forget to update ACTIVITY_STREAM_LOCALES when adding additional locales. It's not super frequent and should be less often now although in the 59 nightly cycle, we have added gl, gn, si.

	Zibi Braniecki [:zbraniecki][:gandalf] Reporter
	Comment 20 • 7 years ago

Damn. In 60 I hope to migrate us to get GetAppLocales directly from L10nRegistry (rather than from ChromeRegistry) and that will make it much easier for you to mock it.

You'll do something like:

```
Services.locale.getAppLocalesAsLangTags(); // ["en-US"]

let fs = new FileSource("mock-source-de", ["de"], "/dev/null");
L10nRegistry.registerSource(fs);

Services.locale.getAppLocalesAsLangTags(); // ["de", "en-US"]

L10nRegistry.removeSource("mock-source-de");

Services.locale.getAppLocalesAsLangTags(); // ["en-US"]
```

so if you want you can also just disable the test and wait for the switch.

	[github robot]
	Comment 22 • 7 years ago

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/ea0b2b3346e3a77bc13e2b858f6fa53f1477bee9
fix(locales): Use app locale to match Firefox UI instead of user requested locales (#3920)

Fix Bug 1423703 - Use negotiated locales, rather than requested in Activity Stream

	Ed Lee :Mardak Assignee
	Comment 23 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/0d163e2c4792