All users were logged out of Bugzilla on October 13th, 2018

The method 'setTheme' in shared/theme.js doesn't trigger a live theme change

RESOLVED FIXED in Firefox 41

Status

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: sjakthol, Assigned: sjakthol)

Tracking

unspecified
Firefox 41

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
STR:
1. Open the Developer Tools
2. Open browser console.
3. Type following commands to the browser console:
 let { devtools: { require } } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
 let { setTheme } = require("devtools/shared/theme");
 setTheme("dark");
 // OR setTheme("light") if you are using the dark theme

Expected results: The theme changes for the active toolboxes.
Actual results: The theme is not changed until you reopen the toolbox.

The reason for this problem is at [1] which updates the pref first and then reads it again to the oldValue field. Then at [2] the theme switching returns early as the old value was incorrectly set to the the new value in [1].

A fixed version of this method has already crept to few places around the tree [3], [4].

[1] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme.js#92
[2] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-switching.js#24

[3] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/performance/test/browser_perf-theme-toggle-01.js#79-88
[4] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webaudioeditor/test/browser_wa_graph-markers.js#66-75
(Assignee)

Comment 1

3 years ago
If bug 1111047 is fixed with the way suggested there, this problem will most likely go away.
Depends on: 1111047
(Assignee)

Comment 2

3 years ago
Created attachment 8613862 [details] [diff] [review]
bug-1169993-correct-old-value.patch

Here's a patch that fixes this particular problem. Fixing bug 1111047 would fix this too but this is a lot easier for now.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e28d053db87
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8613862 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
No longer depends on: 1111047
See Also: → bug 1111047
(Assignee)

Updated

3 years ago
Blocks: 1119149
Comment on attachment 8613862 [details] [diff] [review]
bug-1169993-correct-old-value.patch

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

Looks good, thanks
Attachment #8613862 - Flags: review?(bgrinstead) → review+
(In reply to Sami Jaktholm from comment #0)
> [3]
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/performance/
> test/browser_perf-theme-toggle-01.js#79-88
> [4]
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/
> webaudioeditor/test/browser_wa_graph-markers.js#66-75

Can you please add a second patch here (or open a follow up bug) to get rid of this duplication by reusing setTheme?
(Assignee)

Comment 5

3 years ago
Created attachment 8614760 [details] [diff] [review]
bug-1169993-use-shared-setTheme.patch

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Can you please add a second patch here (or open a follow up bug) to get rid
> of this duplication by reusing setTheme?

Here's one. Tested locally on an older build so hopefully nothing related has changed in the last few days.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15ff0a9ee16e
Attachment #8614760 - Flags: review?(bgrinstead)
(Assignee)

Comment 6

3 years ago
Here's a better try run with everything rebased on top of latest fx-team: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e719c7b039f
(Assignee)

Comment 7

3 years ago
Ugh, third time is the charm (the previous was missing my patches :( ): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab088ed8c5a7
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: leave-open
Comment on attachment 8614760 [details] [diff] [review]
bug-1169993-use-shared-setTheme.patch

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

Nice
Attachment #8614760 - Flags: review?(bgrinstead) → review+
Don't think we need leave-open since both patches look ready to go once we have a green try push
Keywords: leave-open
(Assignee)

Comment 10

3 years ago
The bustage in the try run seems to be resolved already in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=70fa9da159a0.
https://hg.mozilla.org/mozilla-central/rev/4c65f2781bac
https://hg.mozilla.org/mozilla-central/rev/42741c021cc7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.