Closed
Bug 561032
Opened 12 years ago
Closed 11 years ago
Move official branding into browser/branding/official
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 6
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 3 obsolete files)
10.51 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
Since now the Firefox branding is tri-licensed, there is no reason to keep it in other-licenses. I think it is time to move it where it belongs, and that should be browser/branding/official.
Assignee | ||
Updated•12 years ago
|
Attachment #440715 -
Attachment is patch: true
Attachment #440715 -
Attachment mime type: application/octet-stream → text/plain
Attachment #440715 -
Flags: review?(shaver)
Assignee | ||
Updated•12 years ago
|
Attachment #440715 -
Flags: review?(l10n)
Comment 1•12 years ago
|
||
This looks like you're effectively removing the official branding from l10n. Is that intended?
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to comment #1) > This looks like you're effectively removing the official branding from l10n. Is > that intended? That's why I asked review from you, too. In both l10n files, it looks like the browser directory is handled. Wouldn't the browser/branding/official directory fall into this ? In that case, the only worry would be the transition for translators, I really don't know how this works.
Comment 3•12 years ago
|
||
For the l10n infrastructure to work on these files as "browser", the branding files had to be in browser/locales/en-US/branding/official or so, but beneath browser/locales/en-US. I'm not sure if the branding dir magic would work with that.
Assignee | ||
Comment 4•12 years ago
|
||
I take it that this would work, then. Would there be a problem with transition for translations ? Would it be worth, for example, having something handling the move in the l10n scripts ?
Assignee: nobody → mh+mozilla
Attachment #440715 -
Attachment is obsolete: true
Attachment #440722 -
Flags: review?(l10n)
Attachment #440715 -
Flags: review?(shaver)
Attachment #440715 -
Flags: review?(l10n)
Assignee | ||
Updated•12 years ago
|
Attachment #440722 -
Flags: review?(shaver)
Comment 5•12 years ago
|
||
Looking at the code in http://mxr.mozilla.org/mozilla-central/source/config/config.mk#741, I expect that the relativesrcdir needs to be "browser", and then you need to fiddle with the jar.mn to actually start picking up for branding/official. You'd really want to try to do some repacks locally, I'd suggest trying es-AR or fr from http://hg.mozilla.org/l10n-central/. You'd do your normal build, but with a --l10n-base-dir=path/to/parent/of/fr argument to configure. Then you cd to browser/locales and do make installers-fr That should do the magic and trigger the code at http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#190, which we use for repacks. The other (not so common) path is to use --enable-ui-locale as an argument to configure, I guess both ways need to keep working.
Attachment #440722 -
Flags: review?(shaver) → review?(gavin.sharp)
Comment 6•12 years ago
|
||
Comment on attachment 440722 [details] [diff] [review] Patch v2 Turning this into an actual r-, see my previous comment.
Attachment #440722 -
Flags: review?(l10n) → review-
Assignee | ||
Updated•12 years ago
|
Attachment #440722 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 7•12 years ago
|
||
> Looking at the code in
> http://mxr.mozilla.org/mozilla-central/source/config/config.mk#741, I expect
> that the relativesrcdir needs to be "browser", and then you need to fiddle with
> the jar.mn to actually start picking up for branding/official.
Actually, there is no need for that. Everything works fine with the patch as it was.
The only problems were with compare-locales, that use l10n and filter.py, with which the output is better if other-licenses/branding/firefox is left there, so that the output looks like:
fr
browser/branding/official
brand.dtd
// add and localize this file
brand.properties
// add and localize this file
other-licenses/branding/firefox
brand.dtd
// remove this file
brand.properties
// remove this file
As for --enable-ui-locale and make -C browser/locales installers-fr, both worked properly.
Attachment #440722 -
Attachment is obsolete: true
Attachment #442060 -
Flags: review?(l10n)
Assignee | ||
Updated•12 years ago
|
Attachment #442060 -
Flags: review?(gavin.sharp)
Comment 8•12 years ago
|
||
Comment on attachment 442060 [details] [diff] [review] Patch v2.1 sorry, breaking compare-locales is an r-.
Attachment #442060 -
Flags: review?(l10n) → review-
Comment 9•12 years ago
|
||
Comment on attachment 442060 [details] [diff] [review] Patch v2.1 Misunderstood the previous comment, got all confused by my own code and modules on top of each other. Seems to work in practice, so r=me.
Attachment #442060 -
Flags: review- → review+
Comment 11•11 years ago
|
||
Comment on attachment 442060 [details] [diff] [review] Patch v2.1 I suppose we want a followup on removing the other-licenses/branding/firefox references at some point?
Attachment #442060 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 12•11 years ago
|
||
And a timeframe. When do we want this to land? FF6? Later?
Comment 13•11 years ago
|
||
(In reply to comment #12) > And a timeframe. When do we want this to land? FF6? Later? Any reason not to land this right now?!
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to comment #13) > (In reply to comment #12) > > And a timeframe. When do we want this to land? FF6? Later? > > Any reason not to land this right now?! Because I'm not confident enough about the impact on the release process. Landing at the beginning of the 6-week train would help ensuring everything is fine before merging to aurora.
Assignee | ||
Comment 15•11 years ago
|
||
So I guess landing tomorrow would be good.
Comment 16•11 years ago
|
||
(In reply to comment #15) > So I guess landing tomorrow would be good. You can land it any time now, aurora has been branched.
Assignee | ||
Comment 17•11 years ago
|
||
Refreshed to fit context changes.
Attachment #442060 -
Attachment is obsolete: true
Attachment #525639 -
Flags: review+
Assignee | ||
Comment 18•11 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/afc62991f24c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Comment 19•11 years ago
|
||
It looks like compare-locales is broken. After renaming the files to browser/branding/official they are reported to be removed.
Assignee | ||
Comment 20•11 years ago
|
||
That's interesting. When I tried (but it was nearly a year ago), it worked fine.
Assignee | ||
Comment 21•11 years ago
|
||
So, what I did try last year was with a current l10n, and that still works: $ ./compare-locales/scripts/compare-locales ./mozilla-central/browser/locales/l10n.ini ./l10n/ zh-TW zh-TW (snip) browser/branding/official brand.dtd // add and localize this file brand.properties // add and localize this file (snip) other-licenses/branding/firefox brand.dtd // remove this file brand.properties // remove this file (snip) What doesn't work correctly, though, and that I apparently didn't test, is after updating the locale: $ ./compare-locales/scripts/compare-locales ~/hg/mozilla-central/browser/locales/l10n.ini ./l10n/ fr fr browser/branding/official brand.dtd // remove this file brand.properties // remove this file browser/branding/official/brand.properties +homePageImport +homePageMigrationDescription +homePageMigrationPageTitle +homePageSingleStartMain It's interesting to see that on top of saying to remove the file, it *still* presents the new strings for brand.properties. That does sound like a bug in compare-locales.
Assignee | ||
Comment 22•11 years ago
|
||
So, the root of the problem is that compare-locales can't distinguish, in the l10n directory, between something that originally was under browser/locales and something that originally was under browser/branding/official/locales.
You need to log in
before you can comment on or make changes to this bug.
Description
•