Open Bug 1398730 Opened 7 years ago Updated 2 years ago

Should have a configuration for private windows

Categories

(Testing :: mozscreenshots, enhancement, P2)

Product:
Testing
Component:
mozscreenshots
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: Gijs, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1398730 - Add PrivateBrowsing config with support in TestRunner, work in progress;
59 bytes, text/x-review-board-request
Details 
We're missing private-window-specific regressions, which is a shame.
Priority: -- → P2
Assignee: nobody → will2616
Status: NEW → ASSIGNED
Attachment #8911656 - Flags: review?(jaws) → review?(mconley)
Note that I added a hack to avoid showing the automation UI indicators here: https://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm#100

It relies on the fact that we only open one window in all mozscreenshots configs so far. So your new private browsing window will probably have the automation indicators again.

You can either apply my patch to your windows or find a cleaner solution to fix this :)
(In reply to Johann Hofmann [:johannh] from comment #3)
> Note that I added a hack to avoid showing the automation UI indicators here:
> https://searchfox.org/mozilla-central/rev/
> 3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/tools/mozscreenshots/
> mozscreenshots/extension/TestRunner.jsm#100
> 
> It relies on the fact that we only open one window in all mozscreenshots
> configs so far. So your new private browsing window will probably have the
> automation indicators again.
> 
> You can either apply my patch to your windows or find a cleaner solution to
> fix this :)

I added an update for this, thanks!
Comment on attachment 8911656 [details]
Bug 1398730 - Add private browsing to mozscreenshots config, fixed lint issues, leaves windows open at end;

https://reviewboard.mozilla.org/r/183056/#review191146

So I finally sorted out what's going on here. I _suspect_ that Mochitest (the thing that's running this screenshot test suite) doesn't really like it if you close the original browser window, and that's what calling `browserWindow.close()` does.

