Closed Bug 1150417 Opened 5 years ago Closed 4 years ago

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

Categories

(Toolkit Graveyard :: Build Config, defect, major)

defect
Not set
major

Tracking

(firefox40+ fixed, firefox41+ fixed)

VERIFIED FIXED
mozilla41
Tracking Status
firefox40 + fixed
firefox41 + fixed

People

(Reporter: alfredkayser, Assigned: Gijs)

References

Details

Attachments

(5 files, 1 obsolete file)

Attached image 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.
This is an about:support dump (after the "Refresh").
Severity: normal → major
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Note, there are no error messages of anything in the error console.
Maybe a fallout from bug 706103?
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.
Severity: major → critical
Depends on: 706103
Note, this also impacts Thunderbird in the same way!
Depends on: 1150006
(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
(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.
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.
Morphing this so it reflects how we intend to fix this. See also background in bug 706103 comment 11 through 17.
Severity: critical → major
Summary: Customs themes all stopped working since April 1st, 2015 → Move default theme overrides into separate chrome.manifest so it doesn't break custom themes
Dão, do you have cycles to work on this now? We should definitely fix this before 40 moves to Aurora.
Flags: needinfo?(dao)
Summary: Move default theme overrides into separate chrome.manifest so it doesn't break custom themes → Move default theme aero/XP overrides into separate chrome.manifest so it doesn't break custom themes
Thanks! Can I do anything to help here?
Attached file 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).
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...
Bug 1150843 and bug 1150867 should further unbreak third-party themes.

I'm going to try to work on this next week.
Component: Themes → Build Config
Flags: needinfo?(dao)
Blocks: 1156308
Now the main features are okay, but Bug 1156308 describes something you may forgot to fix.
Flags: needinfo?(dao)
[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?
Flags: needinfo?(dao)
Flags: needinfo?(dao)
Duplicate of this bug: 1156308
Tracking enabled for 40 and 41.
(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.
Flags: needinfo?(dao)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Points: --- → 5
Flags: qe-verify-
Flags: in-testsuite-
Flags: firefox-backlog+
Attached file MozReview Request: bz://1150417/Gijs (obsolete) —
/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 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...
Attachment #8608676 - Flags: feedback?(mh+mozilla)
Attachment #8608676 - Flags: feedback?(dao)
(fwiw, I've only tested this on OS X, where it seems to work pretty much as expected)
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.
Attachment #8608676 - Flags: feedback?(mh+mozilla)
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.
Attachment #8608676 - Flags: feedback?(dao)
(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.
Flags: needinfo?(mh+mozilla)
I'm on it. I'll probably file a separate bug.
(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/arrow-vertical-light@2x.png   (../shared/devtools/tooltip/arrow-vertical-light@2x.png)
 #ifdef E10S_TESTING_ONLY
   skin/classic/browser/e10s-64@2x.png (../shared/e10s-64@2x.png)
 #endif
   skin/classic/browser/warning16.png                        (../shared/warning16.png)
   skin/classic/browser/warning16@2x.png                     (../shared/warning16@2x.png)
 
+../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.
Flags: needinfo?(mh+mozilla)
Bug 1150417 - move overrides into theme manifest, r?glandium,dao
Attachment #8612323 - Flags: review?(mh+mozilla)
Attachment #8612323 - Flags: review?(dao)
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...)
Flags: needinfo?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
Bug 1150417 - part 2: add toolkit support, r?glandium,dao
Attachment #8612362 - Flags: review?(mh+mozilla)
Attachment #8612362 - Flags: review?(dao)
Attachment #8612323 - Flags: review?(mh+mozilla) → review+
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 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.
Attachment #8612362 - Flags: review?(mh+mozilla) → review+
Attachment #8612323 - Flags: review?(dao) → review+
Comment on attachment 8612362 [details]
MozReview Request: Bug 1150417 - part 2: add toolkit support, r?glandium,dao

Thanks!
Attachment #8612362 - Flags: review?(dao) → review+
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
Depends on: 1170207
https://hg.mozilla.org/mozilla-central/rev/39821d952adc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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)
Flags: needinfo?(alfredkayser)
Attachment #8608676 - Attachment is obsolete: true
I am trying to setup a XP box/vm, but it takes some time unfortunately.
Flags: needinfo?(alfredkayser)
This fix seems to work, no more issues with overrides for my themes.
Will this also be applied to the 40 branch?
(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. :-)
Status: RESOLVED → VERIFIED
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.
Attachment #8612323 - Flags: approval-mozilla-aurora?
Attachment #8612323 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1176703
Blocks: 1190465
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.
(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.
(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.
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(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").
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
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.
Depends on: 1191535
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.