Open Bug 1294568 Opened 3 years ago Updated 2 years ago

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

Categories

(Testing :: mozscreenshots, defect, P3)

All
Windows
defect

Tracking

(firefox51 affected)

Tracking Status
firefox51 --- affected

People

(Reporter: MattN, Unassigned)

References

Details

Attachments

(1 file)

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
Component: General → mozscreenshots
Product: Firefox → Testing
Version: Trunk → unspecified
Assignee: nobody → rndmustafa
Status: NEW → ASSIGNED
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
Attachment #8929574 - Flags: review?(jaws) → review-
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.
Attachment #8929574 - Flags: review?(jaws) → review-
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.
Assignee: rndmustafa → nobody
Status: ASSIGNED → NEW
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.
Attachment #8929574 - Flags: review?(mconley)
You need to log in before you can comment on or make changes to this bug.