1180583 - Remove old 'browser.devedition.theme.enabled' pref

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Aris]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150702173756

Steps to reproduce:

Firefox Developer Edition already uses the lw-theme way to handle developer theme. There is no need to keep 'browser.devedition.theme.enabled' pref anymore.

It only makes things harder to distinguish between versions with and without developer theme usage.

Nightly channel does a great job, where this preference got removed a while ago.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2069
This suggests the pref is not cleared on purpose, but I don't know much about it.

Comment 2

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #1)
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> nsBrowserGlue.js#2069
> This suggests the pref is not cleared on purpose, but I don't know much
> about it.

301 bgrins.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Marco Bonardo [::mak] from comment #1)
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> nsBrowserGlue.js#2069
> This suggests the pref is not cleared on purpose, but I don't know much
> about it.

We can remove it now.  I had just left it around in case the lw theme change had problems and had to get backed out, but at this point we should get rid of it.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: deved-theme-pref.patch

Removes the pref from firefox.js and prefs_general.js.  Didn't make any changes to the migration stuff in nsBrowserGlue though since I think we still want to handle migrations in the same way.

Try push on fx-team: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcd25cbfc91b
Try push on dev edition: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7560adba601

Comment 5

[:Gijs (he/him)]

Comment on attachment 8631866 [details] [diff] [review]
deved-theme-pref.patch

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

Didn't we also change the talos configuration? Probably want to update that too if we haven't already...

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: deved-theme-talos.patch [checked-in]

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8631866 [details] [diff] [review]
> deved-theme-pref.patch
> 
> Review of attachment 8631866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Didn't we also change the talos configuration? Probably want to update that
> too if we haven't already...

Good point.  Joel, here is a patch to remove this unused pref from the talos config.  What do you think?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8632113 [details] [diff] [review]
deved-theme-talos.patch [checked-in]

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

great!  Do we need this on all branches, or nightly and let it ride the trains?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Joel Maher (:jmaher) from comment #7)
> Comment on attachment 8632113 [details] [diff] [review]
> deved-theme-talos.patch
> 
> Review of attachment 8632113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> great!  Do we need this on all branches, or nightly and let it ride the
> trains?

Nightly and let it ride the trains would be best

Comment 9

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Created attachment 8632113 [details] [diff] [review]
> deved-theme-talos.patch
> 
> (In reply to :Gijs Kruitbosch from comment #5)
> > Comment on attachment 8631866 [details] [diff] [review]
> > deved-theme-pref.patch
> > 
> > Review of attachment 8631866 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Didn't we also change the talos configuration? Probably want to update that
> > too if we haven't already...
> 
> Good point.  Joel, here is a patch to remove this unused pref from the talos
> config.  What do you think?

What I did wonder is... are we still disabling this theme (by using the relevant lwtheme pref, or something?) on devedition on talos? And if not, I wonder if that's some cheap aurora talos wins... ;-)

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > Created attachment 8632113 [details] [diff] [review]
> > deved-theme-talos.patch
> > 
> > (In reply to :Gijs Kruitbosch from comment #5)
> > > Comment on attachment 8631866 [details] [diff] [review]
> > > deved-theme-pref.patch
> > > 
> > > Review of attachment 8631866 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Didn't we also change the talos configuration? Probably want to update that
> > > too if we haven't already...
> > 
> > Good point.  Joel, here is a patch to remove this unused pref from the talos
> > config.  What do you think?
> 
> What I did wonder is... are we still disabling this theme (by using the
> relevant lwtheme pref, or something?) on devedition on talos? And if not, I
> wonder if that's some cheap aurora talos wins... ;-)

We have 'lightweightThemes.selectedThemeID' set to "" in http://hg.mozilla.org/build/talos/file/b4b41ebeec41/talos/PerfConfigurator.py#l447 so it should be already be unapplied.

Comment 11

[Brian Grinstead [:bgrins]]

checkin-needed for Attachment 8631866 [details] [diff]

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: devtheme_talos.patch, update talos.json to pick up latest talos bits

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

landed talos bits:
http://hg.mozilla.org/build/talos/rev/deac3ce69268

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8632144 [details] [diff] [review]
devtheme_talos.patch, update talos.json to pick up latest talos bits

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

LGTM

Comment 15

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/eefff310415b
remote:   https://hg.mozilla.org/integration/fx-team/rev/1cb8ddecbcff

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eefff310415b
https://hg.mozilla.org/mozilla-central/rev/1cb8ddecbcff

Comment 17

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 42.0a1 (2015-07-06) on ubuntu 14.04 LTS, 32 bit!

Build ID: 20150706030206
User Agent: Mozilla/5.0 (X11; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0

The bug's fix is now verified on Latest Beta 42.0b2(2015-09-28)!

Build ID: 20150928102225
User Agent: Mozilla/5.0 (X11; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0

[bugday-20151014]

Comment 18

[Tanvir Rahman]

I have reproduced this bug with aurora 41.0a2 (2015/07/06) on Windows 10, 64 bit!

The Bug's fix is now verified Beta 42.0b9

Build ID 	20151022152545
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0

[bugday-20160720]