Move official branding into browser/branding/official

RESOLVED FIXED in Firefox 6

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
Firefox 6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 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

7 years ago
Attachment #440715 - Flags: review?(l10n)

Comment 1

7 years ago
This looks like you're effectively removing the official branding from l10n. Is that intended?
(Assignee)

Comment 2

7 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

7 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

7 years ago
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 ?
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

7 years ago
Attachment #440722 - Flags: review?(shaver)

Comment 5

7 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

7 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

7 years ago
Attachment #440722 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

7 years ago
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.
Attachment #440722 - Attachment is obsolete: true
Attachment #442060 - Flags: review?(l10n)
(Assignee)

Updated

7 years ago
Attachment #442060 - Flags: review?(gavin.sharp)

Comment 8

7 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

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

Updated

6 years ago
Duplicate of this bug: 646261
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

6 years ago
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?!
(Assignee)

Comment 14

6 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

6 years ago
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.
(Assignee)

Comment 17

6 years ago
Created attachment 525639 [details] [diff] [review]
Refreshed patch

Refreshed to fit context changes.
Attachment #442060 - Attachment is obsolete: true
Attachment #525639 - Flags: review+
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/mozilla-central/rev/afc62991f24c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
It looks like compare-locales is broken. After renaming the files to  browser/branding/official they are reported to be removed.
(Assignee)

Comment 20

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

Comment 21

6 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

6 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.
(Assignee)

Updated

6 years ago
Depends on: 649938
You need to log in before you can comment on or make changes to this bug.