1294568 - Support changing the Windows system theme (e.g. to high contrast mode) in mozscreenshots for automation

Status: NEW

(Testing :: mozscreenshots, defect, P3)

Description

[Matthew N. [:MattN]]

mozscreenshots has support for changing the system theme in the SystemTheme.jsm configuration[1] on Windows but it wasn't checked in to mozilla-central since we need to ensure we never leave the machine in a non-default theme for any reason or we will end up causing failures for other test suites run later on that machine (e.g. reftests). We also don't want to leave the display properties window open as that may interfere with future screenshots.

If we can ensure that we won't leave the machines in an undesired state, it should be as simple as adding SystemTheme.jsm to /browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ (possibly updating it for Win10) and adding the .bat[2] and .vbs[3] dependencies to /browser/tools/mozscreenshots/mozscreenshots/extension/lib/. All the code could likely use some cleanup as it wasn't written for m-c standards initially.

Some ideas to ensure we revert to the original theme:
a) Use SimpleTest.registerCleanupFunction to revert the theme
** I'm not sure how reliable the theme changing code is since changing back to the original theme is going to be just as unreliable as changing themes in the first place. Changing Windows themes relies on various OS APIs/registry keys that may be fragile and poorly documented.
b) Have a way to indicated to automation that the machine/account should be reset to a fresh state.
** For example, output something in the log which triggers a re-imaging. I think we had something like this before to indicate a reboot was needed.
** Perhaps this is easier with TaskCluster (TC) but the screenshot jobs don't run on TC yet.

[1] https://github.com/mnoorenberghe/mozscreenshots/blob/master/mozscreenshots/extension/configurations/SystemTheme.jsm
[2] https://github.com/mnoorenberghe/mozscreenshots/blob/master/mozscreenshots/extension/lib/ChangeWindowsTheme.bat
[3] https://github.com/mnoorenberghe/mozscreenshots/blob/master/mozscreenshots/extension/lib/CloseDisplayProperties.vbs

Comment 1

[rndmustafa]

Attachment: Bug 1294568 - Support changing the Windows system theme (e.g. to high contrast mode) in mozscreenshots for automation .

Review commit: https://reviewboard.mozilla.org/r/200846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200846/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8929574 [details]
Bug 1294568 - Support changing the Windows system theme (e.g. to high contrast mode) in mozscreenshots for automation .

https://reviewboard.mozilla.org/r/200846/#review206118

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/SystemTheme.jsm:24
(Diff revision 1)
> +       let env = Cc["@mozilla.org/process/environment;1"]
> +                   .getService(Ci.nsIEnvironment);
> +       let resourcesDir = env.get("SystemRoot") + "\\Resources";
> +       let winVersion = getWindowsVersion();
> +
> +       if (winVersion.majorVersion == 10) { // Windows 10

Much of this can be replaced with AppConstants.isPlatformAndVersionAtLeast()

https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/modules/AppConstants.jsm#157-161

Here are examples of use: https://searchfox.org/mozilla-central/search?q=isPlatformAndVersionAtLeast%28%22win&path=

As long as you check for 10 first, then lower versions you can use the *AtLeast method.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/SystemTheme.jsm:85
(Diff revision 1)
> +       }
> +     } else {
> +       this.configurations = {
> +         defaultTheme: {
> +           async applyConfig() {
> +             Promise.resolve(); // This is here so the default set works on all OSs.

You can delete this line. async functions by definition will always return a promise even if not explicitly returned. That empty promise will be resolved unless the async function throws an exception.

So an empty async function here should work.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/SystemTheme.jsm:108
(Diff revision 1)
> +       setTimeout(function focusTab() {
> +         let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +         browserWindow.gBrowser.selectedBrowser.focus();
> +         // TODO: poll registry for theme value to know when ready? or does it get set right away while the modal is up?
> +       }, 5000);

I don't know if it will be set right away or not, but you can replace this with TestUtils.waitForCondition.

https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/testing/modules/TestUtils.jsm#116

Then inside of that, you can use WindowsRegistry.readRegKey to see if the value is what you expect.

https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/modules/WindowsRegistry.jsm#26-53

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/SystemTheme.jsm:115
(Diff revision 1)
> +         browserWindow.gBrowser.selectedBrowser.focus();
> +         // TODO: poll registry for theme value to know when ready? or does it get set right away while the modal is up?
> +       }, 5000);
> +     }.bind(this));
> +
> +     return new Promise((c) => setTimeout(c, 5000)); // Need to wait for theme to load

If you're switching to the default theme you could use `TestUtils.waitForCondition()` and the condition you would check would be if `window.matchMedia("(-moz-windows-default-theme)").matches` like we do at https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/browser/base/content/browser.js#1321

Comment 3

[rndmustafa]

Comment on attachment 8929574 [details]
Bug 1294568 - Support changing the Windows system theme (e.g. to high contrast mode) in mozscreenshots for automation .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200846/diff/1-2/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8929574 [details]
Bug 1294568 - Support changing the Windows system theme (e.g. to high contrast mode) in mozscreenshots for automation .

https://reviewboard.mozilla.org/r/200846/#review210284

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/SystemTheme.jsm:128
(Diff revision 2)
> +         browserWindow.gBrowser.selectedBrowser.focus();
> +       }, 5000);
> +     }.bind(this));
> +
> +     // The line below fails to take screenshots, unsure why.
> +     // return TestUtils.waitForCondition(() => getCurrentThemePath() == themeFilePath, "Waiting for theme to load");

Does the condition ever become true? Does the test just time out here?

By default waitForCondition will try every 100ms for 5 seconds, so it _should_ be the same as the setTimeout below.

Can you log what getCurrentThemePath() and themeFilePath are equal to inside of this? It might help you figure out what is going wrong here.

Comment 5

[rndmustafa]

Comment on attachment 8929574 [details]
Bug 1294568 - Support changing the Windows system theme (e.g. to high contrast mode) in mozscreenshots for automation .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200846/diff/2-3/

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Rand let me know that he's getting bogged down with other things and won't be able to finish this one off. He's attached his most recent snapshot of work, and so this free for someone else to carry forward.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8929574 [details]
Bug 1294568 - Support changing the Windows system theme (e.g. to high contrast mode) in mozscreenshots for automation .

https://reviewboard.mozilla.org/r/200846/#review215546

Clearing review until we can get someone else to pick this up.