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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Brian Grinstead [:bgrins]]

+++ 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

Comment 1

[Brian Grinstead [:bgrins]]

This can be reproduced on m-c by manually setting the "lightweightThemes.selectedThemeID" pref to "[e-mail]" and restarting the browser

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: install-lwtheme.patch

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)

Comment 3

[Dão Gottwald [: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 :/

Comment 4

[Brian Grinstead [:bgrins]]

(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?

Comment 5

[Brian Grinstead [:bgrins]]

(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

[Eric Shepherd [:sheppy]]

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.

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: 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.

Comment 9

[Brian Grinstead [:bgrins]]

(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).

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

This should just work - would love to get this fixed in this cycle.

Comment 11

[Dão Gottwald [:dao]]

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).

Comment 12

[Justin Dolske [:Dolske]]

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?

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 14

[Justin Dolske [:Dolske]]

Dao, can you pick this up then? Comment 11 makes it sound like you know what to do. :)

Comment 15

[Dão Gottwald [:dao]]

I can take a look, but comment 11 was very high-level. I don't know what exactly needs to be done.

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

If this is appropriate for uplift, note that we believe if fixed a bug 1306561 which would need a 51 uplift.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/41a2caef2b66

Comment 20

[Gerry Chang [:gchang]]

Hi :dao, 
Since bug 1306561 is also affected in 51, do you want to uplift this to 51 aurora?

Comment 21

[Dão Gottwald [:dao]]

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

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8804631 [details] [diff] [review]
patch

Fix a regression related to Dev Edition theme. Take it in 51 aurora.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3fa837aa715

Comment 24

[Adrian Florinescu [:aflorinescu]]

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?

Comment 25

[:Gijs (he/him)]

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.

Comment 26

[Gerry Chang [:gchang]]

We can leave this bug as fixed and mark status-firefox52 affected in bug 1313960 because 51 is in aurora now.

Comment 27

[Andrei Vaida [:avaida]]

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).