Closed Bug 1410731 Opened 3 years ago Closed 3 years ago

[l20n] Fix selection of packaged locales at runtime

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1410731 - Use multilocale.json as a locale set for L10nRegistry sources.
59 bytes, text/x-review-board-request
mossop
: review+
Details 
The code in http://searchfox.org/mozilla-central/rev/091894faeac5b54b7e40b0a304c3d3268f7b645d/browser/components/nsBrowserGlue.js#671

has two issues:

1) it does not add a fallback locale, even when such is present in the build
2) the AppConstants.INSTALL_LOCALE does not get updated on repack

My initial idea to fix the former is to either:
a) Fix bug 1362617 to get multilocale.json in the build and load it at startup
b) hardcode en-US as the fallback locale

While the (a) is definitely preferred in the long run, I believe that (b) could be used temporarily. We do hard-code fallback to en-US at build-time, so it feels ok to me to do the same at runtime until (a) is solved.

For (2), my initial idea would be to switch to use `resource://gre/update.locale` which does update on repack. But maybe there's a deeper problem that AppConstants.INSTALL_LOCALE should update?
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Priority: -- → P3
Putting it here just as an option for how we could solve it.
Aaand forgot to set NI on Pike to get his feedback :)
Flags: needinfo?(l10n)
Mossop, you introduced INSTALL_LOCALE in bug 1192924. It seems that most things in that jsm are actually constant per app, while the INSTALL_LOCALE *should* actually depend on the distributed locale, and notably the update locale.

Rather than trying to wrestle the build folks into letting us modify a JSM as part of the repacks, I'd lobby to rightout remove that one, and replace its use in the test_UpdateUtils_url.js and here. Probably with different values.

Re update.locale, I'm not fond of that file. TBH, I'm surprised that we use it, and by the contents of it right now. I would have thought that the multi fennec nightly sets update.locale to 'multi', but it doesn't (it's en-US). No idea if that's a bug or if that's intended, now that we have Nightly in google play.

Guess I'm leaning towards a) via multilocale.json for this bug.
Flags: needinfo?(l10n) → needinfo?(dtownsend)
Thanks Axel. I'll work with Dave on this.

One follow-up question to your guidance.

Since for now, the multilocale.json will always look like: ["ab-CD", "en-US"] on Desktop, would you be ok with temporarily getting just the ab-CD and adding `en-US`, like I did in my patch here, to unblock us while we work on bug 1362617?
Flags: needinfo?(l10n)
"just" seems to be the culprit to me, don't have an answer that I'd suggest to build upon.
Flags: needinfo?(l10n)
Ok. Let's set bug 1362617 as blocking this one and see how much work it'd take to get the multilocale.json operational.
Depends on: 1362617
(In reply to Axel Hecht [:Pike] from comment #4)
> Mossop, you introduced INSTALL_LOCALE in bug 1192924. It seems that most
> things in that jsm are actually constant per app, while the INSTALL_LOCALE
> *should* actually depend on the distributed locale, and notably the update
> locale.
> 
> Rather than trying to wrestle the build folks into letting us modify a JSM
> as part of the repacks, I'd lobby to rightout remove that one, and replace
> its use in the test_UpdateUtils_url.js and here. Probably with different
> values.

Sounds fine to me
Flags: needinfo?(dtownsend)
Note to myself: once we fix bug 1362617, we'll still need to make repackaging update it.
Comment on attachment 8920898 [details]
Bug 1410731 - Use multilocale.json as a locale set for L10nRegistry sources.

https://reviewboard.mozilla.org/r/191848/#review198228

So I'm a bit wary of this because your fetch is going to race with other parts of startup. We we set up to cope with the possibility that we don't register the sources until after UI has started to be painted?

::: browser/components/nsBrowserGlue.js:627
(Diff revision 3)
>      this._distributionCustomizer.applyPrefDefaults();
>    },
>  
>    // runs on startup, before the first command line handler is invoked
>    // (i.e. before the first window is opened)
> -  _beforeUIStartup: function BG__beforeUIStartup() {
> +  _beforeUIStartup: async function BG__beforeUIStartup() {

I don't want to make this function async or it invalidates the comment above. If we need the l10n stuff to be async then split it out into its own async function that gets called from _beforeUIStartup
Attachment #8920898 - Flags: review?(dtownsend) → review-
> So I'm a bit wary of this because your fetch is going to race with other parts of startup. We we set up to cope with the possibility that we don't register the sources until after UI has started to be painted?

With async generator in L10nRegistry we can make generateContexts wait for this to be done. It will make it not racy, so the worst outcome will be l10n done after first paint.

Alternatively, we can try to load this multilocale.json synchronously in C++ in LocaleService and just populate LocaleService::GetAvailableLocales with the data from it.

Would you recommend that?
Flags: needinfo?(dtownsend)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #13)
> > So I'm a bit wary of this because your fetch is going to race with other parts of startup. We we set up to cope with the possibility that we don't register the sources until after UI has started to be painted?
> 
> With async generator in L10nRegistry we can make generateContexts wait for
> this to be done. It will make it not racy, so the worst outcome will be l10n
> done after first paint.

This sounds good. Chances are you'll win the race but we should be sure.
Flags: needinfo?(dtownsend)
Comment on attachment 8920898 [details]
Bug 1410731 - Use multilocale.json as a locale set for L10nRegistry sources.

https://reviewboard.mozilla.org/r/191848/#review200094
Attachment #8920898 - Flags: review?(dtownsend) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2a69f4fe898
Use multilocale.json as a locale set for L10nRegistry sources. r=mossop
https://hg.mozilla.org/mozilla-central/rev/c2a69f4fe898
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1433411
You need to log in before you can comment on or make changes to this bug.