Closed
Bug 1410731
Opened 3 years ago
Closed 3 years ago
[l20n] Fix selection of packaged locales at runtime
Categories
(Core :: Internationalization, enhancement, P3)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
Attachments
(1 file)
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 | ||
Updated•3 years ago
|
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Priority: -- → P3
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•3 years ago
|
||
Putting it here just as an option for how we could solve it.
Assignee | ||
Comment 3•3 years ago
|
||
Aaand forgot to set NI on Pike to get his feedback :)
Flags: needinfo?(l10n)
Comment 4•3 years ago
|
||
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)
Assignee | ||
Comment 5•3 years ago
|
||
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)
Comment 6•3 years ago
|
||
"just" seems to be the culprit to me, don't have an answer that I'd suggest to build upon.
Flags: needinfo?(l10n)
Assignee | ||
Comment 7•3 years ago
|
||
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
Comment 8•3 years ago
|
||
(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)
Assignee | ||
Comment 9•3 years ago
|
||
Note to myself: once we fix bug 1362617, we'll still need to make repackaging update it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•3 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 13•3 years ago
|
||
> 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)
Comment 14•3 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•3 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment 19•3 years ago
|
||
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
![]() |
||
Comment 20•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2a69f4fe898
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•