1403682 - Convert configuration CSS selectors to pixel regions

Status: RESOLVED FIXED

(Testing :: mozscreenshots, enhancement)

Description

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

Bug 1403680 will provide CSS selectors for each region.

This bug should convert the CSS selectors to rectangles in pixel units relative to the window frame.

This should live in browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm

We should also expand the calculated rectangle by 10px on each side so as to include any shadows or other border effects.

This should be implemented along with automated tests that confirm the region calculated matches our expectations. The tests for this should live in a new file in the browser/tools/mozscreenshots directory (must start with `browser_`).

Comment 1

[Michael Williams]

Attachment: Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

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

Comment 2

[Michael Williams]

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

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

Comment 3

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

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/185916/#review190904

Making good progress, thanks for letting me see where you're at.

::: browser/tools/mozscreenshots/browser_boundingbox.js:10
(Diff revision 2)
> +"use strict";
> +
> +add_task(async function capture() {
> +  let rect = TestRunner._findBoundingBox("#tabbrowser-tabs");
> +  // Check width calculation on simple example
> +  ok(true, rect.width == Services.wm.getMostRecentWindow("navigator:browser").document.querySelector("#tabbrowser-tabs").getBoundingClientRect().width + 20);

Instead of using ok(true, a == b), use is(a, b).

Also, both ok() and is() take a third argument which is a message that will be printed. It is very useful to help describe what and why the assertion exists.

For example,
is(a.width, b.width, "The width of `a` should equal that of `b` at start of test");

::: browser/tools/mozscreenshots/browser_boundingbox.js:12
(Diff revision 2)
> +add_task(async function capture() {
> +  let rect = TestRunner._findBoundingBox("#tabbrowser-tabs");
> +  // Check width calculation on simple example
> +  ok(true, rect.width == Services.wm.getMostRecentWindow("navigator:browser").document.querySelector("#tabbrowser-tabs").getBoundingClientRect().width + 20);
> +  // Check height calculation on simple example
> +  ok(true, rect.height == Services.wm.getMostRecentWindow("navigator:browser").document.querySelector("#tabbrowser-tabs").getBoundingClientRect().height + 20);

`window` should exist in the current scope and it will be equal to `Services.wm.getMostRecentWindow("navigator:browser")`.

`document` should exist in the current scope and it will be equal to `window.document`.

JSM files do not have access to `window` or `document`, though the function you write could take `window` or `document` as an optional argument.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:172
(Diff revision 2)
> +  _findBoundingBox(selector) {
> +    // TODO: Add union functionality to take an array of selectors
> +    let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +    let element = browserWindow.document.querySelector(selector);
> +    let rect = browserWindow.document.getBoxObjectFor(element);
> +    return {x: rect.screenX - 10,

You should use Geometry.jsm here to convert the rect that you get from getBoxObjectFor to a Geometry.jsm Rect object.

Then you can use Geometry.jsm's Rect.inflate() method.

The only problem with this is that Rect.inflate takes scalars and here we want to inflate by a fixed amount not a percentage. I don't see any callers to Rect.inflate so we can just change how that function works.

This function should return a Geometry.jsm Rect.

Comment 4

[Michael Williams]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Making good progress, thanks for letting me see where you're at.

Thanks for the feedback, I'll get right on it.

Comment 5

[Michael Williams]

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

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

Comment 6

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

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/185916/#review191206

Looks good, I think this is really close. After you make these changes this will probably be r+.

::: browser/tools/mozscreenshots/browser_boundingbox.js: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/. */

As you are the author of this test you can decide which license you want to apply. MPL2 is fine, but we normally use Public Domain.

See https://www.mozilla.org/en-US/MPL/license-policy/ for more information.

The public domain header is as follows:

Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/

::: browser/tools/mozscreenshots/browser_boundingbox.js:9
(Diff revision 3)
> +
> +"use strict";
> +
> +add_task(async function capture() {
> +  let rect = TestRunner._findBoundingBox(["#tabbrowser-tabs"]);
> +  let tabBar = window.document.querySelector("#tabbrowser-tabs").getBoundingClientRect();

`document` is a global on its own (also accessible from `window.document`), so this and other places can just be `document`.

::: browser/tools/mozscreenshots/browser_boundingbox.js:15
(Diff revision 3)
> +  // Check width calculation on simple example
> +  is(rect.width, tabBar.width + 20, "Checking _findBoundingBox width calculation");
> +  // Check height calculation on simple example
> +  is(rect.height, tabBar.height + 20, "Checking _findBoundingBox height caclulation");
> +
> +  rect = TestRunner._findBoundingBox(["#forward-button", "#TabsToolbar"]);

Should you add a test case for a selector that doesn't match anything? What should happen there?

::: browser/tools/mozscreenshots/browser_boundingbox.js:24
(Diff revision 3)
> +  let height = Math.max(tabToolbar.bottom, fButton.bottom) - Math.min(tabToolbar.top, fButton.top);
> +  // Check width calculation on simple union
> +  is(rect.width, width + 20, "Checking _findBoundingBox union width calculation");
> +  // Check height calculation on simple union
> +  is(rect.height, height + 20, "Checking _findBoundingBox union height calculation");
> +  // TODO: Add more test cases, specifically testing unions

This todo can be removed since you added a union test  case above.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:165
(Diff revision 3)
>  
>    // helpers
>  
> +  /**
> +  * Calculate the bounding box based on CSS selector from config for cropping
> +  * @param {String[]} selectors - array of CSS selectors for relevant DOM element

nit, please add a blank line between above this.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:173
(Diff revision 3)
> +  _findBoundingBox(selectors) {
> +    // TODO: Testing, see browser_boundingbox.js
> +
> +    // Not sure if we want this check or not
> +    // Added new Rect(0, 0, 0, 0) to get rid of lint error
> +    if (selectors.length == 0) {

Yeah, this is a good check.

You can simplify this to just `if (!selectors.length) {`

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:181
(Diff revision 3)
> +    for (let i = 1; i < selectors.length; i++) {
> +      let newElement = browserWindow.document.querySelector(selectors[i]);

Since you don't use `i` on its own, you can change this loop to a `for..of` loop.

`for (let selector of selectors) { }`

Comment 7

[Michael Williams]

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

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

Comment 8

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

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/185916/#review192008

Looks good, we can land this once you make the following change.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:172
(Diff revisions 3 - 4)
> -
> -    // Not sure if we want this check or not
> -    // Added new Rect(0, 0, 0, 0) to get rid of lint error
> -    if (selectors.length == 0) {
>        log.info("_findBoundingBox: selectors argument is empty");
>        return new Rect(0, 0, 0, 0);

Since the other error condition (selector not found) returns null, we should just make this return null too.

Comment 9

[Michael Williams]

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185916/diff/4-5/

Comment 10

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

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/185916/#review192402

This patch should be updated to use the selectors that Mike is implemented in bug 1403680.

Comment 11

[Michael Williams]

Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185916/diff/5-6/

Comment 12

[Michael Williams]

Attachment: Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

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

Comment 13

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

Comment on attachment 8918660 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/189484/#review196874

r+ with the following changes made.

::: browser/tools/mozscreenshots/browser_boundingbox.js:7
(Diff revision 1)
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +"use strict";
> +
> +add_task(async function capture() {

the function name here seems to be holdover, and not actually related to the test. you can remove the test name and just make this anonymous.

add_task(async function() {
  ...
});

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:214
(Diff revision 1)
> +        finalRect = finalRect.union(newRect);
> +      }
> +    }
> +
> +    // Add fixed padding
> +    finalRect = finalRect.inflateFixed(10 * scale);

This constant (`10`) should be moved to a member variable, then the test can read the value.

The test currently makes an assumption about what the padding will be, and just reading the test it's not clear why the function adds 20 to the width.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:216
(Diff revision 1)
> +    let finalX = browserWindow.screenX * scale;
> +    let finalY = browserWindow.screenY * scale;
> +    let finalW = browserWindow.outerWidth * scale;
> +    let finalH = browserWindow.outerHeight * scale;

These variable names are a bit confusing compared to finalRect. It would seem that finalRect.left would be synonymous with finalRect.x.

I think better names for these would be screenLeft, screenTop, screenWidth, screenHeight.

Comment 14

[Michael Williams]

Comment on attachment 8918660 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51fded9c47aa
Add _findBoundingBox function and tests for mozscreenshots cropping; r=jaws

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/51fded9c47aa

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

M-e10s(ss) fails now on mozilla-central (doesn't run on autoland):

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=25bc10affd99b854d971b27f3111050616ab22d5&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=ss

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139152184&repo=mozilla-central

[task 2017-10-24T10:52:28.390Z] 10:52:28     INFO - TEST-START | browser/tools/mozscreenshots/browser_boundingbox.js
[task 2017-10-24T10:52:28.536Z] 10:52:28     INFO - TEST-INFO | started process screentopng
[task 2017-10-24T10:52:29.388Z] 10:52:29     INFO - TEST-INFO | screentopng: exit 0
[task 2017-10-24T10:52:29.388Z] 10:52:29     INFO - Buffered messages logged at 10:52:28
[task 2017-10-24T10:52:29.388Z] 10:52:29     INFO - Entering test bound setup
[task 2017-10-24T10:52:29.389Z] 10:52:29     INFO - installing extension temporarily
[task 2017-10-24T10:52:29.389Z] 10:52:29     INFO - Checking for mozscreenshots extension
[task 2017-10-24T10:52:29.389Z] 10:52:29     INFO - TEST-PASS | browser/tools/mozscreenshots/browser_boundingbox.js | The mozscreenshots extension should be installed - 
[task 2017-10-24T10:52:29.390Z] 10:52:29     INFO - Leaving test bound setup
[task 2017-10-24T10:52:29.394Z] 10:52:29     INFO - Entering test bound 
[task 2017-10-24T10:52:29.397Z] 10:52:29     INFO - Buffered messages finished
[task 2017-10-24T10:52:29.399Z] 10:52:29     INFO - TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/browser_boundingbox.js | Checking _findBoundingBox width calculation - Got 1280, expected 1300
[task 2017-10-24T10:52:29.401Z] 10:52:29     INFO - Stack trace:
[task 2017-10-24T10:52:29.402Z] 10:52:29     INFO -     chrome://mochikit/content/browser-test.js:test_is:1011
[task 2017-10-24T10:52:29.404Z] 10:52:29     INFO -     chrome://mochitests/content/browser/browser/tools/mozscreenshots/browser_boundingbox.js:null:11
[task 2017-10-24T10:52:29.406Z] 10:52:29     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
[task 2017-10-24T10:52:29.408Z] 10:52:29     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
[task 2017-10-24T10:52:29.411Z] 10:52:29     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

Related changesets:
https://hg.mozilla.org/mozilla-central/rev/51fded9c47aa
https://hg.mozilla.org/mozilla-central/rev/b46909f2b668
https://hg.mozilla.org/mozilla-central/rev/e91ca49ccf6c

Jared, can you care about a fix or let me know what revisions need to be backed out? Thanks.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-screenshots' browser/tools/mozscreenshots/browser_boundingbox.js on mozilla-central:

https://hg.mozilla.org/mozilla-central/rev/967c95cee709756596860ed2a3e6ac06ea3a053f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=a80d568a417ea8410cd2d874c1e0267fb92888fe&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139152799&repo=mozilla-central

[task 2017-10-24T10:54:38.352Z] 10:54:38     INFO - TEST-START | browser/tools/mozscreenshots/browser_boundingbox.js
[task 2017-10-24T10:54:38.477Z] 10:54:38     INFO - TEST-INFO | started process screentopng
[task 2017-10-24T10:54:39.270Z] 10:54:39     INFO - TEST-INFO | screentopng: exit 0
[task 2017-10-24T10:54:39.274Z] 10:54:39     INFO - Buffered messages logged at 10:54:38
[task 2017-10-24T10:54:39.276Z] 10:54:39     INFO - Entering test bound setup
[task 2017-10-24T10:54:39.279Z] 10:54:39     INFO - installing extension temporarily
[task 2017-10-24T10:54:39.284Z] 10:54:39     INFO - Checking for mozscreenshots extension
[task 2017-10-24T10:54:39.287Z] 10:54:39     INFO - TEST-PASS | browser/tools/mozscreenshots/browser_boundingbox.js | The mozscreenshots extension should be installed - 
[task 2017-10-24T10:54:39.297Z] 10:54:39     INFO - Leaving test bound setup
[task 2017-10-24T10:54:39.299Z] 10:54:39     INFO - Entering test bound 
[task 2017-10-24T10:54:39.299Z] 10:54:39     INFO - Buffered messages finished
[task 2017-10-24T10:54:39.300Z] 10:54:39     INFO - TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/browser_boundingbox.js | Checking _findBoundingBox width calculation - Got 1280, expected 1300
[task 2017-10-24T10:54:39.301Z] 10:54:39     INFO - Stack trace:
[task 2017-10-24T10:54:39.301Z] 10:54:39     INFO -     chrome://mochikit/content/browser-test.js:test_is:1011
[task 2017-10-24T10:54:39.301Z] 10:54:39     INFO -     chrome://mochitests/content/browser/browser/tools/mozscreenshots/browser_boundingbox.js:null:11
[task 2017-10-24T10:54:39.303Z] 10:54:39     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
[task 2017-10-24T10:54:39.303Z] 10:54:39     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
[task 2017-10-24T10:54:39.304Z] 10:54:39     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

Comment 19

[Michael Williams]

Comment on attachment 8918660 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5be34ec792e
Add _findBoundingBox function and tests for mozscreenshots cropping; r=jaws

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d5be34ec792e