Closed
Bug 796051
Opened 12 years ago
Closed 12 years ago
Generate make target to package needed l10n files in gecko
Categories
(Firefox OS Graveyard :: GonkIntegration, defect, P1)
Firefox OS Graveyard
GonkIntegration
Tracking
(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed, b2g18 fixed)
People
(Reporter: Pike, Assigned: Pike)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
Updated•12 years ago
|
Priority: -- → P1
Comment 2•12 years ago
|
||
Who can do this work?
Assignee | ||
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
I've got a work-in-progress locally now, even though I'm hitting a few snags still. Taking.
Assignee: nobody → l10n
Assignee | ||
Comment 5•12 years ago
|
||
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.
Attachment #679088 -
Flags: review?(margaret.leibovic)
Attachment #679088 -
Flags: feedback?(mh+mozilla)
Assignee | ||
Comment 6•12 years ago
|
||
FYI, this went through try, https://tbpl.mozilla.org/?tree=Try&rev=1ddc9866ec77
Comment 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
Margaret, can you get to the review here? It'd be good to get the starting points to start doing automation work.
Comment 9•12 years ago
|
||
(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 10•12 years ago
|
||
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"?
Attachment #679088 -
Flags: review?(margaret.leibovic)
Attachment #679088 -
Flags: review?(jones.chris.g)
Attachment #679088 -
Flags: feedback+
Assignee | ||
Comment 11•12 years ago
|
||
(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+
Comment 13•12 years ago
|
||
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)
Assignee | ||
Comment 14•12 years ago
|
||
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?
Flags: needinfo?(jones.chris.g)
Updated•12 years ago
|
Flags: needinfo?(fabrice)
Updated•12 years ago
|
Flags: needinfo?(jones.chris.g)
Comment 15•12 years ago
|
||
(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
Flags: needinfo?(fabrice)
Assignee | ||
Comment 16•12 years ago
|
||
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
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
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 20•12 years ago
|
||
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-
Assignee | ||
Comment 21•12 years ago
|
||
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
Attachment #686328 -
Attachment is obsolete: true
Attachment #686334 -
Attachment is obsolete: true
Attachment #686787 -
Flags: review?(fabrice)
Comment 22•12 years ago
|
||
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+
Assignee | ||
Comment 23•12 years ago
|
||
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
Assignee | ||
Comment 25•12 years ago
|
||
Filed bug 817197 to investiate packaging follow-ups, and bug 817198 for releng automation follow-up.
Comment 26•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 27•12 years ago
|
||
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.*
Assignee | ||
Comment 28•12 years ago
|
||
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
Comment 29•12 years ago
|
||
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
Comment 30•12 years ago
|
||
(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)
Whiteboard: [status-b2g18:fixed]
Updated•12 years ago
|
status-b2g18:
--- → fixed
Whiteboard: [status-b2g18:fixed]
You need to log in
before you can comment on or make changes to this bug.
Description
•