237725 - Use staging area for branded resources

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Currently the makefile in other-licenses/branding/firefox copies files into the
objdir (which is the same as the srcdir in a srcdir build). This is risky
because if someone checks in from a srcdir build, they will check in official
branding. 

The real solution here is to do the following:

Makefile in other-licenses/branding/firefox creates stage dir, e.g. dist/branding/ 

Makefile in browser/app (etc) checks for dist/branding. if exists, then it
continues. if not, it creates dist/branding and copies in generic art. 

resources (e.g. windows resource scripts like splash.rc) are adapted to look in
$(DIST)/branding for images.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Marking P2 as someone is likely to screw up and check in something they shouldn't. 

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

scott says he's going to take a stab at this.

as a reminder here's what we should have

1 - Branded Browser Art:
other-licenses/branding/firefox/Watermrk.bmp
other-licenses/branding/firefox/Header.bmp
other-licenses/branding/firefox/firefox.ico 

2 - Branded Mail Art:
other-licenses/branding/thunderbird/Watermrk.bmp
other-licenses/branding/thunderbird/Header.bmp
other-licenses/branding/thunderbird/thunderbird.ico 

3 - Generic Browser Installer Art:
browser/installer/windows/Watermrk.bmp
browser/installer/windows/Header.bmp
browser/installer/windows/app.ico <-- replace "browser.ico" & associated rsrcs

4 - Generic Mail Installer Art:
mail/installer/windows/Watermrk.bmp
mail/installer/windows/Header.bmp
mail/installer/windows/app.ico <-- replace "mail.ico" & associated resources

If --enable-official-branding is set, thus defining MOZ_USE_OFFICIAL_BRANDING,
depending on whether or not MOZ_PHOENIX or MOZ_THUNDERBIRD is set, the contents
of 1 or 2 is copied into $(DIST)/branding

When the build reaches toolkit/mozapps/installer/wizard/windows/setuprsc/ or
browse/app or mail/app, the Makefiles in those locations should check to see if
MOZ_USE_OFFICIAL_BRANDING is set. If it is NOT set then the makefiles should
copy the generic art files to $(DIST)/branding.

*** Any code or resource script (.rc) that expects a branding file to exist in
the srcdir or parallel objdir must now know to look in $(DIST)/branding instead.
*** 

This last part is probably the trickiest but I think it's possible.

Comment 3

[Scott MacGregor]

This last part might be tricky. I don't think a resource script (.rc file) has
any notion of the location of the $DIST directory. I see some RC files which use
relative paths to include some files. But that wont' work for OBJDIR
builds....More news later....

Comment 4

[Scott MacGregor]

Attachment: first cut at a fix

Several things need to happen next:

1) I haven't tested the installer changes yet
2) I'll need help testing OSX and Linux builds to make sure the changes work
there.
3) This checkin will effect OS/2 as well. We'll need Mike to be in the loop for
the corresponding OS/2 changes.

Comment 5

[Scott MacGregor]

this patch has nasty conflicts with Brian's check in to "Make use of VPATH so
that xpm files will be first looked for in the objdir." Brian, I couldn't find a
bug checkin associatedc with that change. Can you elaborate more on what that
check in was trying to do? Every line I changed for unix xpm files is
conflicting with that checkin :)

Comment 6

[Scott MacGregor]

I'm having a problem getting this to work with the installer. The installer
directories in:

mozilla/browser/installer, mozilla/browser/installer/windows

never get the export command run on them because they aren't linked up to any
top level directories in the Makefile system. i.e. we run export on
browser\Makefile.in but it doesn't know anything about the installer sub
directory so we never enter it.

This prevents us from copying the branding for the installer out to
dist\branding during the export phase before we actually build
mozilla\toolkit\installer.

Hmmm...

Comment 7

[Scott MacGregor]

Something weird is going on with how we build mozilla/toolkit/mozapps/installer
and mozapps/xpinstall.

These diretories get built before we do a full export on the tree. 

1) If I run make -f client.mk export, the installer icons from
browser\installer\windows show up in dist\branding. Ditto for the app icons in
browser\app

2) If I remove dist and then do make -f client.mk build_all, we start compiling
toolkit/mozapps/installer before we ever enter the export phase for
mozilla/browser. 

I suspect this is because of the following line in mozilla\Makefile.in:

http://lxr.mozilla.org/mozilla/source/Makefile.in#288

Comment 8

[Scott MacGregor]

Just to demonstrate the problem, to work around it I had to do something nasty
like this:

Index: Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/Makefile.in,v
retrieving revision 1.248
diff -u -w -r1.248 Makefile.in
--- Makefile.in 5 Apr 2004 18:10:59 -0000       1.248
+++ Makefile.in 15 Apr 2004 21:09:10 -0000
@@ -287,7 +287,11 @@

 # mozapps installer needs to be built after xpinstall
 ifdef MOZ_XUL_APP
+
 ifdef MOZ_INSTALLER
