1150417 - Move default theme aero/XP overrides into separate chrome.manifest so it doesn't break custom themes

Status: VERIFIED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Alfred Kayser]

Attachment: TB_april1_2015.jpg

All custom themes (at least all mine) stopped working since the nightly releases of April 1st, 2015 for Firefox and Thunderbird.

(I hope this is not a April Fools issue).

See attachments.

Comment 1

[Alfred Kayser]

Attachment: aboutsupport_april1_2015.txt

This is an about:support dump (after the "Refresh").

Comment 2

[Alfred Kayser]

Note, there are no error messages of anything in the error console.

Comment 3

[Stefan [:stefanh]]

Maybe a fallout from bug 706103?

Comment 4

[Alfred Kayser]

Indeed this is caused by 706103.
That is a "ridiculous" change: 
In the chrome.manifest there are now many override's like this:
override chrome://global/skin/popup.css chrome://global/skin/popup-aero.css osversion>=6

For custom themes these override are a nightmare.
Custom themes are not allowed to do overrides, but now we have to include files such as popup-aero-css.css in our theme importing the normal popup.css just to be able to restore normal order again.

This impacts ALL customs themes enormously.

This needs to be reverted and another solution is to be found for this than using overrides in this way.

Comment 5

[Alfred Kayser]

Note, this also impacts Thunderbird in the same way!

Comment 6

[leichixian]

Depends on: 1150006

Comment 7

[leichixian]

