Closed Bug 796051 Opened 7 years ago Closed 7 years ago
Generate make target to package needed l10n files in gecko
We need a few files from toolkit to generate localized error messages, and maybe more. I don't think we want full toolkit. This is shared with what mobile does in bug 792077. The current list of overrides I need to see localized error messages is just override chrome://global/locale/netError.dtd chrome://b2g-l10n/locale/netError.dtd override chrome://global/locale/global.dtd chrome://b2g-l10n/locale/global.dtd override chrome://global/locale/intl.properties chrome://b2g-l10n/locale/intl.properties override chrome://global/locale/intl.css chrome://b2g-l10n/locale/intl.css override chrome://global/locale/appstrings.properties chrome://b2g-l10n/locale/appstrings.properties locale b2g-l10n de jar:de.jar!/locale/de/ (to quote my fake de.manifest.)
Blocks a blocker so basecamp-blocking+.
blocking-basecamp: ? → +
Who can do this work?
This should be fairly easy with the work in bug 797745. We'd then mimick the chrome-% target in mobile/android/locales/Makefile.in. Makefile gurus can help here, anyone from the toolkit/build peers. As time permits, I could also step in.
Depends on: 797745
I've got a work-in-progress locally now, even though I'm hitting a few snags still. Taking.
Assignee: nobody → l10n
Margaret, do you think you can review this? The patch somewhat depends on the branding decisions that stas is driving still. Here's what I'm doing so far: IMHO, we only need netError.dtd and appstrings.properties, in one combo, for neterrors, with dom's global.dtd for LTR. From toolkit intl.properties for accept-lang, and I've taken intl.css for neterror page css overrides, just in case. Right now, the netError strings are verbatim the ones we use for mobile, so I packaged those up. The benefit of that is that we don't need to localize them. The downside is that they're hardcoding our branding to say "Firefox". In addition, I changed l10n.ini, to not compare anything for b2g, but to pick up mobile, wich in return picks up toolkit, which also has dom. I paired that with a filter.py that ignores anything that's not in the 5 picked files. I added a chrome-% target which is the same thing we have for mobile for building per-locale dist. I did not: - remove all the files we don't use from b2g/locales - think about installer/crashreporter - think about single-locale repacks - think about packager.mk, beyond comparing the one in mobile with b2g, and the MULTI_LOCALE packaging stuff needs to be ported at least. Also asking Mark for feedback, as this is generally build stuff.
FYI, this went through try, https://tbpl.mozilla.org/?tree=Try&rev=1ddc9866ec77
Comment on attachment 679088 [details] [diff] [review] use strings from mobile, dom, toolkit, none of b2g Review of attachment 679088 [details] [diff] [review]: ----------------------------------------------------------------- From build perspective, this looks sane.
Attachment #679088 - Flags: feedback?(mh+mozilla) → feedback+
Margaret, can you get to the review here? It'd be good to get the starting points to start doing automation work.
(In reply to Axel Hecht [:Pike] from comment #8) > Margaret, can you get to the review here? It'd be good to get the starting > points to start doing automation work. Thanks for the ping, this got lost during the work week last week. I'll look at it now.
Comment on attachment 679088 [details] [diff] [review] use strings from mobile, dom, toolkit, none of b2g Review of attachment 679088 [details] [diff] [review]: ----------------------------------------------------------------- This looks like it makes sense, but I don't think that I can actually review this. I'm not sure who would be the best reviewer, but maybe cjones can review it (or knows who should). ::: b2g/locales/filter.py @@ +10,2 @@ > "b2g"): > + return "ignore" Should this be False instead of "ignore"?
(In reply to Margaret Leibovic [:margaret] from comment #10) > Comment on attachment 679088 [details] [diff] [review] <...> > > ::: b2g/locales/filter.py > @@ +10,2 @@ > > "b2g"): > > + return "ignore" > > Should this be False instead of "ignore"? The code in http://hg.mozilla.org/l10n/compare-locales/file/c4d0d7a859ff/lib/Mozilla/Paths.py#l309 makes those equal, and I found the string return values to be easier to understand than the bools.
Comment on attachment 679088 [details] [diff] [review] use strings from mobile, dom, toolkit, none of b2g Sorry for the review lag, I've been traveling and mostly offline. Looks fine to me.
Attachment #679088 - Flags: review?(jones.chris.g) → review+
Marking for C2, given this meets the criteria of known P1/P2 blocking-basecamp+ bugs at the end of C1.
Target Milestone: --- → B2G C2 (20nov-10dec)
I tried to evaluate the impact of bug 812784, which is about this approach breaking getSelectedLocale('global') and other toolkit chrome providers. Going through http://mxr.mozilla.org/mozilla-central/ident?i=getSelectedLocale&filter=, I triaged the following regressions: B2G critical, AFAICT: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#251, extracting locale-specific meta data from web app manifests B2G not-so-critical: All things using nsURLFormatter, http://mxr.mozilla.org/mozilla-central/ident?i=formatURL http://mxr.mozilla.org/mozilla-central/ident?i=formatURLPref Health report and telemetry reporting 'en-US' as locale Locale-specific content and protocol handlers won't work right now. RTL won't work I'll need someone with fx os knowledge to verify that triage. Chris, can you provide that, or redirect?
(In reply to Axel Hecht [:Pike] from comment #14) > I tried to evaluate the impact of bug 812784, which is about this approach > breaking getSelectedLocale('global') and other toolkit chrome providers. > > Going through > http://mxr.mozilla.org/mozilla-central/ident?i=getSelectedLocale&filter=, I > triaged the following regressions: > > B2G critical, AFAICT: > http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#251, > extracting locale-specific meta data from web app manifests This is bug 815918, I'll mark it depending on bug 812784
Comment on attachment 679088 [details] [diff] [review] use strings from mobile, dom, toolkit, none of b2g This patch creates too many problems right now by prematurely optimizing the size of the toolkit strings impact. Let's push that out. New patch coming up now, with interdiff comparing to this one.
Attachment #679088 - Attachment is obsolete: true
Just use the full toolkit strings with the mobile neterror strings. Pushed to https://tbpl.mozilla.org/?tree=Try&rev=80949c61952f This doesn't change our branding strings off of B2G, despite what I thought. Mostly a question of what branding we'd want for unofficial, I'll move that question back to bug 808326. Requesting review from cjones again.
Attachment #686334 - Flags: review?(jones.chris.g)
Comment on attachment 686334 [details] [diff] [review] package all of toolkit for locales with the chrome-% target, make compare-locales return the right results. Hm, I don't expect us to be using anything from services/sync. On second thought too, depending on mobile/ doesn't seem right either. Other than that this seems fine, but r? to fabrice to comment on above.
Attachment #686334 - Flags: review?(jones.chris.g) → review?(fabrice)
Comment on attachment 686334 [details] [diff] [review] package all of toolkit for locales with the chrome-% target, make compare-locales return the right results. Review of attachment 686334 [details] [diff] [review]: ----------------------------------------------------------------- r- because: - we don't use sync - we don't use extension/spellcheck (gaia has its own solution) - the references to mobile are a legacy from the creation of b2g/ from a clone of the mobile/ scaffolding, and must be removed. The only strings we need on the gecko side are for the netError.html page, and (currently, but we'll move it to gaia) the "slow script" dialog.
Attachment #686334 - Flags: review?(fabrice) → review-
I went for mobile strings, because we have those for the 1.0 locales. Seems we need the same code base in locales we don't have in mobile, though, so going for b2g strings with a locale list there is probably the right thing to do. The current state of b2g/locales/en-US wasn't in shape to expose to localizers, so this does some major cleanup, and sets all-locales to be just es-ES and pt-BR, until we can talk about more languages. Also addressed the other review comments. Pushed this (without all-locales part) to https://tbpl.mozilla.org/?tree=Try&rev=6f3e9ba8a04d
Comment on attachment 686787 [details] [diff] [review] removing un-used locales/en-US content, address review comments Review of attachment 686787 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but I'd like to understand the rationale for keeping only 2 locales. That looks pretty bad from a community standpoint. Why not add all the locales that we support already in Gaia? ::: b2g/locales/en-US/chrome/aboutCertError.dtd @@ -34,5 @@ > -you know there's a good reason why this site doesn't use trusted identification."> > -<!ENTITY certerror.addTemporaryException.label "Visit site"> > -<!ENTITY certerror.addPermanentException.label "Add permanent exception"> > - > -<!ENTITY certerror.technical.heading "Technical Details"> We don't have any certerror UI yet, but I'm not sure we want to get rid of this one.
Attachment #686787 - Flags: review?(fabrice) → review+
Thanks for the review. Re your comments: the locales on git are not really our official locale list, that's just es and pt-BR, we're wrapping up actually generating builds for that outside of what's in gaia's git repo. Those there are just for developer testing really. For certerror UI, we should use the right strings once we get there, if the ones we copied from mobile back when make sense, who knows. https://hg.mozilla.org/integration/mozilla-inbound/rev/b88467155adc
No longer depends on: 812784
No longer blocks: 812830
Duplicate of this bug: 812830
Filed bug 817197 to investiate packaging follow-ups, and bug 817198 for releng automation follow-up.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
For posterity, I asked Axel about how to use this new code in the context of b2g multilocale builds: 15:31 <@Pike> bhearsum: you should be able to go through the list of locales on b2g/locales/all-locales, and make merge-% ; make chrome-% And to verify we're doing things correctly: 15:50 <@Pike> bhearsum: there should be es-ES.jar and es-ES.manifest in dist/bin/chrome, and pt-BR.*
Uplifted to aurora and beta, I didn't do the merge to b2g-18. Fwiw, I need them on beta as that's where the localizers find their strings, and for l10n of b2g, those should really be the same. https://hg.mozilla.org/releases/mozilla-aurora/rev/91bb104beb36 https://hg.mozilla.org/releases/mozilla-beta/rev/d1c7dfa26a77
Pushed this to b2g18. It required fixing a minor merge conflict in b2g/locales/Makefile.in around the libs-% target. Also, forgot to run 'hg addremove' before pushing, so this ended up in two revs instead of one: https://hg.mozilla.org/releases/mozilla-b2g18/rev/248e248d95a3 https://hg.mozilla.org/releases/mozilla-b2g18/rev/474136c8136a
(FYI, the plan is to do ongoing m-b -> b2g18 merges, so unless it's urgent, you don't need to push it to both individually)
You need to log in before you can comment on or make changes to this bug.