Closed Bug 1164954 Opened 7 years ago Closed 6 years ago
Both Dev Edition and Default theme are showing up as selected in Customize mode in Aurora40
+++ 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
This can be reproduced on m-c by manually setting the "lightweightThemes.selectedThemeID" pref to "email@example.com" and restarting the browser
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: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8605933 - Flags: feedback?(dao)
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-
(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?
(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.
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.
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.
(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).
This should just work - would love to get this fixed in this cycle.
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).
Attachment #8607767 - Flags: review?(dao+bmo)
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?
(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.
Dao, can you pick this up then? Comment 11 makes it sound like you know what to do. :)
I can take a look, but comment 11 was very high-level. I don't know what exactly needs to be done.
Component: Theme → Add-ons Manager
Product: Firefox → Toolkit
Attachment #8804631 - Flags: review?(dtownsend) → review+
Pushed by firstname.lastname@example.org: 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.
Hi :dao, Since bug 1306561 is also affected in 51, do you want to uplift this to 51 aurora?
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
Attachment #8804631 - Flags: approval-mozilla-aurora?
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+
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?
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)
We can leave this bug as fixed and mark status-firefox52 affected in bug 1313960 because 51 is in aurora now.
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
You need to log in before you can comment on or make changes to this bug.