1191535 - Overrides from all themes are applied simultaneously when using dynamic theme switching (DSS)

Status: RESOLVED WONTFIX

(Toolkit :: Themes, defect)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1150417 +++

(from bug 1150417 comment 53)
(In reply to Pardal Freudenthal (:ShareBird) from comment #53)
> 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.

Does this reproduce on Firefox 40? Or just 41? What about Nightly (42)?

Your theme on AMO can't be installed on 41 or 42, so I can't really test on 41 as you suggested, but I checked on 40, and running the following code in the browser console:

locs = Components.manager.getManifestLocations()
for (let i = 0; i < locs.length; i++) { console.log(locs.queryElementAt(i, Ci.nsIURI).spec); }

doesn't log the manifest you're requesting I edit (instead, it logs the manifest URI for the silvermel theme). That should mean it's not being read. So it's not clear to me how what you're suggesting would work, or that it's the same issue. 

In any case, I'd like to investigate separately as it will require a different kind of fix than what we already did, and reopening the same bug will make it a pain to track QA efforts. It is also already too late to fix this for Firefox 40, which had the same fixes, which were tracked in that bug.

Comment 1

[:Gijs (he/him)]

(I just tried editing the two Toolbar.png lines for osversion 6.2 and 6.3 on my win8 box to toolbar-small.png for my main Firefox 40 install, and that didn't do anything to the toolbar icons I could see with the silvermel theme in use...)

Comment 2

[Pardal Freudenthal (:ShareBird)]

Running the code you provided, I get this line:
"file:///D:/Programme/Mozilla/firefox/next/firefox/browser/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/chrome.manifest"
So, it loads the chrome.manifest from the default theme.

I've tested not only on my Windows 7 machine, but also on a XP machine as well. I got the same results. The images being overridden doesn't appear.

Comment 3

[:Gijs (he/him)]

Are you testing in safe mode? Or normal mode? Can you reproduce against a clean profile (with only your theme, not the extension bit added) ?

It's very strange that the default theme manifest would be loaded when a different theme is selected, and I don't really know why that would happen (or why it's happening on your machines but not mine). Is the version on AMO the right one to test?

Comment 4

[Pardal Freudenthal (:ShareBird)]

(In reply to :Gijs Kruitbosch from comment #3)
> Are you testing in safe mode? Or normal mode? Can you reproduce against a
> clean profile (with only your theme, not the extension bit added) ?
> 

Sorry, I will try tomorrow in a clean profile. I don't have the time to do it now.
As an additional information, I'm using the zipped file from last Aurora, not the install but it shouldn't make a difference here.

> It's very strange that the default theme manifest would be loaded when a
> different theme is selected, and I don't really know why that would happen
> (or why it's happening on your machines but not mine). Is the version on AMO
> the right one to test?

To be honest, it is not a surprise for me. Notice that the overrides keywords were disabled for themes because of one theme using it and messing up all other themes on the past.

Comment 5

[:Gijs (he/him)]

I'll be interested to hear what happens in a clean profile.

(In reply to Pardal Freudenthal (:ShareBird) from comment #4)
> Notice that the overrides
> keywords were disabled for themes because of one theme using it and messing
> up all other themes on the past.

What is your source for this? I would expect them to have been blocked because themes are meant to be safe, and :

override chrome://browser/content/browser.js chrome://myeviltheme/skin/browser.js

defeats that kind of security (and still isn't allowed).

Comment 6

[Pardal Freudenthal (:ShareBird)]

Sorry! Shame on me! How many times I've written "Did you test in a clean profile"? And now I didn't...
So, the issues are not present in a clean profile. 

I've figured out what was causing the problems. For development proposes, I set the preference extensions.dss.enabled to true. Unfortunately for all my dozen development profiles in all machines. 
With this preference set to true the chrome.manifest from all themes are loaded.

Please, I apologize. Working 9-10 hours every day on my regular job, with so little time to maintain two themes for Firefox and Thunderbird in this release cycle, with so many changes on the UI every new version, I rushed with my despairs...

Anyway, we can close this bug as unconfirmed, or edit it to better reflect the real problem. I would like to know why this occurs, since the preference is the core from an extension I'm working on for a while now, "Switch Themes", that allows switching themes without restart.

Comment 7

[:Gijs (he/him)]

No worries. I learned something new - I wasn't aware of this dss preference.

This pref causes us to write the entire list of theme addons into the addon list at shutdown ( http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#1418 ) and that will cause all the manifests to be loaded, causing all the overrides to apply simultaneously.

The bad news is that I don't think we will invest in fixing this issue, because doing so would be quite difficult. Comparatively, only a small fraction of our userbase would be interested in switching themes like this, and the filesize and code sanity savings on the Firefox side for this way of organizing things (using override quite heavily instead of duplicating all our files) are quite large.

The future of complete themes as they are is uncertain. We at least agree that it is unfortunate that applying a different (complete) theme right now requires a restart (note that even with the pref flipped, installing a new complete theme also still requires a restart, which is inconvenient and a little confusing). I would expect that when we evaluate theming for "great or dead", the "great" option would definitely involve making themes apply without a restart, like lightweight themes work now, and making themes more powerful than lightweight themes are now, and ideally easier to maintain than complete themes are right now (because we know that testing them is difficult for individual developers as soon as themes try to do much more than replace the default icons with identically sized ones).

Until we get clarity on the future for theming, as I noted in the "bad news" paragraph, I don't think we will invest heavily in fixing this. We'd need to heavily modify the code that deals with themes to basically treat them as restartless add-ons when dss is enabled (and not load all of them at startup and then hope for the best). DSS as it is is likely to have quirks in terms of invalidation that would be hard to address in the current framework. We don't have manpower available to fix all these things right now (certainly not while we're not sure how we will proceed from here).

Comment 8

[Pardal Freudenthal (:ShareBird)]

Thank you for your understanding, patience and for your thoughts...

Yes, now I remember to have seen something about this a while ago. I will investigate if I can deal with this inside the extension, but I'm not really optimist. 
The extension works like a charm in Firefox 39 and below (although it needs urgently a lot of refactoring, since I wrote it a long time ago). 
Basically it uses along with this preference the reloadChrome method from the ChromeRegistry service and the sessionstore service.
If you want, you can have a look on it in this thread:
http://forums.mozillazine.org/viewtopic.php?p=14209619#p14209619

Comment 9

[Robert Kaiser]

I personally think that in the light of "great or dead", DSS should be on the "dead" list. We should remove the pref and feature, it never worked completely smoothly and there is no big interest in putting time into making it actually fully work.

Comment 10

[Frank Lion]

(In reply to :Gijs Kruitbosch from comment #7)
 
> The future of complete themes as they are is uncertain.... 

>... and ideally easier to maintain
> than complete themes are right now (because we know that testing them is
> difficult for individual developers as soon as themes try to do much more
> than replace the default icons with identically sized ones).

Not that difficult at all, in my case. Any consideration of Complete Themes should take into account the new way some of us are making them. We have sorted the very thing that Asa Dotzler always used to beat us with a stick about - continuous breaking. Now, they don't, major breakings don't happen and toolbar buttons appear in the themes the same day that they appear in the default and certainly my own themes do a lot more than use different sized toolbarbuttons than the default.

Feel free to consider the future, but I would just ask that everyone is up to date with the present.

Comment 11

[Pardal Freudenthal (:ShareBird)]

(In reply to Frank Lion from comment #10)
> 
> Not that difficult at all, in my case. Any consideration of Complete Themes
> should take into account the new way some of us are making them. We have
> sorted the very thing that Asa Dotzler always used to beat us with a stick
> about - continuous breaking. Now, they don't, major breakings don't happen
> and toolbar buttons appear in the themes the same day that they appear in
> the default and certainly my own themes do a lot more than use different
> sized toolbarbuttons than the default.
> 
> Feel free to consider the future, but I would just ask that everyone is up
> to date with the present.

Only a consideration about this new way. Although minimize the effects that changes in the default theme may have, it does not guarantee that those changes do not cause damage to the themes developed in this way.

Note that even though it was I who developed this method, described in this article:
http://www.tudobom.de/articles/yatt/#light_weight, I myself don't use it, given its limitations. I could not develop Silvermel and Charamel using this method...

Comment 12

[Frank Lion]

(In reply to Pardal Freudenthal (:ShareBird) from comment #11)
>  I myself don't use it,
> given its limitations.

Limitations then. Things have changed since back then, Pardal. :) 

Still, I just thought I ought to mention it in passing, if people are considering the future of Complete Themes. Just so everybody is 'on the same page'.

Comment 13

[Pardal Freudenthal (:ShareBird)]

(In reply to Frank Lion from comment #12)
> Limitations then. Things have changed since back then, Pardal. :) 
> 
> Still, I just thought I ought to mention it in passing, if people are
> considering the future of Complete Themes. Just so everybody is 'on the same
> page'.

Sure!

Comment 14

[Tim Nguyen :ntim]

DSS was removed in bug 1343821.