823218 - (metro-branding) Use desktop's branding data rather than copying it over

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Jim Mathies [:jimm]]

I'm not sure if this is going to be possible, but I'll give it a shot. We duplicate a lot of image data here so it would be good if we could share the same resources.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 694412 [details] [diff] [review]
patch v.1

The parts of this patch that sit in browser/metro/* are elm specific, browser/branding/* changes I'll land on both elm and mc.

Comment 3

[Jim Mathies [:jimm]]

Note to self - DIST_SUBDIR is set to 'browser' on elm but is empty on mc, so these changes will have to wait for bug 755724.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 694412 [details] [diff] [review]
patch v.1

Looks good to me, but I'd like a build system expert to sanity check the Makefile bits.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 694412 [details] [diff] [review]
patch v.1

Review of attachment 694412 [details] [diff] [review]:
-----------------------------------------------------------------

You may want to split this patch in two parts: one landable on m-c and the other for elm.

::: browser/branding/aurora/content/Makefile.in
@@ +15,5 @@
> +# resources needed for the metro tile interface
> +ifdef MOZ_METRO
> +ifeq ($(MOZ_WIDGET_TOOLKIT) $(DIST_SUBDIR),windows metro)
> +export::
> +	$(NSINSTALL) $(wildcard $(srcdir)/VisualElements*) $(DIST)/bin/tileresources

Please use the generic copy/install rules instead of doing that.
See http://hg.mozilla.org/mozilla-central/file/f5ed2691d901/config/rules.mk#l1539

Comment 6

[Jim Mathies [:jimm]]

Attachment: patch v.2

(In reply to Mike Hommey [:glandium] from comment #5)
> Please use the generic copy/install rules instead of doing that.
> See
> http://hg.mozilla.org/mozilla-central/file/f5ed2691d901/config/rules.mk#l1539

updated.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Mike Hommey [:glandium] from comment #5)
> You may want to split this patch in two parts: one landable on m-c and the
> other for elm.

Yeah I'll sort out these landings once metro-build lands on mc. I can't land the branding folder changes until DIST_SUBDIR is set right.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 697669 [details] [diff] [review]
patch v.2

Requesting review for landing this on elm, since I don't think I should carry over Matt's original r+ due to additional changes.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 697669 [details] [diff] [review]
patch v.2

Review of attachment 697669 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/branding/nightly/Makefile.in
@@ +71,5 @@
>  	cp $(addprefix $(srcdir)/, $(LINUX_BRANDING_FILES)) $(DIST)/branding/
>  	$(NSINSTALL) -D $(DIST)/install
>  endif
>  ifeq ($(OS_ARCH),OS2)
>  	cp $(addprefix $(srcdir)/, $(OS2_BRANDING_FILES)) $(DIST)/branding/

While you're here, could you convert these to INSTALL_TARGETS ?

Comment 10

[Jim Mathies [:jimm]]

Attachment: additions v.1

This is on top of patch v.2. There are a couple changes here, one I removed the ifdef MOZ_METRO from the content makefiles, since the DIST_SUBDIR check suffices. 

Also, in the root branding makefile for nightly + gtk there was an nsinstall directive to create an install directory for the export target. I left that in and converted the rest to INSTALL_TARGETS. Not sure if that was needed, there was no comment indicating why it was there.

Comment 11

[Jim Mathies [:jimm]]

Hmm, in looking at that in diff, I'll bet INSTALL_TARGETS += BRANDING needs the ifdef on windows as well. will test build to see.

Comment 12

[Jim Mathies [:jimm]]

Attachment: additions v.1

No issues with INSTALL_TARGETS when building metro, but I did need to add a config.mk include to the nightly branding makefile to get at DIST_SUBDIR.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 698284 [details] [diff] [review]
additions v.1

Review of attachment 698284 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/branding/nightly/Makefile.in
@@ +17,5 @@
>  
>  PREF_JS_EXPORTS = $(srcdir)/pref/firefox-branding.js
>  
> +# On Windows only do this step for browser, skip for metro.
> +$(warning $(MOZ_WIDGET_TOOLKIT) $(DIST_SUBDIR))

Leftovers from debugging?

@@ +68,3 @@
>  
> +BRANDING_DEST := $(DIST)/branding
> +INSTALL_TARGETS += BRANDING

Why aren't you modifying the equivalent browser/branding/*/Makefile.in files?

@@ +76,1 @@
>  	$(NSINSTALL) -D $(DIST)/install

I'm fairly sure this isn't any useful. Just remove it.

Comment 14

[Jim Mathies [:jimm]]

Attachment: rollup patch

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 698630 [details] [diff] [review]
rollup patch

bah, forgot the config.mk include.

Comment 16

[Jim Mathies [:jimm]]

Attachment: rollup patch

This should have everything.

Comment 17

[Jim Mathies [:jimm]]

https://hg.mozilla.org/projects/elm/rev/d9ba99053f9b

Comment 18

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.

Comment 19

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/de6aed05a69b