1193204 - The Developer Edition theme is not set by default on Aurora 42

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Andrei Vaida [:avaida]]

Reproducible on:
* Aurora 42.0a2 (2015-08-10)

Affected platforms:
* Windows 7 (x64)
* Windows 10 (x86, 10240)
* Mac OS X 10.10.4
* Ubuntu 14.04 (x86)

Steps to reproduce:
1. Launch Firefox Developer Edition using a _new_ profile.
2. Check out the default browser theme.

Expected result:
The default theme is set to "Developer Edition", the dark variant.

Actual result:
The default theme is set to "Default" _instead_ of "Developer Edition".

Notes:
* This issue is NOT reproducible with Aurora 41.0a2 (2015-08-10) so it's a recent regression that might have been introduced during the merge.
* It's also worth mentioning that e10s is enabled by default in Aurora 42.0a2.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Important regression which should be fixed before aurora enables are back. Tracking

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Brian, Dave, could you help on this? Thanks

Comment 3

[Brian Grinstead [:bgrins]]

Firstly, great catch!  I am running aurora locally and for some reason #ifdef MOZ_DEV_EDITION is returning false and the theme isn't being preprocessed on.

This will break lots of things beyond just the theme, see https://dxr.mozilla.org/mozilla-central/search?q=MOZ_DEV_EDITION&redirect=false&case=false&limit=60&offset=0

I have no idea why this isn't being defined - it's like the branding changes aren't working.  Can I ask, are you seeing the Dev Edition logo in the task bar when running this build or the Nightly logo?

Comment 4

[Brian Grinstead [:bgrins]]

It looks like the Aurora branding changes are completely broken in a local build.

Downloading from http://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/, the branding is fixed but `lightweightThemes.selectedThemeID` is user set as an empty string upon first startup on a new profile.

Comment 5

[Brian Grinstead [:bgrins]]

OK, so I think this is a UI migration bug, specifically we need to remove the browser.devedition.theme.enabled check when resetting the theme in here: https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2121-2124.  Since that pref has now been removed in Bug 1180583 the migration is automatically removing the theme.

Whatever is going on with the local branding changes in Comments 3 and 4 seem unrelated but will make it hard to test this locally.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: old-migration.patch

99% sure this is the problem, I'm still working on a local environment that I can confirm

Comment 7

[:Gijs (he/him)]

Comment on attachment 8646980 [details] [diff] [review]
old-migration.patch

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

Can we use prefHasUserValue instead so we keep the migration for people who upgrade from 40 to 42 or something? r=me either way, though we'll still need to verify that this fixes it.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8646980 [details] [diff] [review]
> old-migration.patch
> 
> Review of attachment 8646980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we use prefHasUserValue instead so we keep the migration for people who
> upgrade from 40 to 42 or something?

I think it would need to be a migration from 39 to hit this case since we did the original switch to lw theme in 40 for Bug 1148996.  I'm happy to remove any reference to the pref altogether.

> r=me either way, though we'll still need
> to verify that this fixes it.

Just confirmed in a local build with trunk-as-aurora simulation that the current patch fixes the issue.

Comment 9

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > Comment on attachment 8646980 [details] [diff] [review]
> > old-migration.patch
> > 
> > Review of attachment 8646980 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can we use prefHasUserValue instead so we keep the migration for people who
> > upgrade from 40 to 42 or something?
> 
> I think it would need to be a migration from 39 to hit this case since we
> did the original switch to lw theme in 40 for Bug 1148996.  I'm happy to
> remove any reference to the pref altogether.
> 
> > r=me either way, though we'll still need
> > to verify that this fixes it.
> 
> Just confirmed in a local build with trunk-as-aurora simulation that the
> current patch fixes the issue.

Perfect, ship it.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/30f47ec273a9

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Could you fill the uplift request to aurora right now? thanks!

Comment 12

[Brian Grinstead [:bgrins]]

Thanks Andrei for catching this.  Here's an ongoing try push with the patch applied on top of current aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06fc56224f71.

Here's a try push with trunk-as-aurora simulation just in case whatever branding problems I'm seeing in Aurora carry over to the link above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c3315d9db3d.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8646980 [details] [diff] [review]
old-migration.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1180583 
[User impact if declined]: Dev Edition theme won't apply on new profiles on Dev Edition
[Describe test coverage new/current, TreeHerder]: None specifically :(, it's migration code that isn't tested directly AFAIK.
[Risks and why]: Taking this on shortly before aurora builds go out, but at least it's a trivial change.  I have some test pushes running in Comment 12 that can be checked when they finish.
[String/UUID change made/needed]:

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8646980 [details] [diff] [review]
old-migration.patch

Thanks. Please make sure that lands asap :)

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/30f47ec273a9

Comment 16

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ac553d3b93f

Comment 17

[Brian Grinstead [:bgrins]]

Andrei, can you please confirm that the latest aurora builds are applying the theme correctly for new profiles?

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Can I ask, are you seeing the Dev Edition logo in the 
> taskbar when running this build or the Nightly logo?
I do see the Dev Edition logo in the taskbar and all the other branding associated to it, except for the theme. Here's a screenshot from 2015-08-10: http://i.imgur.com/e3fX1vm.png and one from 2015-08-12: http://i.imgur.com/Mc8DIFB.png.

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Andrei, can you please confirm that the latest aurora builds are applying
> the theme correctly for new profiles?
I'll take a look as soon as 2015-08-13 builds of 42.0a2 are available, thanks!

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Andrei, can you please confirm that the latest aurora builds are applying
> the theme correctly for new profiles?

This is verified fixed on Aurora 42.0a2 (2015-08-13), using Windows 7 (x64), Mac OS X 10.10.4, Windows 10  (x86, 10240) and Ubuntu 14.04 (x86). 

The default theme is now set to "Developer Edition", the dark variant. It's also worth mentioning that the version has also been corrected, it's now properly displayed as "42.0a2".