+ifdef MOZ_PHOENIX
+tier_50_dirs     +=  browser/installer/windows browser/app
+endif
 tier_50_dirs     +=  toolkit/mozapps/installer
 endif
 endif

This ensured that the icons were exported from browser/app and the installer
directory BEFORE we tried to build mozapps/installer.

Brian, what do you think about making a duplicate copy of the icon in
mozilla\browser\app\app.ico and putting it in browser\installer windows.

Then the mozilla\Makefile.in change would look like:

+ifdef MOZ_PHOENIX
+tier_50_dirs     +=  browser/installer/windows
+endif
 tier_50_dirs     +=  toolkit/mozapps/installer
 endif

I'm open to other suggestions to.

Comment 9

[Scott MacGregor]

Attachment: updated fix

Comment 10

[Scott MacGregor]

Comment on attachment 146252 [details] [diff] [review]
updated fix

I think this patch is ready for r/sr. Brian showed me the trick to get around
my problems by moving mozilla/toolkit/mozapps/installer to get built after
mozilla/browser since nothing depends on it. This ensures that the icons from
browser\installer are copied out to dist\branding before we try to use them
when building the mozapps installer.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 146252 [details] [diff] [review]
updated fix

>--- browser/app/Makefile.in	12 Apr 2004 07:25:20 -0000	1.52
>+++ browser/app/Makefile.in	16 Apr 2004 05:40:22 -0000
> export:: brand.dtd.in
> 	$(PERL) $(topsrcdir)/config/preprocessor.pl $(DEFINES) $(ACDEFINES) $^ > brand.dtd
>+ifndef MOZ_USE_OFFICIAL_BRANDING
>+	$(NSINSTALL) -D $(DIST)/branding
>+ifeq ($(OS_ARCH),WINNT)
>+	cp $(srcdir)/firefox.ico   $(DIST)/branding/firefox.ico
>+	cp $(srcdir)/firefox.ico   $(DIST)/branding/app.ico
>+	cp $(srcdir)/document.ico  $(DIST)/branding/document.ico
>+endif
>+ifneq (,$(filter mac cocoa,$(MOZ_WIDGET_TOOLKIT)))
>+	cp $(srcdir)/macbuild/firefox.icns  $(DIST)/branding/firefox.icns
>+endif
>+ifneq (,$(filter gtk gtk2,$(MOZ_WIDGET_TOOLKIT)))
>+	cp $(srcdir)/mozicon16.xpm $(DIST)/branding/mozicon16.xpm
>+	cp $(srcdir)/mozicon50.xpm $(DIST)/branding/mozicon50.xpm
>+endif
>+ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
>+	cp $(srcdir)/default.xpm   $(DIST)/branding/default.xpm	
>+endif
>+endif

These should not be part of the brand.dtd.in rule or these commands will only
be run when brand.dtd.in changes (so on Windows, if the unofficial artwork
changed, the new files would not get copied).  I would make a new rule that
depends on the icon files.  Don't worry about ifdef'ing that part of it for
platforms.  Alternately you could make a new rule that depends on nothing and
the commands would always be executed.

I also noticed you have changes in all of the bmp files, make you you don't
accidentely check in the official artwork somehow.  sr=bryner with that fixed.

Comment 12

[Scott MacGregor]

Attachment: associated thunderbird changes

Comment 13

[Scott MacGregor]

of course it wasn't till after I checked this in that I realized this probably
effects the OS/2 builds. But I'm not sure how yet.

Comment 14

[Scott MacGregor]

One thing this patch didn't address was the about / credit images which get
packaged up into JAR files. I'm not sure how we want to solve this yet. 

We could put jar.mn files in other_licenses\branding\firefox|thunderbird
but then we need the ability to support #ifdefs in the jar.mn files in
browser\base\content so we don't override with the unbranded art.

Comment 15

[Scott MacGregor]

all of this has been checked into the 1.7 branch and the trunk. The last
remaining issue are the images that end up in JAR files (i.e. the about images). 

I see a couple ways to do this:

For all of them:
* Add a jar.mn file in other\licenses\branding\fx|tb. 

Then either:

1) run browser|mail\base\jar.mn through the xul pp and only put the content
images into the jar file if official branding is turned off

2) create a new directory under browser|mail\content which is only entered if
official branding is turned off, this directory has a jar.mn file that packages
up the generic about dialog artwork.

3) I guess I don't have an idea for option 3 yet... :)

Comment 16

[Robert Parenton]

I'm not too familiar with the entire build system, but couldn't we do a final
JAR packaging that would overwrite the generic artwork with the official branded
artwork after the normal JAR packaging if the official artwork flag is set?

Comment 17

[Phil Ringnalda (:philor)]

Scott, is there actually anything left to do here, considering how much branding has changed since 2004?

Comment 18

[Scott MacGregor]

Looks like the patches landed a long time ago and the remaining work that kept this bug open doesn't apply anymore.

Comment 19

[Phil Ringnalda (:philor)]

Comment on attachment 146252 [details] [diff] [review]
updated fix

Clearing obsolete review request.