Closed Bug 1410733 Opened 3 years ago Closed 3 years ago

Open up LocaleService::GetRequestedLocales to more than one entry

Categories

(Core :: Internationalization, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Priority: -- → P3
See Also: → 1410736
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.
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 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
Attachment #8920897 - Flags: review?(jfkthame) → review+
Comment on attachment 8920901 [details]
Bug 1410733 - Minor cleanups to the callers of GetRequestedLocales.

https://reviewboard.mozilla.org/r/191854/#review197562
Attachment #8920901 - Flags: review?(jfkthame) → review+
Pushed by zbraniecki@mozilla.com:
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
Backout by archaeopteryx@coole-files.de:
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
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?
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
Pushed by zbraniecki@mozilla.com:
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
Thanks! I agree. I'll try to move us away from general.useragent.locale in 59
https://hg.mozilla.org/mozilla-central/rev/b8e007846661
https://hg.mozilla.org/mozilla-central/rev/e7fc4fb91ca6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.