(In reply to leichixian from comment #6)
> Depends on: 1150006

Opps... mistake the format
Depends on: https://bugzilla.mozilla.org/show_bug.cgi?id=1150006

Comment 8

[Dão Gottwald [:dao]]

(In reply to leichixian from comment #7)
> (In reply to leichixian from comment #6)
> > Depends on: 1150006
> 
> Opps... mistake the format
> Depends on: https://bugzilla.mozilla.org/show_bug.cgi?id=1150006

Well noticed, that bug removed all -aero.css files from /toolkit/themes/. I plan to do the same for /browser/themes/ soon.

Comment 9

[leichixian]

S this(In reply to Dão Gottwald [:dao] from comment #8)
> (In reply to leichixian from comment #7)
> > (In reply to leichixian from comment #6)
> > > Depends on: 1150006
> > 
> > Opps... mistake the format
> > Depends on: https://bugzilla.mozilla.org/show_bug.cgi?id=1150006
> 
> Well noticed, that bug removed all -aero.css files from /toolkit/themes/. I
> plan to do the same for /browser/themes/ soon.

This is why I said in bug 1150006 that you need to explain how to override files, or it would be a nightmare to 3rd party theme writer.

Comment 10

[:Gijs (out for now; he/him)]

Morphing this so it reflects how we intend to fix this. See also background in bug 706103 comment 11 through 17.

Comment 11

[:Gijs (out for now; he/him)]

Dão, do you have cycles to work on this now? We should definitely fix this before 40 moves to Aurora.

Comment 12

[Alfred Kayser]

Thanks! Can I do anything to help here?

Comment 13

[Alfred Kayser]

Attachment: fixit.bat

Dirty little batch files to copy all overridden files to -aero.png/.css files so that I can continue to develop/test my themes with the latest nightlies (for e.g. the great stuff of the readinglist).

Comment 14

[Pardal Freudenthal (:ShareBird)]

I suppose third-party themes will be able to use this approach too. Is it right? Then we can get rid of the hack registering a skin-only package...

Comment 15

[Dão Gottwald [:dao]]

Bug 1150843 and bug 1150867 should further unbreak third-party themes.

I'm going to try to work on this next week.

Comment 16

[leichixian]

Now the main features are okay, but Bug 1156308 describes something you may forgot to fix.

Comment 17

[:Gijs (out for now; he/him)]

[Tracking Requested - why for this release]:
addon-compat

(In reply to Dão Gottwald [:dao] from comment #15)
> Bug 1150843 and bug 1150867 should further unbreak third-party themes.
> 
> I'm going to try to work on this next week.

Did this get lost? Do you have time to work on this now that this is on aurora? If not, d'you mind if I take it?

Comment 19

[jen strickland :jenstrickland]

Tracking enabled for 40 and 41.

Comment 20

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #17)
> [Tracking Requested - why for this release]:
> addon-compat
> 
> (In reply to Dão Gottwald [:dao] from comment #15)
> > Bug 1150843 and bug 1150867 should further unbreak third-party themes.
> > 
> > I'm going to try to work on this next week.
> 
> Did this get lost? Do you have time to work on this now that this is on
> aurora?

I haven't forgotten about it but I don't have time to work on this at the moment.

> If not, d'you mind if I take it?

Not at all.

Comment 21

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: bz://1150417/Gijs

/r/9175 - Bug 1150417 - move overrides into theme-only manifest, r?dao,glandium

Pull down this commit:

hg pull -r 84e0b5f6a0a4254bc39996bc13066c6095e7879e https://reviewboard-hg.mozilla.org/gecko/

Comment 22

[:Gijs (out for now; he/him)]

Comment on attachment 8608676 [details]
MozReview Request: bz://1150417/Gijs

Dão, Mike, what do you think of this?

My main aim when writing this was to avoid splitting the override portion of the manifest from the rest of it (which I believe will help us avoid overlooking stuff when refactoring things). Let me know if you think that aim is mistaken.

The #ifdefs are kind of ugly, but I couldn't really think of a better way to do this - then again, perhaps a custom makefile invocation of the preprocessor and/or grep would let us avoid these and just magically extract only the overrides? Then again again, there are ifdefs in these jar.mn files that would make that tricky to get right, and we'd still need some ifdefs to avoid the overrides being duplicated into the main manifest files...

Comment 23

[:Gijs (out for now; he/him)]

(fwiw, I've only tested this on OS X, where it seems to work pretty much as expected)

Comment 24

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/9175/#review7895

::: browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.manifest.in:1
(Diff revision 1)
> +#define THEME_MANIFEST

I think you don't need this. That is, I think you can just trick the jar maker into making the chrome.manifest for the theme addon for you.
(trying)
well, that would require some adjustments to python/mozbuild/mozbuild/jar.py to make it work, but I think it would be more desirable than this.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8608676 [details]
MozReview Request: bz://1150417/Gijs

Looks good to me, but I'll defer to glandium.

Note that we don't need toolkit/themes/.../help/ for Firefox.

Comment 26

[:Gijs (out for now; he/him)]

(In reply to Mike Hommey [:glandium] from comment #24)
> https://reviewboard.mozilla.org/r/9175/#review7895
> 
> :::
> browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.
> manifest.in:1
> (Diff revision 1)
> > +#define THEME_MANIFEST
> 
> I think you don't need this. That is, I think you can just trick the jar
> maker into making the chrome.manifest for the theme addon for you.
> (trying)
> well, that would require some adjustments to python/mozbuild/mozbuild/jar.py
> to make it work, but I think it would be more desirable than this.

I am wary of people telling me to "just" hack some python utils that are in use all over the project for different purposes. Note that we really only want the overrides in this new manifest, not everything else. I don't know that it's easy to hack jar.py to do that. I'll have a look, I guess, but it probably requires two extra flags at the top of the jar.mn file or something? We wouldn't want this kind of thing to affect the content/ manifests, or the manifests for other apps.

Comment 27

[Mike Hommey [:glandium]]

I'm on it. I'll probably file a separate bug.

Comment 28

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #27)
> I'll probably file a separate bug.

Turns out I won't. After sleeping on it, I have a minimalistic approach that works, only minimalistically changing jar.py.

diff --git a/browser/themes/linux/jar.mn b/browser/themes/linux/jar.mn
index b97dbd0..d7ce32e 100644
--- a/browser/themes/linux/jar.mn
+++ b/browser/themes/linux/jar.mn
@@ -445,12 +445,13 @@ browser.jar:
   skin/classic/browser/devtools/tooltip/arrow-vertical-light.png   (../shared/devtools/tooltip/arrow-vertical-light.png)
   skin/classic/browser/devtools/tooltip/[e-mail]   (../shared/devtools/tooltip/[e-mail])
 #ifdef E10S_TESTING_ONLY
   skin/classic/browser/[e-mail] (../shared/[e-mail])
 #endif
   skin/classic/browser/warning16.png                        (../shared/warning16.png)
   skin/classic/browser/[e-mail]                     (../shared/[e-mail])
 
+../extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.jar:
 % override chrome://browser/skin/feeds/audioFeedIcon.png              chrome://browser/skin/feeds/feedIcon.png
 % override chrome://browser/skin/feeds/audioFeedIcon16.png            chrome://browser/skin/feeds/feedIcon16.png
 % override chrome://browser/skin/feeds/videoFeedIcon.png              chrome://browser/skin/feeds/feedIcon.png
 % override chrome://browser/skin/feeds/videoFeedIcon16.png            chrome://browser/skin/feeds/feedIcon16.png
diff --git a/python/mozbuild/mozbuild/jar.py b/python/mozbuild/mozbuild/jar.py
index 0824403..8c9e1b0 100644
--- a/python/mozbuild/mozbuild/jar.py
+++ b/python/mozbuild/mozbuild/jar.py
@@ -65,17 +65,17 @@ def getModTime(aPath):
 
 
 class JarMaker(object):
     '''JarMaker reads jar.mn files and process those into jar files or
       flat directories, along with chrome.manifest files.
       '''
 
     ignore = re.compile('\s*(\#.*)?$')
-    jarline = re.compile('(?:(?P<jarfile>[\w\d.\-\_\\\/]+).jar\:)|(?:\s*(\#.*)?)\s*$')
+    jarline = re.compile('(?:(?P<jarfile>[\w\d.\-\_\\\/{}]+).jar\:)|(?:\s*(\#.*)?)\s*$')
     relsrcline = re.compile('relativesrcdir\s+(?P<relativesrcdir>.+?):')
     regline = re.compile('\%\s+(.*)$')
     entryre = '(?P<optPreprocess>\*)?(?P<optOverwrite>\+?)\s+'
     entryline = re.compile(entryre
                            + '(?P<output>[\w\d.\-\_\\\/\+\@]+)\s*(\((?P<locale>\%?)(?P<source>[\w\d.\-\_\\\/\@\*]+)\))?\s*$'
                            )
 
     def __init__(self, outputFormat='flat', useJarfileManifest=True,


You still need to add browser/extensions/{...}/chrome.manifest to package-manifest.in with this, and, obviously, do the same in other jar.mn files. One caveat is that this creates a useless chrome.manifest file in browser/extensions, but it doesn't matter because a) nothing reads it and b) it doesn't end up in the shipped firefox.

I'll make sure my refactor of jar.py cleans that up, but we can live with this in the meanwhile.

Comment 29

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1150417 - move overrides into theme manifest, r?glandium,dao

Bug 1150417 - move overrides into theme manifest, r?glandium,dao

Comment 30

[:Gijs (out for now; he/him)]

This works well for browser/themes, but not for toolkit.

I tried to use ../browser/extensions/.. which worked if I built manually with the specific directories in the right order, but the lines were not in the packaged manifest in a plain ./mach build followed by ./mach package. Come to think of it, that may be because I was #ifdef'ing them inside MOZ_APP_BASENAME... of course, we can't use ../browser/ as a location for stuff if we're not actually building a browser...

Thoughts, Mike? Am I missing something obvious? (toolkit doesn't seem to have an extensions folder that gets merged, or something...)

Comment 31

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #30)
> This works well for browser/themes, but not for toolkit.
> 
> I tried to use ../browser/extensions/.. which worked if I built manually
> with the specific directories in the right order, but the lines were not in
> the packaged manifest in a plain ./mach build followed by ./mach package.
> Come to think of it, that may be because I was #ifdef'ing them inside
> MOZ_APP_BASENAME... of course, we can't use ../browser/ as a location for
> stuff if we're not actually building a browser...
> 
> Thoughts, Mike? Am I missing something obvious? (toolkit doesn't seem to
> have an extensions folder that gets merged, or something...)

It turns out the obvious thing I'm missing is what ifdefs to use. Fixed now, toolkit addendum incoming.

Comment 32

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1150417 - part 2: add toolkit support, r?glandium,dao

Bug 1150417 - part 2: add toolkit support, r?glandium,dao

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8612323 [details]
MozReview Request: Bug 1150417 - move overrides into theme manifest, r?glandium,dao

https://reviewboard.mozilla.org/r/9175/#review8367

Ship It!

Comment 34

[Mike Hommey [:glandium]]

Comment on attachment 8612362 [details]
MozReview Request: Bug 1150417 - part 2: add toolkit support, r?glandium,dao

https://reviewboard.mozilla.org/r/9559/#review8369

That what I was going to suggest.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 8612362 [details]
MozReview Request: Bug 1150417 - part 2: add toolkit support, r?glandium,dao

Thanks!

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9d52a8b0e028
https://hg.mozilla.org/integration/fx-team/rev/f7b3b8f0283b

Comment 37

[:Gijs (out for now; he/him)]

Backed out because actually... this doesn't work. I'm not sure why I didn't run into this before, but override directives aren't supported in theme manifests.

https://hg.mozilla.org/integration/fx-team/rev/f285f21edd1d

Comment 38

[Philip Chee]

(In reply to :Gijs Kruitbosch from comment #37)
> but override directives aren't supported in theme manifests.
No shit, Sherlock.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/39821d952adc

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39821d952adc

Comment 41

[:Gijs (out for now; he/him)]

Alfred, can you verify that this fix works as advertised for you with a Nightly from today? (Windows is still building (should be done "soon"), OS X and Linux are already available)

Comment 43

[Alfred Kayser]

I am trying to setup a XP box/vm, but it takes some time unfortunately.

Comment 44

[Alfred Kayser]

This fix seems to work, no more issues with overrides for my themes.
Will this also be applied to the 40 branch?

Comment 45

[:Gijs (out for now; he/him)]

(In reply to Alfred Kayser from comment #44)
> This fix seems to work, no more issues with overrides for my themes.
> Will this also be applied to the 40 branch?

Going to request approval now that it's been verified, yeah. :-)

Comment 46

[:Gijs (out for now; he/him)]

Comment on attachment 8612323 [details]
MozReview Request: Bug 1150417 - move overrides into theme manifest, r?glandium,dao

Approval Request Comment
[Feature/regressing bug #]: bug 706103
[User impact if declined]: third-party themes have to do a lot of work in order not to break
[Describe test coverage new/current, TreeHerder]: this basically shuffles some file contents around; if it broke anything we would have noticed by now (as we did the first time it landed when it did break something
[Risks and why]: low/none
[String/UUID change made/needed]: nope

NB: needs bug 1170207 to also be uplifted.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6d3862da170

Comment 48

[Pardal Freudenthal (:ShareBird)]

The override approach is still breaking third-party themes. For example, because of the override in https://bugzilla.mozilla.org/show_bug.cgi?id=1188291 for the close button, my themes have no close buttons anymore.

Comment 49

[:Gijs (out for now; he/him)]

(In reply to Pardal Freudenthal (:ShareBird) from comment #48)
> The override approach is still breaking third-party themes. For example,
> because of the override in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1188291 for the close button,
> my themes have no close buttons anymore.

The patch there doesn't add any override directives, so I don't understand what problem you're experiencing. In any case, please file a followup bug for your issue.

Comment 50

[Pardal Freudenthal (:ShareBird)]

(In reply to :Gijs Kruitbosch from comment #49)
> (In reply to Pardal Freudenthal (:ShareBird) from comment #48)
> > The override approach is still breaking third-party themes. For example,
> > because of the override in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1188291 for the close button,
> > my themes have no close buttons anymore.
> 
> The patch there doesn't add any override directives, so I don't understand
> what problem you're experiencing. In any case, please file a followup bug
> for your issue.

Sorry, wrong bug. The overrides are in https://bugzilla.mozilla.org/show_bug.cgi?id=1173729

Anyway, I will investigate it further, but it seems that moving the overrides into a manifest inside the default theme doesn't solve the problems for third-party themes.

Comment 51

[Pardal Freudenthal (:ShareBird)]

So, I've tested it further using Firefox 41.0a2 (2015-08-05) and the issues are still there.

This is true for all images being overridden in the theme's chrome.manifest.

Comment 52

[:Gijs (out for now; he/him)]

(In reply to Pardal Freudenthal (:ShareBird) from comment #51)
> So, I've tested it further using Firefox 41.0a2 (2015-08-05) and the issues
> are still there.
> 
> This is true for all images being overridden in the theme's chrome.manifest.

I've said this before, please file a followup bug. Please also provide significantly more details (like STR and a better description than "issues").

Comment 53

[Pardal Freudenthal (:ShareBird)]

The "issues" are exactly what this bug is about. Just exactly what the bug opener describes at the begin.
Which more details you need? I've tested using the last aurora build in Windows 7 and Windows XP. 

It's maybe difficult to catch the problems, since most of theme developers have already renamed the affected files. I've only noticed because of the close button I've mentioned before and the fact that the overrides I've mentioned before has as target Windows 7.

To reproduce this issues you can make a simple test.

1. Load Charamel or Silvermel from http://forums.mozillazine.org/viewtopic.php?p=14253377#p14253377
2. My toolbar files are already renamed, instead of Toolbar they are called toolbar and toolbar-small,
so, edit the chrome.manifest inside \browser\extensions\{972ce4c6-7e08-4474-a285-3208198ce6fd} and rename the Toolbar override for the platform you are on, to toolbar-small.
For example: if you are on Windows 7 change:
override chrome://browser/skin/Toolbar.png chrome://browser/skin/Toolbar-aero.png os=WINNT osversion=6.1 to
override chrome://browser/skin/toolbar-small.png chrome://browser/skin/Toolbar-aero.png os=WINNT osversion=6.1
3. restart Firefox and you will not be able to see most of the toolbar buttons...

I didn't file a "followup bug" because it is not a new issue. This bug is not fixed.