Closed
Bug 1339754
Opened 8 years ago
Closed 7 years ago
Compact themes are not applied after restart if you try to enable them with a complete theme installed
Categories
(Toolkit :: Add-ons Manager, defect, P5)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox53 | --- | affected |
firefox54 | --- | affected |
People
(Reporter: cgeorgiu, Unassigned)
References
Details
(Whiteboard: triaged)
[Affected versions]:
- latest Nightly 54.0a1 (2017-02-14)
- latest Aurora 53.0a2 (2017-02-14)
[Affected platforms]:
- Windows 10 x64
- macOS 10.12 (Sierra)
- Ubuntu 16.04 x64 LTS
[Steps to reproduce]:
1. Launch Firefox
2. Go to: https://addons.mozilla.org/En-us/firefox/complete-themes/ and install a complete theme, e.g "LavaFox V2"
3. Restart the browser in order to install the complete theme
4. Go to about:addons and select "Appearance" (if it's not already in focus)
5. Enable Compact Dark theme and click "Restart now" to apply the selected theme
[Expected result]:
- Compact Dark theme is enabled after restart and the other themes are displayed as Disabled
[Actual result]:
- Compact Dark theme is not enabled and all themes are displayed as Disabled
[Regression range]:
- This is not a regression as it is reproducible on Nightly 53.0a1 (20170115030210) where this feature first landed
[Additional notes]:
- both Compact Dark and Compact Light are affected
- see screenshot with about:addons after browser restart: http://imgur.com/a/MNl2N
Reporter | ||
Updated•8 years ago
|
Has STR: --- → yes
Updated•8 years ago
|
Component: Theme → Add-ons Manager
Product: Firefox → Toolkit
Comment 1•8 years ago
|
||
Does this affect other lightweight themes besides the compact ones we're now shipping?
Andrew, do you have time to look at this?
Flags: needinfo?(ciprian.georgiu)
Flags: needinfo?(aswan)
Reporter | ||
Comment 2•8 years ago
|
||
(In reply to :Gijs from comment #1)
> Does this affect other lightweight themes besides the compact ones we're now
> shipping?
No, it only affects compact themes.
Flags: needinfo?(ciprian.georgiu)
Comment 4•8 years ago
|
||
This seems to be an old issue which reproduces constantly on Nightly and Dev Edition branches. See Bug 1194638.
Comment 5•8 years ago
|
||
(In reply to :Gijs from comment #1)
> Andrew, do you have time to look at this?
I'll put it in my queue, hopefully can get to it this week...
Comment 6•8 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #5)
> (In reply to :Gijs from comment #1)
> > Andrew, do you have time to look at this?
>
> I'll put it in my queue, hopefully can get to it this week...
That would be great. While I do not think this should block compact themes ( not many people use complete themes ) I would support this fix being uplifted to 53. Please needinfo me if you need help with that.
Updated•8 years ago
|
Priority: -- → P3
Comment 7•8 years ago
|
||
I think the basic problem here is that when we switch from a complete them to a lightweight theme, the code that applies the change after a restart is here:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/mozapps/extensions/LightweightThemeManager.jsm#328
Now, the add-ons manager itself is started indirectly from, of all things, nsXREDirProvider::DoStartup, which gets called from here:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/xre/nsAppRunner.cpp#4307
But built-in themes don't get added to the LWT manager until the final-ui-startup event which happens a bit later:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/xre/nsAppRunner.cpp#4307
The simplest fix would be to somehow move the addition of built-in lightweight themes earlier but I'm in over my head on the whole startup sequence. We could also add code to the LWT manager to handle this sequence (ie, if getUsedTheme() returns null, remember the theme we want to apply and then apply it in addBuiltinTheme() ) but that sounds gross to me.
I could use some guidance here...
Flags: needinfo?(aswan) → needinfo?(dtownsend)
Comment 8•8 years ago
|
||
Whoops, copy/paste error, the final-ui-startup event is dispatched here:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/xre/nsAppRunner.cpp#4401
which eventually leads to the builtin themes getting installed here:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/browser/components/nsBrowserGlue.js#644-663
Comment 9•8 years ago
|
||
I think what you want to do is to move the calls to addBuiltinTheme to just before the add-ons manager gets started up. You can add a listener for the profile-do-change topic to browserblue's init function (https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#389). profile-do-change is called immediately before the add-ons manager startup (http://searchfox.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1150).
At first glance that should all be safe but you might find you have to tinker with the LWT code to make sure all the data structures it needs for those calls to succeed are initialised early enough.
Flags: needinfo?(dtownsend)
Updated•8 years ago
|
Whiteboard: triaged
Comment 10•8 years ago
|
||
Complete themes are deprecated and will no longer be loaded in Firefox 57. From what I can see so far, this looks like quite a tricky bug for just a few releases. The number of complete theme users is small, is this worth the effort at this stage?
Suggesting moving down to a P5 or won't fixing.
Comment 11•8 years ago
|
||
(In reply to Andy McKay [:andym] from comment #10)
...
> Suggesting moving down to a P5 or won't fixing.
I ust assumed P3 would relegate this to the ages.
Updated•8 years ago
|
Priority: P3 → P5
Comment 13•7 years ago
|
||
We no longer support complete themes -> wontfix.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•