1190465 - Move default theme overrides into separate chrome.manifest for other non-firefox toolkit consumers too.

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Philip Chee]

Bug #1150417 moved theme overrides in toolkit into a separate chrome.manifest but only for Firefox (and spoons). comm-central apps like Thunderbird and SeaMonkey will benefit from this too.

Comment 1

[Philip Chee]

Attachment: Proposed fix.

Comment 2

[:Gijs (he/him)]

Does this actually work? Is that manifest created, packaged and read in all those apps?

Comment 3

[Philip Chee]

Kruitbosch from comment #2)
> Does this actually work? Is that manifest created, packaged and read in all
> those apps?
Only tested in Win32 SeaMonkey so far.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e04203fa56cb(In reply to :Gijs

Comment 4

[Philip Chee]

Attachment: Proposed fix. Second attempt

try-server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99f6fc21ab19

Comment 5

[:Gijs (he/him)]

(In reply to Philip Chee from comment #4)
> Created attachment 8643157 [details] [diff] [review]
> Proposed fix. Second attempt
> 
> try-server:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=99f6fc21ab19

Nowhere in the patch you attached seems to actually set CONFIG['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES'] ?

Comment 6

[Philip Chee]

Comment on attachment 8643157 [details] [diff] [review]
Proposed fix. Second attempt

(In reply to :Gijs Kruitbosch from comment #5)

> Nowhere in the patch you attached seems to actually set
> CONFIG['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES'] ?

That's in Attachment 8643539 [details] [diff] in Bug 1022354 (comm-central)
https://bug1022354.bmoattachments.org/attachment.cgi?id=8643539

I decided that rather than hard coding applications into this patch, comm-central apps could "opt-in" as needed. If you don't opt-in this is effectively a NOOP (e.g. b2g, android, etc)

By the way does the file have to be called "chrome.jar" or could it be anything.jar?

-----------------------------------
Also in toolkit/themes/windows/global/jar.mn I think the #ifdef XP_WIN can be removed since we don't support OS/2 any more. Shall I file a new bug for this?

Comment 7

[:Gijs (he/him)]

(In reply to Philip Chee from comment #6)
> Comment on attachment 8643157 [details] [diff] [review]
> Proposed fix. Second attempt
> 
> (In reply to :Gijs Kruitbosch from comment #5)
> 
> > Nowhere in the patch you attached seems to actually set
> > CONFIG['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES'] ?
> 
> That's in Attachment 8643539 [details] [diff] in Bug 1022354 (comm-central)
> https://bug1022354.bmoattachments.org/attachment.cgi?id=8643539
> 
> I decided that rather than hard coding applications into this patch,
> comm-central apps could "opt-in" as needed. If you don't opt-in this is
> effectively a NOOP (e.g. b2g, android, etc)

OK.

> By the way does the file have to be called "chrome.jar" or could it be
> anything.jar?

We read the chrome.manifest file. The jar generating code, for reasons beyond my investigation and therefore understanding, creates manifests with the same filename and a different extension. So no, in order to get a chrome.manifest file, we pretend to stick it all into chrome.jar (where "all" is effectively "nothing").

> -----------------------------------
> Also in toolkit/themes/windows/global/jar.mn I think the #ifdef XP_WIN can
> be removed since we don't support OS/2 any more. Shall I file a new bug for
> this?

New bug please.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8643157 [details] [diff] [review]
Proposed fix. Second attempt

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

rs=me

::: toolkit/themes/moz.build
@@ +24,5 @@
>      elif CONFIG['MOZ_THEME_FASTSTRIPE']:
>          DIRS += ['faststripe/global']
>  
> +if CONFIG['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES']:
> +    DEFINES['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES'] = True 

nit: trailing whitespace

Comment 9

[[e-mail]]

(In reply to Gijs Kruitbosch from comment #8)
> (From update of attachment 8643157 [details] [diff] [review])
> > +if CONFIG['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES']:
> > +    DEFINES['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES'] = True 
> 
> nit: trailing whitespace

Actually why do you need this change at all? This moz.build doesn't actually do anything except select the appropriate platform directory.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3df50e80ed

Comment 11

[Carsten Book [:Tomcat]]

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