1403677 - [meta] Implement support for cropping of screenshots

Status: NEW

(Testing :: mozscreenshots, enhancement)

Description

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

Often times screenshots include too much of the UI. Test A may check the home button, while Test B may check the location bar. A change to the location bar will currently cause both Test A and Test B to fail. This is expensive because a human must verify that the failure related to Test B is acceptable and not unexpected.

If we crop the screenshots to the area that we are concerned about we can drastically reduce false positives.

This is a meta bug to track the work involved here.

Comment 1

[rndmustafa]

Attachment: random change

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

Comment 2

[Robin Miller]

Attachment: Bug 1403677 - Initial implementation of integration.

Can now crop screenshots properly, for basic cases. Still needs work to add support for more complex cases such as DevTools or new windows.

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

Comment 3

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

Comment on attachment 8918598 [details]
Bug 1403677 - Initial implementation of integration.

https://reviewboard.mozilla.org/r/189432/#review196846

These changes should be made to their respective patches in their respective bugs.

Comment 4

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

Comment on attachment 8918598 [details]
Bug 1403677 - Initial implementation of integration.

https://reviewboard.mozilla.org/r/189432/#review196848

::: browser/tools/mozscreenshots/mozscreenshots/extension/Screenshot.jsm:106
(Diff revision 1)
> -        if (windowID && Services.sysinfo.getProperty("version").indexOf("10.") !== 0) {
> -          // Capture only that window on 10.7+
> -          args.push("-l");
> -          args.push(windowID);
> -        }
> +        // if (windowID && Services.sysinfo.getProperty("version").indexOf("10.") !== 0) {
> +        //   // Capture only that window on 10.7+
> +        //   args.push("-l");
> +        //   args.push(windowID);
> +        // }

Why is this commented out?

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:266
(Diff revision 1)
>        return;
>      }
>  
> -    await this._onConfigurationReady(combo);
> +    // TODO: account for multiple windowTypes
> +    // Collect selectors from combo configs for cropping region
> +    let windowType = undefined;

You don't have to explicitly intialize to undefined.

`let windowType;` will work the same.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:269
(Diff revision 1)
> +      if (windowType) {
> +        windowType = obj.windowType;

Should this be:

if (obj.windowType) {
  windowType = obj.windowType;
}

as is it will only overwrite windowType if it is already defined.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:272
(Diff revision 1)
> +      for (let select of obj.selectors) {
> +        finalSelectors.push(select);

rename `select` to `selector`

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:277
(Diff revision 1)
> +      for (let select of obj.selectors) {
> +        finalSelectors.push(select);
> +      }
> +    }
> +
> +    let rect = this._findBoundingBox(finalSelectors, windowType);

Does it really make sense to use the last specified windowType? That seems like it may be confusing to future test authors.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/DevTools.jsm:26
(Diff revision 1)
> -    let panels = ["options", "webconsole", "jsdebugger", "styleeditor",
> -                  "performance", "netmonitor"];
> +    // let panels = ["options", "webconsole", "jsdebugger", "styleeditor",
> +    //               "performance", "netmonitor"];

If this code is broken it should either be deleted from the tree or we should find a way to not run it by default. We shouldn't check in large swaths of commented-out code.