504953 - Clean up firefox branding in the build system

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Justin Dolske [:Dolske]]

The organization of the branding stuff irks me every time I touch it... The Firefox (release) bits are in /other-licenses/branding/firefox, and the generic (pre-release) branding is in /browser/branding/unofficial, but the nightly (Minefield) branding is scattered all over. This leads to a number of special cases in the build for nightly vs release/prerelease. And I'm always scared that make some change that works for nightly builds but will explode in release builds.

So, I've reorganized things to make this simpler. There should be no differences in the bits users get; this is all just build-side changes.

* All the scattered nightly bits move into /browser/branding/nightly (which is currently emptys sans 1 file). This tree now becomes identical (in layout) to /other-licenses/branding/firefox and /browser/branding/unofficial.

* MOZ_BRANDING_DIRECTORY is now always set. It defaults to /browser/branding/nightly. This also removes the need for the OFFICIAL_BRANDING and MOZ_USE_GENERIC_BRANDING hacks.

* Simplify $MOZ_BRANDING_DIRECTORY/Makefile.in. All it really does is copy files into $dist/branding/. There were a few cases of files being renamed as they're copied, I've moved that into browser/app/Makefile.in (so it's done in one place instead of each branding flavor).

* Fixed a few file permissions that didn't need to be executable (their different color in my shell's ls output was irritating me).

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Builds on OSX, need to do some more testing on Windows/Linux.

* Gavin says old-homepage-default.properties is dead, so I've removed that too.
* "app.ico" doesn't seem to actually be used, so we no longer do the "cp firefox.ico branding/app.ico". Looks like the relevant code for Windows/OS2 just uses firefox.ico anyway.

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 389230 [details] [diff] [review]
Patch v.1

Clobber builds on Windows (w/ official branding) and Mac (nightly) seem to be ok, so I'll go for review...

Comment 3

[Benjamin Smedberg]

Comment on attachment 389230 [details] [diff] [review]
Patch v.1

I'm so happy! This is the way it was always supposed to be.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

Please remove old-homepage-default.properties is dead, please also remove it from browser/installer/{windows,unix}/packages-static too.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

* Removes old-homepage-default.properties from packages-static
* Fixes browser/app/Makefile.in to work on Linux (remove a bogus nsinstall)

Comment 6

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/af248748c7ea

Comment 7

[Axel Hecht [:Pike]]

This breaks l10n in weird ways.

Why are unofficial and nightly branding exposed to l10n now, and why without telling me?

Please revert the change to unofficial/locales/jar.mn, and make a similar hack to nightly/locales/jar.mn.

Comment 8

[Justin Dolske [:Dolske]]

Can you define "breaks l10n in weird ways"?

Why do you think nightlies/unofficial builds should be different than release versions? That's generally a bad idea.

Comment 9

[Axel Hecht [:Pike]]

You've added new modules to l10n without telling the l10n infrastructure. So all build infrastructure tells the localizers they're fine, just if they try to create an ad-hoc build, it'll fail over the missing branding strings.

We could properly add that new branding modules, but there was a conscious previous decision to not add unofficial branding and nightly branding to the localizable files. It's just adding noise and complexity in particular for the unofficial branding. Note that there is a lingering discussion on whether official branding should be localizable, too.

Reversing the decision to exclude modules without talking to the people involved is bad.

Comment 10

[Mike Connor [:mconnor]]

What was the reasoning behind that decision?  We absolutely should be able to build unofficial versions in any locale... I can't imagine why we wouldn't want to ensure that was possible.

Comment 11

[Axel Hecht [:Pike]]

(In reply to comment #10)
> What was the reasoning behind that decision? 

In case that question is directed at me:

There is little value in localizing "Minefield", "Shiretoko", or "Namaroka", and there is a potential danger in getting those names wrong.

Thus the decision to not expose those strings to l10n.

> We absolutely should be able to
> build unofficial versions in any locale... I can't imagine why we wouldn't want
> to ensure that was possible.

Sure, that's http://hg.mozilla.org/mozilla-central/rev/9b33be091bf5, just using a different setup in jar.mn.

Original bug was bug 440431.

Comment 12

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

(In reply to comment #10)
> We absolutely should be able to build unofficial versions in any locale...

That doesn't require the branding to be localizable, though, if we're willing to live with the lack of brand transcription in a few cases. We've lived with it so far, and it doesn't seem unreasonable for unofficial/nightly branding.

The changes to jar.mn can be reverted to address the l10n issue without a significant impact on this patch in general, right? Seems like we should just do that, unless I'm missing something. We can file a followup on making the other branding sets localizable if that's really desired.

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Followup fix v.1

Returns nightly and unofficial branding to en-US only.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 392788 [details] [diff] [review]
Followup fix v.1

Thanks, r=me with one nit, make the comment in the nightly branding say Nightly instead of Unofficial?

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 392788 [details] [diff] [review]
Followup fix v.1

This is good to go.

Comment 16

[Justin Dolske [:Dolske]]

Pushed the followup fix: http://hg.mozilla.org/mozilla-central/rev/415d1cdd5d7f