Closed
Bug 1164954
Opened 10 years ago
Closed 8 years ago
Both Dev Edition and Default theme are showing up as selected in Customize mode in Aurora40.0a2
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
VERIFIED
FIXED
mozilla52
People
(Reporter: bgrins, Assigned: dao)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [devedition-polish][devtools-ux])
Attachments
(1 file, 2 obsolete files)
1.48 KB,
patch
|
mossop
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1164178 +++ STR: 1. Install Aurora40.0a2 from http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-win32/1431416426/ 2. Open up Customize Mode 3. Click themes dropdown Expected The Dev Edition theme is selected Actual Both the Dev Edition theme and Default theme are selected It seems that the Default theme is never deactivated in the same way if a user action hasn't enabled a lightweight theme. This is causing other issues, like https://bugzilla.mozilla.org/show_bug.cgi?id=1164178#c9
Reporter | ||
Comment 1•10 years ago
|
||
This can be reproduced on m-c by manually setting the "lightweightThemes.selectedThemeID" pref to "firefox-devedition@mozilla.org" and restarting the browser
Reporter | ||
Comment 2•10 years ago
|
||
I discovered that since _setCurrentTheme was never called for the dev edition theme, it was never properly 'installed' as far as the addon listeners were concerned. This means the default theme was not being disabled. I have an alternate idea that involves managing this all inside the LW theme manager on `startup` that calls the relevant addon listeners if needed. But I'm not sure how to detect this scenario from in there (the default theme is still being considered active)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8605933 [details] [diff] [review] install-lwtheme.patch browser-devedition.js runs per browser window, not per browser session, so this doesn't seem like the right place for doing that. Looks like a pretty gross hack, too :/
Attachment #8605933 -
Flags: feedback?(dao) → feedback-
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #3) > Comment on attachment 8605933 [details] [diff] [review] > install-lwtheme.patch > > browser-devedition.js runs per browser window, not per browser session, so > this doesn't seem like the right place for doing that. > > Looks like a pretty gross hack, too :/ Do you have any thoughts of a better way to do this? I can't seem to tell from the LightweightThemeManager whether or not the theme has been 'activated' (more specifically, the AddonWrapper says it always has because it matches LWTM.currentTheme). Maybe there is some listener I can call on startup if currentTheme != null that will, if needed, notify the default theme to become deactivated?
Flags: needinfo?(dao)
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #3) > browser-devedition.js runs per browser window, not per browser session, so > this doesn't seem like the right place for doing that. This 'isActive' condition would only be hit one time - once the currentTheme has been set it won't run again (even if the check runs per window). This condition only holds true when a LW theme is set via a default pref value.
Comment 6•10 years ago
|
||
I'm seeing the following... With Dev Edition theme enabled in about:config, the following happens: If I open the Customize panel from the hamburger menu and use the "Themes" menu to pick "Default", the theme switches to the default theme... *BUT* the next time Sync triggers (apparently -- it does seem to correlate with Sync, both automatic and manually-triggered, but could possibly happen at other times too), the theme reverts to the Dev Edition theme. If I open about:addons and go to the Themes panel, clicking any other theme has no effect. The Dev Edition theme stays active. With the Dev Edition theme disabled, all attempts at configuring themes work as expected.
Reporter | ||
Comment 7•10 years ago
|
||
Hopefully this is better - I'm checking at the same time the theme is registered in nsBrowserGlue, and also have a pref set to make sure it happens only once.
Attachment #8605933 -
Attachment is obsolete: true
Attachment #8607767 -
Flags: review?(dao)
Reporter | ||
Comment 9•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #7) > Created attachment 8607767 [details] [diff] [review] > lwtheme-install.patch > > Hopefully this is better - I'm checking at the same time the theme is > registered in nsBrowserGlue, and also have a pref set to make sure it > happens only once. Dão, can you please take a look at this? It's a pretty bad issue that we'd like to get fixed. See Comment 4 - I think this new approach is better since it only runs once per profile, but maybe the LightweightThemeManager can handle this case transparently (theme applied due to a default value of selectedThemeID).
Flags: qe-verify+
Comment 10•9 years ago
|
||
This should just work - would love to get this fixed in this cycle.
Updated•9 years ago
|
Whiteboard: [devedition-polish][devtools-ux]
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8607767 [details] [diff] [review] lwtheme-install.patch After more than a year (oops) this still doesn't seem like the right fix to me. I think we should fix this in LightweightThemeManager (i.e. actually support lightweightThemes.selectedThemeID having a default value) or by "installing" the dev edition theme somehow differently (without the lightweightThemes.selectedThemeID default value).
Flags: needinfo?(dao+bmo)
Attachment #8607767 -
Flags: review?(dao+bmo)
Comment 12•8 years ago
|
||
This is a bit of an older bug, but sounds like it's manifesting in a new way via bug 1306561 -- Brian, do you expect to be able to be pick this up, or should I find a new owner?
Flags: needinfo?(bgrinstead)
Comment 13•8 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #12) > This is a bit of an older bug, but sounds like it's manifesting in a new way > via bug 1306561 -- Brian, do you expect to be able to be pick this up, or > should I find a new owner? Brian is OoO for a bit, better to find a new owner.
Flags: needinfo?(bgrinstead)
Comment 14•8 years ago
|
||
Dao, can you pick this up then? Comment 11 makes it sound like you know what to do. :)
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 15•8 years ago
|
||
I can take a look, but comment 11 was very high-level. I don't know what exactly needs to be done.
Assignee | ||
Comment 16•8 years ago
|
||
Assignee: bgrinstead → dao+bmo
Attachment #8607767 -
Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
Attachment #8804631 -
Flags: review?(dtownsend)
Assignee | ||
Updated•8 years ago
|
Component: Theme → Add-ons Manager
Product: Firefox → Toolkit
Updated•8 years ago
|
Attachment #8804631 -
Flags: review?(dtownsend) → review+
Comment 17•8 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/41a2caef2b66 Notify add-on listeners when adding a built-in theme that is supposed to be the selected theme. r=mossop
If this is appropriate for uplift, note that we believe if fixed a bug 1306561 which would need a 51 uplift.
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/41a2caef2b66
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 20•8 years ago
|
||
Hi :dao, Since bug 1306561 is also affected in 51, do you want to uplift this to 51 aurora?
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8804631 [details] [diff] [review] patch Approval Request Comment [Feature/regressing bug #]: bug 1148996 [User impact if declined]: see comment 0 [Describe test coverage new/current, TreeHerder]: n/a [Risks and why]: fairly simple fix, low risk [String/UUID change made/needed]: none
Flags: needinfo?(dao+bmo)
Attachment #8804631 -
Flags: approval-mozilla-aurora?
Comment 22•8 years ago
|
||
Comment on attachment 8804631 [details] [diff] [review] patch Fix a regression related to Dev Edition theme. Take it in 51 aurora.
Attachment #8804631 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
status-firefox51:
--- → affected
Comment 23•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3fa837aa715
Comment 24•8 years ago
|
||
I can confirm this fix as verified on: - 51.0a2 Build ID 20161113004006 - 52.0a1 Build ID 20161113030203. It also appears that this fix introduced the regression 1313960, but due to comment https://bugzilla.mozilla.org/show_bug.cgi?id=1313960#c1, it seems to me that the problem in 1313960 was only exposed by this fix. From this perspective, I am wondering if bug 1313960 should be tracked separately, while going ahead with uplifting this fix? Gerry, Gijs, your thoughts?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gchang)
Comment 25•8 years ago
|
||
I'm not sure why I was asked, as I didn't write the patch. I'm also confused why we're talking about uplift, given that this fix is about devedition and the default theme and is already on 51, and today 51 goes to beta, while neither beta nor release have the devedition theme, so uplifting to 50 seems out of the question. Anyway, maybe Dão has something to add.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dao+bmo)
Comment 26•8 years ago
|
||
We can leave this bug as fixed and mark status-firefox52 affected in bug 1313960 because 51 is in aurora now.
Flags: needinfo?(gchang)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(dao+bmo)
Comment 27•8 years ago
|
||
Removing the qe-verify flag since this bug has already been confirmed fixed on Fx52 builds that support the Dev Edition theme (see Comment 24).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•