Closed Bug 1190465 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Themes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1022354
Attached patch Proposed fix. (obsolete) — Splinter Review
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Does this actually work? Is that manifest created, packaged and read in all those apps?
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
(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 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?
Attachment #8643157 - Flags: review?(gijskruitbosch+bugs)
(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 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
Attachment #8643157 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/ae3df50e80ed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.