Alice0775 White Reporter
	Description • 9 years ago

Build Identifier:
https://hg.mozilla.org/releases/mozilla-aurora/rev/e2ce1aac996e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0 ID:20150511122607

Aurora40.0a2 theme is broken since last Marge.

Steps to reproduce:
1. Install Aurora40.0a2 from http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-win32/1431416426/
2. Attempt to change theme to default from developer theme.
   You should disable Developer them from options
3. Restart

Actual Results:
Not Aurora default theme

Expected Results:
Persist Aurora default theme

And Toolbox/Toolbar items are transparent and title bar is also transparent on Windows7 Classic Visual style.

	Brian Grinstead [:bgrins] Assignee
	Comment 2 • 9 years ago

The transparency issue is probably caused by Bug 1158872 and using an accentcolor for the lightweight theme

	Brian Grinstead [:bgrins] Assignee
	Comment 3 • 9 years ago

(In reply to Alice0775 White from comment #0)
> Build Identifier:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e2ce1aac996e
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
> ID:20150511122607
> 
> Aurora40.0a2 theme is broken since last Marge.
> 
> Steps to reproduce:
> 1. Install Aurora40.0a2 from
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> aurora-win32/1431416426/
> 2. Attempt to change theme to default from developer theme.
>    You should disable Developer them from options
> 3. Restart
> 
> Actual Results:
> Not Aurora default theme
> 
> Expected Results:
> Persist Aurora default theme

If you've selected a different theme, I'd expect that it does not reopen with the dev edition theme but rather persists your selection (in this case the default theme).  Note that this behavior has changed for 40 since Bug 1148996 - it used to be that you went to customize mode to turn the theme on and off but now you do it as if it were any other theme.

	Alice0775 White Reporter
	Comment 4 • 9 years ago

Cutomize > Themes > Default 
I cannot change Dev theme in most case.
Sometimes, the appearances changed to default, but The dev theme returns after restart the browser.

	Alice0775 White Reporter
	Comment 5 • 9 years ago

I forgot to mention that I was tested with newly created profile.

	Brian Grinstead [:bgrins] Assignee
	Comment 6 • 9 years ago

(In reply to Alice0775 White from comment #4)
> Cutomize > Themes > Default 
> I cannot change Dev theme in most case.
> Sometimes, the appearances changed to default, but The dev theme returns
> after restart the browser.

What do you mean it can't change in most cases?  Is the button disabled or does pressing it do nothing, or something else?

	Alice0775 White Reporter
	Comment 7 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Alice0775 White from comment #4)
> > Cutomize > Themes > Default 
> > I cannot change Dev theme in most case.
> > Sometimes, the appearances changed to default, but The dev theme returns
> > after restart the browser.
> 
> What do you mean it can't change in most cases?  Is the button disabled or
> does pressing it do nothing, or something else?

Hovering mouse over "Default" changes appearances, but after click it returns to Dev theme.

	Alice0775 White Reporter
	Comment 8 • 9 years ago

I can also reproduce the problem(except transparency problem) on Ubuntu14.04.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2ce1aac996e
Mozilla/5.0 (X11; Linux i686; rv:40.0) Gecko/20100101 Firefox/40.0 ID:20150511122607

	Jonathan Howard
	Comment 9 • 9 years ago

Clean profile;
Customize > Themes > Default click does not change theme.
Add-ons > Appearance. Default is not disabled and has no enable button.

Customize > Themes > Developer Edition - followed by - Themes > Default works.
Alternatively Add-ons > Appearance. Disable then enable Developer Edition to get enable button on default.

	Alice0775 White Reporter
	Comment 10 • 9 years ago

(In reply to Jonathan Howard from comment #9)
> Clean profile;
> Customize > Themes > Default click does not change theme.
> Add-ons > Appearance. Default is not disabled and has no enable button.
> 

yep

> Customize > Themes > Developer Edition - followed by - Themes > Default
> works.
> Alternatively Add-ons > Appearance. Disable then enable Developer Edition to
> get enable button on default.

What is the appearance of the browser after restart?

	Jonathan Howard
	Comment 11 • 9 years ago

(In reply to Alice0775 White from comment #10)
> (In reply to Jonathan Howard from comment #9)
> > Customize > Themes > Developer Edition - followed by - Themes > Default
> > works.
> > Alternatively Add-ons > Appearance. Disable then enable Developer Edition to
> > get enable button on default.
> 
> What is the appearance of the browser after restart?

Goes back to Developer Edition

	Brian Grinstead [:bgrins] Assignee
	Comment 12 • 9 years ago

OK, so I tested this today and there are some issues in Customize mode, but Addons -> Appearance seems to be working as normal.

In my initial testing (on a clean profile on osx), I'm able to switch between themes fine in customize mode, but there is something confusing happening in the UI where both 'default' and 'developer edition' appeared selected.

	Brian Grinstead [:bgrins] Assignee
	Comment 13 • 9 years ago

The reason it keeps coming back after restart is that the lightweightThemes.selectedThemeID doesn't seem to be updated at all when choosing the default theme (I believe that pref should be deleted if the default theme is applied)

	Brian Grinstead [:bgrins] Assignee
	Comment 14 • 9 years ago

It seems the calls to deleteBranch are ineffective in Aurora after a restart: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#305.

Once it restarts, the default value of the pref is copied back over.  The most straightforward thing seems to be providing a default value of "" in non-aurora channels and then setting it to that when a theme is unapplied.

	Brian Grinstead [:bgrins] Assignee
	Comment 15 • 9 years ago

I didn't realize that after deleting a branch it gets recreated on the next startup if there is a default value specified.  This fixes the problem of the DE theme disappearing upon startup.

	Brian Grinstead [:bgrins] Assignee
	Comment 16 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #15)
> This fixes the problem of
> the DE theme disappearing upon startup.

I mean fixes the problem of the DE theme 'reappearing' on startup if you've selected another one

	:Gijs (he/him)
	Comment 17 • 9 years ago

Comment on attachment 8605500 [details] [diff] [review]
selectedtheme-id.patch

Review of attachment 8605500 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good besides the test change, but I clearly didn't do a good job reviewing this last time, so going to 302 to Dão to make sure we're not missing something again.

::: browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
@@ -50,5 @@
>       "There should be two themes in the 'My Themes' section");
>  
>    let defaultTheme = header.nextSibling;
>    defaultTheme.doCommand();
> -  is(Services.prefs.prefHasUserValue("lightweightThemes.selectedThemeID"), false, "No lwtheme should be selected");

Why doesn't the existing assertion continue to work here?

	Brian Grinstead [:bgrins] Assignee
	Comment 18 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #17)
> @@ -50,5 @@
> >       "There should be two themes in the 'My Themes' section");
> >  
> >    let defaultTheme = header.nextSibling;
> >    defaultTheme.doCommand();
> > -  is(Services.prefs.prefHasUserValue("lightweightThemes.selectedThemeID"), false, "No lwtheme should be selected");
> 
> Why doesn't the existing assertion continue to work here?

Because when we call  _prefs.setCharPref("selectedThemeID", "") it gets set as a user value (even though it's matching the default value).  Unfortunately I don't think we can just clearUserPref instead since in Aurora the default is different.

	Dão Gottwald [:dao]
	Comment 19 • 9 years ago

Comment on attachment 8605500 [details] [diff] [review]
selectedtheme-id.patch

>+pref("lightweightThemes.selectedThemeID", "");
>+
> // Developer edition preferences
> #ifdef MOZ_DEV_EDITION
> sticky_pref("lightweightThemes.selectedThemeID", "firefox-devedition@mozilla.org");
> sticky_pref("browser.devedition.theme.enabled", true);
> #endif

pref("lightweightThemes.selectedThemeID", ""); should be in an #else branch, i.e. you shouldn't be setting that pref twice in dev edition.

I also can't see a good reason for using sticky_pref here, you should probably just use pref.

	Brian Grinstead [:bgrins] Assignee
	Comment 20 • 9 years ago

(In reply to Dão Gottwald [:dao] from comment #19)
> I also can't see a good reason for using sticky_pref here, you should
> probably just use pref.

You mean on the declarations in the #if?  That was done in Bug 1098343 so that you could have different defaults across channels and not have them be reverted when switching back to the default.  When a user set pref ends up matching the other channel's default it is reset to default after closing.

In fact, the more I think about it it seems we should maybe use a sticky_pref for the new one as well.  Mark, if we have a sticky_pref for the aurora channel, I'm assuming that the matching pref for non-aurora channels should also be sticky?

DE theme applied -> "lightweightThemes.selectedThemeID" = "firefox-devedition@mozilla.org" (default on aurora)
No theme applied -> "lightweightThemes.selectedThemeID" = "" (default on all other channels)

I could see a situation where you are on aurora and disable the DE theme, thus setting it to "".  Then if you open release with that same profile, it would match the default value of the pref.  Once closing release, it would reset the pref to the default value.  And opening aurora again, you'd have the theme back on.  Is this right?

	:Gijs (he/him)
	Comment 21 • 9 years ago

Yeah, AIUI sticky_pref *should* be used here regardless of the branch/ifdef.

	Brian Grinstead [:bgrins] Assignee
	Comment 22 • 9 years ago

Preemptively copying over r+ even though I've changed it to sticky_pref.  Awaiting feedback from Mark before landing though.

Try push on Aurora:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f48645b3eb5
Try push on fx-team: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0352811effc9

	Mark Hammond [:markh] [:mhammond]
	Comment 23 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #20)
> sticky_pref for the new one as well.  Mark, if we have a sticky_pref for the
> aurora channel, I'm assuming that the matching pref for non-aurora channels
> should also be sticky?

Yep - it should be sticky everywhere or nowhere.

	Brian Grinstead [:bgrins] Assignee
	Comment 24 • 9 years ago

remote:   https://hg.mozilla.org/integration/fx-team/rev/b46196bd9469

	Brian Grinstead [:bgrins] Assignee
	Comment 25 • 9 years ago

Comment on attachment 8605879 [details] [diff] [review]
selectedtheme-id.patch

Approval Request Comment
[Feature/regressing bug #]: 1148996
[User impact if declined]: On Aurora, the Dev Edition theme will be reapplied after a browser restart, even if the user has disabled it
[Describe test coverage new/current, TreeHerder]: The lightweight theming system has existing test coverage (it just doesn't handle browser restarts which is what triggered this bug).
[Risks and why]: Changing the default lightweight theme pref from undefined to "".  The theming system already gracefully handles this change, but an addon could have assumed this pref didn't exist.  Although this pref was only introduced in 39 (Bug 1094821)
[String/UUID change made/needed]:

	Carsten Book [:Tomcat]
	Comment 26 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b46196bd9469

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fa5d922215a