1410733 - Open up LocaleService::GetRequestedLocales to more than one entry

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

When we were refactoring the Intl code, we limited the list of requested locales to a single item.

We knew we'll need more than one for Fluent, and now that we're planning to enable Fluent in Firefox, we need to alleviate this limitation.

That affects two methods:

1) setRequestedLocales should not assert that the length of provided list is 1
2) getRequestedLocales should add a fallback locale. It's ok to add en-US as this is the fallback locale we use at build time.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1410733 - Add an en-US locale as a hard fallback for LocaleService::RequestedLocales.

Review commit: https://reviewboard.mozilla.org/r/191844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191844/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8920897 [details]
Bug 1410733 - Add an en-US locale as a hard fallback for LocaleService::RequestedLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191844/diff/1-2/

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1410733 - Minor cleanups to the callers of GetRequestedLocales.

Once we add fallback chain to GetRequestedLocales we can slightly improve the
locale negotiation for extensions. I made it tighter against just `en-US` because
in the future it is possible that RequestedLocales fallback chain will not contain
en-US in some scenarios, and it seems that for WebExtensions en-US should be the
last resort no matter what.

The other change is a fix to a regression I introduced when switching to LocaleService,
that somehow noone noticed.

Review commit: https://reviewboard.mozilla.org/r/191854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191854/

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8920897 [details]
Bug 1410733 - Add an en-US locale as a hard fallback for LocaleService::RequestedLocales.

https://reviewboard.mozilla.org/r/191844/#review197138

The change to SetRequestedLocales here seems incomplete: it removes the "only one locale" assertion, but then doesn't do anything with the rest of the list - AFAICS, they'll just be discarded. I'd be more comfortable keeping the assertion in place there until the method is updated to actually handle the list.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8920897 [details]
Bug 1410733 - Add an en-US locale as a hard fallback for LocaleService::RequestedLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191844/diff/2-3/

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8920901 [details]
Bug 1410733 - Minor cleanups to the callers of GetRequestedLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191854/diff/1-2/

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ah, of course! Thank you for noticing it :)

That means that bug 1410736 is blocking us. I'd like to get to it, but not sure if in 58 or 59.
Fortunately, I think for our ability to move forward with l20n, we don't need the setting part, just the getting should add the fallback locale.

Fixed the patch :)

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8920897 [details]
Bug 1410733 - Add an en-US locale as a hard fallback for LocaleService::RequestedLocales.

https://reviewboard.mozilla.org/r/191844/#review197560

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8920901 [details]
Bug 1410733 - Minor cleanups to the callers of GetRequestedLocales.

https://reviewboard.mozilla.org/r/191854/#review197562

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84f325dfaa00
Add an en-US locale as a hard fallback for LocaleService::RequestedLocales. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/3bd7e87109e0
Minor cleanups to the callers of GetRequestedLocales. r=jfkthame

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b17da389d8d
Backed out changeset 3bd7e87109e0 
https://hg.mozilla.org/integration/autoland/rev/1814b6aba6e3
Backed out changeset 84f325dfaa00 for failing xpcshell's intl/l10n/test/test_localization.js on debug. r=backout

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8920897 [details]
Bug 1410733 - Add an en-US locale as a hard fallback for LocaleService::RequestedLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191844/diff/3-4/

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8920901 [details]
Bug 1410733 - Minor cleanups to the callers of GetRequestedLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191854/diff/2-3/

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, the reason it failed is because in tests we often do:

```
function teststh() {
  var originalRequested = locSvc.getRequestedLocales();
  
  locSvc.setRequestedLocales(['x-test']);
  ...
  locSvc.setRequestedLocales(originalRequested);
}
```

and with this patch we stopped supporting placing in requestedLocales the list produced by getRequestedLocales.

I fixed it for now by special casing the en-US as the last fallback. Since we're adding it, we don't need to place it back, so the system should be consistent, and allow for tests to pass without changes.

Jonathan, can you check if you're ok with this?

Comment 15

[Jonathan Kew [:jfkthame]]

Yeah, I suppose so. It feels a bit ugly/hacky, but it's only a temporary measure until SetRequested actually supports a list of multiple locales.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8e007846661
Add an en-US locale as a hard fallback for LocaleService::RequestedLocales. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/e7fc4fb91ca6
Minor cleanups to the callers of GetRequestedLocales. r=jfkthame

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks! I agree. I'll try to move us away from general.useragent.locale in 59

Comment 18

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/b8e007846661
https://hg.mozilla.org/mozilla-central/rev/e7fc4fb91ca6