Closed
Bug 1169993
Opened 9 years ago
Closed 9 years ago
The method 'setTheme' in shared/theme.js doesn't trigger a live theme change
Categories
(DevTools :: Framework, defect)
DevTools
Framework
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: sjakthol, Assigned: sjakthol)
References
Details
Attachments
(2 files)
2.40 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
3.37 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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•9 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•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Comment 3•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
(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•9 years ago
|
||
(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•9 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•9 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•9 years ago
|
Keywords: leave-open
Comment 8•9 years ago
|
||
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+
Comment 9•9 years ago
|
||
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•9 years ago
|
||
The bustage in the try run seems to be resolved already in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=70fa9da159a0.
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4c65f2781bac https://hg.mozilla.org/integration/fx-team/rev/42741c021cc7
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c65f2781bac https://hg.mozilla.org/mozilla-central/rev/42741c021cc7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•