Last Comment Bug 561032 - Move official branding into browser/branding/official
: Move official branding into browser/branding/official
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
: 646261 (view as bug list)
Depends on: 649938
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-22 00:56 PDT by Mike Hommey [:glandium]
Modified: 2011-04-14 01:38 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (11.50 KB, patch)
2010-04-22 00:56 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Patch v2 (11.50 KB, patch)
2010-04-22 02:01 PDT, Mike Hommey [:glandium]
l10n: review-
Details | Diff | Splinter Review
Patch v2.1 (10.82 KB, patch)
2010-04-28 05:12 PDT, Mike Hommey [:glandium]
l10n: review+
gavin.sharp: review+
Details | Diff | Splinter Review
Refreshed patch (10.51 KB, patch)
2011-04-13 00:56 PDT, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2010-04-22 00:56:34 PDT
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.
Comment 1 Axel Hecht [:Pike] 2010-04-22 01:23:57 PDT
This looks like you're effectively removing the official branding from l10n. Is that intended?
Comment 2 Mike Hommey [:glandium] 2010-04-22 01:34:27 PDT
(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 Axel Hecht [:Pike] 2010-04-22 01:42:19 PDT
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.
Comment 4 Mike Hommey [:glandium] 2010-04-22 02:01:40 PDT
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 ?
Comment 5 Axel Hecht [:Pike] 2010-04-22 02:49:44 PDT
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 6 Axel Hecht [:Pike] 2010-04-27 16:22:11 PDT
Comment on attachment 440722 [details] [diff] [review]
Patch v2

Turning this into an actual r-, see my previous comment.
Comment 7 Mike Hommey [:glandium] 2010-04-28 05:12:12 PDT
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 8 Axel Hecht [:Pike] 2010-04-28 05:16:55 PDT
Comment on attachment 442060 [details] [diff] [review]
Patch v2.1

sorry, breaking compare-locales is an r-.
Comment 9 Axel Hecht [:Pike] 2010-04-28 05:42:19 PDT
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.
Comment 10 tymerkaev 2011-03-29 19:25:24 PDT
*** Bug 646261 has been marked as a duplicate of this bug. ***
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-10 14:59:06 PDT
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?
Comment 12 Mike Hommey [:glandium] 2011-04-10 23:45:22 PDT
And a timeframe. When do we want this to land? FF6? Later?
Comment 13 :Ehsan Akhgari 2011-04-11 14:29:27 PDT
(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?!
Comment 14 Mike Hommey [:glandium] 2011-04-12 00:49:54 PDT
(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.
Comment 15 Mike Hommey [:glandium] 2011-04-12 00:50:25 PDT
So I guess landing tomorrow would be good.
Comment 16 :Ehsan Akhgari 2011-04-12 15:43:53 PDT
(In reply to comment #15)
> So I guess landing tomorrow would be good.

You can land it any time now, aurora has been branched.
Comment 17 Mike Hommey [:glandium] 2011-04-13 00:56:12 PDT
Created attachment 525639 [details] [diff] [review]
Refreshed patch

Refreshed to fit context changes.
Comment 18 Mike Hommey [:glandium] 2011-04-13 01:13:44 PDT
http://hg.mozilla.org/mozilla-central/rev/afc62991f24c
Comment 19 Pavel Franc - Mozilla.cz 2011-04-13 14:22:44 PDT
It looks like compare-locales is broken. After renaming the files to  browser/branding/official they are reported to be removed.
Comment 20 Mike Hommey [:glandium] 2011-04-13 14:37:33 PDT
That's interesting. When I tried (but it was nearly a year ago), it worked fine.
Comment 21 Mike Hommey [:glandium] 2011-04-14 00:23:58 PDT
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.
Comment 22 Mike Hommey [:glandium] 2011-04-14 01:10:01 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.