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

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Sami Jaktholm]

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

Comment 1

[Sami Jaktholm]

If bug 1111047 is fixed with the way suggested there, this problem will most likely go away.

Comment 2

[Sami Jaktholm]

Attachment: 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

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8613862 [details] [diff] [review]
bug-1169993-correct-old-value.patch

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

Looks good, thanks

Comment 4

[Brian Grinstead [:bgrins]]

(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?

Comment 5

[Sami Jaktholm]

Attachment: 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

Comment 6

[Sami Jaktholm]

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

Comment 7

[Sami Jaktholm]

Ugh, third time is the charm (the previous was missing my patches :( ): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab088ed8c5a7

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8614760 [details] [diff] [review]
bug-1169993-use-shared-setTheme.patch

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

Nice

Comment 9

[Brian Grinstead [:bgrins]]

Don't think we need leave-open since both patches look ready to go once we have a green try push

Comment 10

[Sami Jaktholm]

The bustage in the try run seems to be resolved already in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=70fa9da159a0.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4c65f2781bac
https://hg.mozilla.org/integration/fx-team/rev/42741c021cc7

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/4c65f2781bac
https://hg.mozilla.org/mozilla-central/rev/42741c021cc7