Closed Bug 1180583 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Theme, defect)

41 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox42 --- verified

People

(Reporter: aris-addons, Assigned: bgrins)

References

Details

Attachments

(3 files)

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.
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.
Component: Untriaged → Theme
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bgrinstead)
(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.
Assignee: nobody → bgrinstead
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bgrinstead)
Summary: Developer Edition enables 'browser.devedition.theme.enabled' pref by mistake → Remove old 'browser.devedition.theme.enabled' pref
See Also: → 1181440
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
Attachment #8631866 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #8631866 - Flags: review?(gijskruitbosch+bugs) → review+
(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?
Attachment #8632113 - Flags: review?(jmaher)
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?
Attachment #8632113 - Flags: review?(jmaher) → review+
(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
(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... ;-)
(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.
checkin-needed for Attachment 8631866 [details] [diff]
Keywords: checkin-needed
Attachment #8632113 - Attachment description: deved-theme-talos.patch → deved-theme-talos.patch [checked-in]
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
Attachment #8632144 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/eefff310415b
https://hg.mozilla.org/mozilla-central/rev/1cb8ddecbcff
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1193204
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]
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.