Both Dev Edition and Default theme are showing up as selected in Customize mode in Aurora40.0a2

VERIFIED FIXED in Firefox 51

Status

()

defect
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: bgrins, Assigned: dao)

Tracking

(Blocks 1 bug, {regression})

40 Branch
mozilla52
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 verified)

Details

(Whiteboard: [devedition-polish][devtools-ux])

Attachments

(1 attachment, 2 obsolete attachments)

+++ 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 "firefox-devedition@mozilla.org" and restarting the browser
Posted patch install-lwtheme.patch (obsolete) — Splinter Review
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?
Flags: needinfo?(dao)
(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.
Posted patch lwtheme-install.patch (obsolete) — Splinter Review
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)
Duplicate of this bug: 1182920
(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+
This should just work - would love to get this fixed in this cycle.
Whiteboard: [devedition-polish][devtools-ux]
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)
Blocks: 1306561
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)
(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)
Dao, can you pick this up then? Comment 11 makes it sound like you know what to do. :)
Flags: needinfo?(dao+bmo)
I can take a look, but comment 11 was very high-level. I don't know what exactly needs to be done.
Posted patch patchSplinter Review
Assignee: bgrinstead → dao+bmo
Attachment #8607767 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
Attachment #8804631 - Flags: review?(dtownsend)
Component: Theme → Add-ons Manager
Product: Firefox → Toolkit
Attachment #8804631 - Flags: review?(dtownsend) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/41a2caef2b66
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi :dao, 
Since bug 1306561 is also affected in 51, do you want to uplift this to 51 aurora?
Flags: needinfo?(dao+bmo)
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 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+
Depends on: 1313960
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)
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.
Flags: needinfo?(gchang)
Flags: needinfo?(dao+bmo)
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.