Hey MattN, do you have a sense of how to proceed here? We want to add a PB state here, I guess - but it doesn't look to me like TestRunner.jsm is really prepared to deal with multiple windows - or at least, cleaning up windows that might have opened during the course of the test. Is that something we should add?
Attachment #8911656 - Flags: review?(mconley)
Flags: needinfo?(MattN+bmo)
Not sure if this answers your question but the way it worked before is that the PB window would open on top of the existing window (i.e. don't close it).

Most existing configurations were already made to get the most recent window and act on it (not filtering by private/not-private) so they should do the right thing. 

I think you could add cleanup to TestRunner.cleanup? It's not ideal but maybe it's good enough for now?
https://dxr.mozilla.org/mozilla-central/rev/aa958b29c149a67fce772f8473e9586e71fbdb46/browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm#186
Flags: needinfo?(MattN+bmo)
Attachment #8911656 - Attachment is obsolete: true
Comment on attachment 8927041 [details]
Bug 1398730 - Add PrivateBrowsing config with support in TestRunner, work in progress;

https://reviewboard.mozilla.org/r/198252/#review203876

We discussed an alternative approach in today's meeting.
Attachment #8927041 - Flags: review?(mconley)
Comment on attachment 8927041 [details]
Bug 1398730 - Add PrivateBrowsing config with support in TestRunner, work in progress;

https://reviewboard.mozilla.org/r/198252/#review204182

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:92
(Diff revision 3)
> +    // Store reference to browserWindow for configurations
> +    this.windows = {
> +      browserWindow
> +    };

We've already got `browserWindow` - I'm not sure why we need `this.windows.browserWindow`...

Could we not open the PrivateBrowsing window here instead? And then have a reference to it?

I feel like if we checked for PrivateBrowsing in the `sets` Array, we could contain all of the "we're using private browsing windows" state in here, and not have to attach additional properties onto the TestRunner object.

I understand why you want to have the windows be attached to TestRunner for clean-up, but perhaps instead, we could have a "windowsToClose" array, and _just_ add the PrivateBrowsing window to that iff we open one.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:122
(Diff revision 3)
> +      if (this.private) {
> +        await this._performCombo(this.combos.item(this.currentComboIndex),
> +                                 this.windows.privateWindow);
> +      }

Do we not need to restore and focus this window, and minimize the other window here?

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:161
(Diff revision 3)
> +      // Check for PrivateBrowsing set name and open private window
> +      // Store reference to both windows for configurations to access
> +      if (setName == "PrivateBrowsing") {
> +        this.private = true;
> +        this.windows.privateWindow = this.windows.browserWindow.OpenBrowserWindow({private: true});
> +      }
> +

This could probably be done in `start`, no?

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PrivateBrowsing.jsm:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

I don't understand... why is this file necessary?
Attachment #8927041 - Flags: review?(mconley)
Comment on attachment 8927041 [details]
Bug 1398730 - Add PrivateBrowsing config with support in TestRunner, work in progress;

https://reviewboard.mozilla.org/r/198252/#review206144

Thanks Mike! See below.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:47
(Diff revision 4)
>    completedCombos: 0,
>    currentComboIndex: 0,
>    _lastCombo: null,
>    _libDir: null,
> +  windowsToClose: [],
> +  private: false,

What is this used for?

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:118
(Diff revision 4)
> +    for (let setName of setNames) {
> +      if (setName == "PrivateBrowsing") {
> +        this.private = true;
> +        break;
> +      }
> +    }

`setNames` is an Array - so I think you can do:

```
this.private = setNames.includes("PrivateBrowsing");
```

But why is it needed? I don't think `this.private` is used anywhere except below - so you could just have it be a local variable.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:137
(Diff revision 4)
> +
> +      for (let i = 0; i < this.combos.length; i++) {
> +        this.currentComboIndex = i;
> +        await this._performCombo(this.combos.item(this.currentComboIndex),
> +                                 privateWindow);
> +      }

We should restore and focus the browserWindow here.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:377
(Diff revision 4)
> -      let filename = padLeft(this.currentComboIndex + 1,
> +      let filename;
> +      // If the window is private, add private tag to filename
> +      // Else provide filename like normal
> +      if (PrivateBrowsingUtils.isWindowPrivate(window)) {
> +        filename = padLeft(this.currentComboIndex + 1,
> +                           String(this.combos.length).length) + "_private" + this._comboName(combo);

This should be `_PrivateBrowsing`.

Perhaps we could put it at the end - that way, we don't have to compute most of this stuff twice - can just append it on the end if the window is a private one.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PrivateBrowsing.jsm:21
(Diff revision 4)
> +// conventions in TestRunner. Additionally, this ensures that the correct
> +// number of sets of screenshots are generated.
> +
> +// TODO: I believe I need to add minimize/maximize in here to make sure that
> +// the correct window is captured
> +this.PrivateBrowsing = {

Wait - so can we just get rid of this file now?

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm:25
(Diff revision 4)
>    init(libDir) {},
>  
>    configurations: {
>      fiveTabs: {
>        selectors: ["#tabbrowser-tabs"],
> -      async applyConfig() {
> +      async applyConfig(browserWindow) {

Aren't there other configs that need to take this argument and use?
Attachment #8927041 - Flags: review?(mconley) → review-
Comment on attachment 8927041 [details]
Bug 1398730 - Add PrivateBrowsing config with support in TestRunner, work in progress;

https://reviewboard.mozilla.org/r/198252/#review207440

Looking good! This is shaping up nicely. See below.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:118
(Diff revision 5)
> +                               browserWindow);
> +    }
> +
> +    // If PrivateBrowsing is specified, run each config again on a private window
> +    if (setNames.includes("PrivateBrowsing")) {
> +      // TODO: set up prefs on window like above (line 90)

Prefs are global, and aren't set window to window. If the private browsing window depends on the prefs that were changed on 90-92, then it'll see the changes too.

So I think we can remove this comment.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:125
(Diff revision 5)
> +
> +      // Add privateWindow to windowsToClose for cleanup
> +      this.windowsToClose.push(privateWindow);
> +      // Minimize browserWindow and focus privateWindow
> +      browserWindow.minimize();
> +      privateWindow.restore();

Is it necessary to restore this window? It was just opened, so I think it should already be restored.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:133
(Diff revision 5)
> +      for (let i = 0; i < this.combos.length; i++) {
> +        this.currentComboIndex = i;
> +        await this._performCombo(this.combos.item(this.currentComboIndex),
> +                                 privateWindow);
> +      }
> +

Y'know, instead of having `windowsToClose`, we could just close the privateWindow right here. I don't think there's any reason to keep it around longer, is there?

That way, we can get rid of `windowsToClose`, and don't need to touch `cleanup`.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:172
(Diff revision 5)
> +      // Don't load PrivateBrowsing to sets, TestRunner handles any necessary
> +      // work for PrivateBrowsing config

Good commentary here. Thanks for that.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm:25
(Diff revision 5)
>    init(libDir) {},
>  
>    configurations: {
>      fiveTabs: {
>        selectors: ["#tabbrowser-tabs"],
> -      async applyConfig() {
> +      async applyConfig(browserWindow) {

There are other configurations that we need to update to use the passed in `browserWindow`, right?

Like I'm pretty sure almost all of them need updating, right?

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm:26
(Diff revision 5)
>  
>    configurations: {
>      fiveTabs: {
>        selectors: ["#tabbrowser-tabs"],
> -      async applyConfig() {
> +      async applyConfig(browserWindow) {
>          fiveTabsHelper();

Looking at the source of `fiveTabsHelper`, it looks like it's also trying to use getMostRecentWindow. Can we pass the browserWindow to it instead?

Basically, look at each configuration, and ese where it uses getMostRecentWindow, and use the passed in window instead.
Attachment #8927041 - Flags: review?(mconley) → review-
Comment on attachment 8927041 [details]
Bug 1398730 - Add PrivateBrowsing config with support in TestRunner, work in progress;

https://reviewboard.mozilla.org/r/198252/#review207458

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm:25
(Diff revision 5)
>    init(libDir) {},
>  
>    configurations: {
>      fiveTabs: {
>        selectors: ["#tabbrowser-tabs"],
> -      async applyConfig() {
> +      async applyConfig(browserWindow) {

Please pass an object to applyConfig with browserWindow being one property and use destructuring to access the property. The reason is because there are other things that I'd like to pass to configurations and they're all optional so positional arguments aren't a good fit.
Example:

```js
async applyConfig({browserWindow}) {
```
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #18)

> ```js
> async applyConfig({browserWindow}) {
> ```

Pretty sure I have it they way you want in the latest revision, let me know if it needs any further tweaks.
Comment on attachment 8927041 [details]
Bug 1398730 - Add PrivateBrowsing config with support in TestRunner, work in progress;

https://reviewboard.mozilla.org/r/198252/#review209440

Looks good! Sounds like you're really close here - just need to figure out a few more configurations.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Buttons.jsm:14
(Diff revision 6)
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource:///modules/CustomizableUI.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +// TODO: Handle init

We discussed this in IRC - the createWidget stuff that adds the stylesheet will need to get factored out and used on the passed in browser window.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/CustomizeMode.jsm:40
(Diff revision 6)
> +    // TODO: Private window gets stuck here on customizing...
> +    // Pops browser window back to the front then stalls

That's very strange...

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/DevTools.jsm:24
(Diff revision 6)
> +// When this function is called in TestRunner (line 253), does it
> +// return for the most recent window or what?

Yeah, it's using the most recent window via `getTargetForSelectedTab` - see line 19.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm:32
(Diff revision 6)
> +        // How does LightweightThemeManager manager work?
> +        // it worked right out of the box without needing browserWindow

I believe the LightweightThemeManager.jsm does the work of iterating and modifying each browser window when an update occurs.

(These comments can now be removed)

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Preferences.jsm:40
(Diff revision 6)
>        if (customFn) {
>          configName += "-" + customFn.name;
>        }
>        this.configurations[configName] = {};
>        this.configurations[configName].selectors = ["#browser"];
>        this.configurations[configName].applyConfig = prefHelper.bind(null, primary, customFn);

These will be accepting `{ browserWindow }` as their first argument though, right?

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm:147
(Diff revision 6)
>      gBrowser.unpinTab(gBrowser.selectedTab);
>    let newTabButton = browserWindow.document.getAnonymousElementByAttribute(browserWindow.gBrowser.tabContainer, "class", "tabs-newtab-button toolbarbutton-1");
>    hoverTab(newTabButton, false);
>  }
>  
> +// TODO: test skips fourPinned config, saying error on line 152

What kind of error? Does this test not get run without your changes?
Attachment #8927041 - Flags: review?(mconley)

The bug assignee didn't login in Bugzilla in the last 7 months.
:MattN, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: will2616 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mozilla+bmo)
Flags: needinfo?(mozilla+bmo)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: