561032 - Move official branding into browser/branding/official

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mike Hommey [:glandium]]

Attachment: 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]]

This looks like you're effectively removing the official branding from l10n. Is that intended?

Comment 2

[Mike Hommey [:glandium]]

(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]]

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]]

Attachment: 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]]

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]]

Comment on attachment 440722 [details] [diff] [review]
Patch v2

Turning this into an actual r-, see my previous comment.

Comment 7

[Mike Hommey [:glandium]]

Attachment: 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]]

Comment on attachment 442060 [details] [diff] [review]
Patch v2.1

sorry, breaking compare-locales is an r-.

Comment 9

[Axel Hecht [:Pike]]

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 11

[:Gavin Sharp [email: [e-mail]]]

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]]

And a timeframe. When do we want this to land? FF6? Later?

Comment 13

[:ehsan akhgari]

(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]]

(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]]

So I guess landing tomorrow would be good.

Comment 16

[:ehsan akhgari]

(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]]

Attachment: Refreshed patch

Refreshed to fit context changes.

Comment 18

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/afc62991f24c

Comment 19

[Pavel Franc - Mozilla.cz]

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]]

That's interesting. When I tried (but it was nearly a year ago), it worked fine.

Comment 21

[Mike Hommey [:glandium]]

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]]

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.