Created attachment 440715 [details] [diff] [review] Patch 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.
This looks like you're effectively removing the official branding from l10n. Is that intended?
(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.
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.
Created attachment 440722 [details] [diff] [review] Patch v2 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 ?
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.
Comment on attachment 440722 [details] [diff] [review] Patch v2 Turning this into an actual r-, see my previous comment.
Created attachment 442060 [details] [diff] [review] Patch v2.1 > 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.
Comment on attachment 442060 [details] [diff] [review] Patch v2.1 sorry, breaking compare-locales is an r-.
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.
*** Bug 646261 has been marked as a duplicate of this bug. ***
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?
And a timeframe. When do we want this to land? FF6? Later?
(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?!
(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.
So I guess landing tomorrow would be good.
(In reply to comment #15) > So I guess landing tomorrow would be good. You can land it any time now, aurora has been branched.
Created attachment 525639 [details] [diff] [review] Refreshed patch Refreshed to fit context changes.
http://hg.mozilla.org/mozilla-central/rev/afc62991f24c
It looks like compare-locales is broken. After renaming the files to browser/branding/official they are reported to be removed.
That's interesting. When I tried (but it was nearly a year ago), it worked fine.
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.
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.
or
