Closed Bug 823218 (metro-branding) Opened 7 years ago Closed 7 years ago

Use desktop's branding data rather than copying it over

Categories

(Firefox for Metro Graveyard :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Blocks: elm-merge
No longer blocks: metro-l10n
Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #694412 - Flags: review?(mbrubeck)
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 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.
Attachment #694412 - Flags: review?(mh+mozilla)
Attachment #694412 - Flags: review?(mbrubeck)
Attachment #694412 - Flags: review+
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
Attachment #694412 - Flags: review?(mh+mozilla) → review-
Attached patch patch v.2Splinter Review
(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.
Attachment #694412 - Attachment is obsolete: true
(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 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.
Attachment #697669 - Flags: review?(mh+mozilla)
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 ?
Attachment #697669 - Flags: review?(mh+mozilla) → review+
Attached patch additions v.1 (obsolete) — Splinter Review
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.
Attachment #698281 - Flags: review?(mh+mozilla)
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.
Attached patch additions v.1Splinter Review
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.
Attachment #698281 - Attachment is obsolete: true
Attachment #698281 - Flags: review?(mh+mozilla)
Attachment #698284 - Flags: review?(mh+mozilla)
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.
Attachment #698284 - Flags: review?(mh+mozilla) → review+
Attached patch rollup patch (obsolete) — Splinter Review
Comment on attachment 698630 [details] [diff] [review]
rollup patch

bah, forgot the config.mk include.
Attachment #698630 - Attachment is obsolete: true
Attached patch rollup patchSplinter Review
This should have everything.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [completed-elm]
Alias: metro-branding
https://hg.mozilla.org/mozilla-central/rev/de6aed05a69b
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.