James Graham [:jgraham] Assignee
	Description • 7 years ago

web-platform-tests are running reftests in a way that is inefficient compared to the reftest harness. Instead of comparing screenshots in gecko code and only computing base64 encoded versions of the images when we want to log the result, we are currently creating base64 versions of every screenshot, which adds considerable runtime. Because our implementation is stateless wrt marionette we don't have the oppertunity to cache canvas elements or similar in gecko.

TO solve this issue we can implement a set of marionette commands specifically targetted at running reftests. These will:

* Open a new minimal window for running the reftests, set up so the viewport is of the correct dimensions.
* Provide an endpoint for submitting an entire reftest (i.e. one test plus possibly multiple refs, according to the semantics of wpt).
* Use the same image capture setup as reftests (same flags to drawWindow, same image comparison function)
* Reuse existing canvas elements where possible
* cache whole canvases we expect to reuse (e.g. because a single ref is used for multiple tests).

	Andreas Tolfsen ❲:ato❳
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8866333 [details]
Bug 1363428 - Allow passing in an existing canvas to capture.canvas,

https://reviewboard.mozilla.org/r/137962/#review142152

::: testing/marionette/capture.js:93
(Diff revision 1)
> + * @param {HTMLCanvasElement} canvas
> + *     Optional canvas to reuse for the screenshot

Missing full stop.

::: testing/marionette/capture.js:100
(Diff revision 1)
>   *
>   * @return {HTMLCanvasElement}
>   *     The canvas on which the selection from the window's framebuffer
>   *     has been painted on.
>   */
> -capture.canvas = function (win, left, top, width, height, highlights = []) {
> +capture.canvas = function (win, left, top, width, height, highlights = [], canvas=null) {

Now that this function takes multiple optional arguments, can you change it to take an options dictionary?

> function (win, left, top, width, height, {highlights = [], canvas = undefined} = {}) {

This means you also need to change usages of this function that pass in highlights, but I think it’s worth the extra work since the argument list is already quite long.

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8866334 [details]
Bug 1363428 - Allow passing flags when drawing to a canavs,

https://reviewboard.mozilla.org/r/137964/#review142156

::: testing/marionette/capture.js:95
(Diff revision 1)
> + * @param {number} flags
> + *     Optional integer representing flags to pass to drawWindow; these are defined on
> + *     CanvasRenderingContext2D

Break at ~72 characters and end with a punctuation.

::: testing/marionette/capture.js:103
(Diff revision 1)
>   *
>   * @return {HTMLCanvasElement}
>   *     The canvas on which the selection from the window's framebuffer
>   *     has been painted on.
>   */
> -capture.canvas = function (win, left, top, width, height, highlights = [], canvas=null) {
> +capture.canvas = function (win, left, top, width, height, highlights = [], canvas=null, flags=null) {

Same as per comment on last patch: Make this part of an options dictionary.

The default value of flags should also be ctx.DRAWWINDOW_DRAW_CARET.

::: testing/marionette/capture.js:113
(Diff revision 1)
>      canvas.width = width * scale;
>      canvas.height = height * scale;
>    }
>  
>    let ctx = canvas.getContext(CONTEXT_2D);
> -  let flags = ctx.DRAWWINDOW_DRAW_CARET;
> +  if (!flags) {

When default value of flags has been changed to ctx.DRAWWINDOW_DRAW_CARET, you can remove this entirely.

	Andreas Tolfsen ❲:ato❳
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8866335 [details]
Bug 1363428 - Skip element highlighting if there are no highlights,

https://reviewboard.mozilla.org/r/137966/#review142186

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8866336 [details]
Bug 1363428 - Use an iterator for iterating windows,

https://reviewboard.mozilla.org/r/137968/#review142164

::: testing/marionette/driver.js:98
(Diff revision 1)
> +/**
> +* Helper function for iterating over windows
> +*/
> +function windowIterator() {
> +    return enumeratorIterator(Services.wm.getEnumerator(null));
> +}

This is a great improvement!

Expose this as a property on the GeckoDriver.prototype:

> Object.defineProperty(GeckoDriver.prototype, "windows", {
>   get: function () { … }
> });

I have a more sophisticated version of this on a local branch that I can later merge into what you have here.  That will allow us to do mapping and filtering also.

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8866336 [details]
Bug 1363428 - Use an iterator for iterating windows,

https://reviewboard.mozilla.org/r/137968/#review142188

	Andreas Tolfsen ❲:ato❳
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8866337 [details]
Bug 1363428 - Refactor Marionette switchToWindow implementation,

https://reviewboard.mozilla.org/r/137970/#review142168

::: commit-message-822ce:1
(Diff revision 1)
> +Bug 1363428 - Refactor marionette switchToWindow implementation, r=ato

s/marionette/Marionette/

::: testing/marionette/driver.js:1476
(Diff revision 1)
> +    throw new NoSuchWindowError(`Unable to locate window: ${switchTo}`);
> +  }
> +};
> +
> +/**
> + * Find a specific window according to some filter function

Add punctuation.

::: testing/marionette/driver.js:1480
(Diff revision 1)
> +/**
> + * Find a specific window according to some filter function
> + *
> + * @param {Iterable.<Window>} winIterable
> + *     Iterable that emits Windows
> + * @param {function} filter

Annotate this:

> @param {function(Window, number): boolean}

::: testing/marionette/driver.js:1481
(Diff revision 1)
> + *      A callback function taking two arguments; the window and
> + *      the outerId of the window, and returning a boolean indicating
> + *      whether the window is the target.

Should have one less space character on the left hand side.

::: testing/marionette/driver.js:1486
(Diff revision 1)
> + *      A window handle object containing the window and some
> + *      associated metadata

Same here.

::: testing/marionette/driver.js:1518
(Diff revision 1)
>    }
> +  return null;
> +};
>  
> -  if (found) {
> -    if (!(found.outerId in this.browsers)) {
> +/**
> + * Set the marionette window

Please expand this comment and explain that it register the browser, waits for it to register, then switches focus to it if |focus| is true.

::: testing/marionette/driver.js:1522
(Diff revision 1)
> -  if (found) {
> -    if (!(found.outerId in this.browsers)) {
> +/**
> + * Set the marionette window
> + *
> + * @param {Object} winProperties
> + *     Object containing window properties such as returned from
> + *     this.findWindow

Change this to:

> GeckoDriver#findWindow

::: testing/marionette/driver.js:1524
(Diff revision 1)
> + *      A boolean value which determines whether to focus
> + *      the window. Defaults to true.

One less space on the left hand side.  Also reformat paragraph.

::: testing/marionette/driver.js:1527
(Diff revision 1)
> + *     this.findWindow
> + * @param {boolean=} focus
> + *      A boolean value which determines whether to focus
> + *      the window. Defaults to true.
> + */
> +GeckoDriver.prototype.setWindowHandle = function* (winProperties, focus) {

Change function definition so that the focus argument has a default value:

> function* (winProperties, focus = true)

	Andreas Tolfsen ❲:ato❳
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review142172

::: commit-message-c936c:1
(Diff revision 1)
> +Bug 1363428 - Handle top level reftest window specially in marionette, r=ato
> +
> +Because this window is a normal Firefox window but doesn't contain any
> +tabs, the normal marionette functions for iterating over windows don't
> +work well. As a sort of hack, identify this window by the id of its
> +root element, and special case it when finding windows in marionette.

s/marionette/Marionette/g

::: testing/marionette/driver.js:362
(Diff revision 1)
>            // For browser windows we have to check if the current tab still exists.
>            if (this.curBrowser.tab && this.curBrowser.contentBrowser) {
>              win = this.curBrowser.window;
> +          } else if (this.isReftestWindow(this.curBrowser.window)) {
> +            // Look for a window with no tabs associated with a reftest run
> +            win = this.curBrowser.window;

Please move this comment out of the elseif-condition so that it describes what is about to be done, not what just happened.

This will match what we do a few lines earlier on :362.

::: testing/marionette/driver.js:381
(Diff revision 1)
> +GeckoDriver.prototype.isReftestWindow = function (window) {
> +  return window && window.gBrowser && this.isReftestBrowser(window.gBrowser);
> +}
> +
> +GeckoDriver.prototype.isReftestBrowser = function (element) {
> +  return element &&
> +    element.tagName === "xul:browser" &&
> +    element.parentElement &&
> +    element.parentElement.id === "reftest";
> +}

Make this part of the reftest module.

::: testing/marionette/driver.js:381
(Diff revision 1)
> +GeckoDriver.prototype.isReftestWindow = function (window) {
> +  return window && window.gBrowser && this.isReftestBrowser(window.gBrowser);
> +}

s/window/win/g because window can in some cases be a global.  This isn’t a big thing, but it improves safety to avoid common global names.

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review142174

::: commit-message-6ffac:1
(Diff revision 1)
> +Bug 1363428 - Add reftest-specific endpoints to marionette, r=ato

s/marionette/Marionette/

::: testing/marionette/driver.js:3445
(Diff revision 1)
> +/**
> + * End a reftest run
> + */
> +GeckoDriver.prototype.endReftest = function* (cmd, resp) {
> +  this._reftest = null;
> +};

Explain what this function does in the comment.

We also want it to successfully close the reftest window so that future Marionette commands can be executed.

::: testing/marionette/driver.js:3542
(Diff revision 1)
>    "localization:l10n:localizeProperty": GeckoDriver.prototype.localizeProperty,
>  
>    "addon:install": GeckoDriver.prototype.installAddon,
>    "addon:uninstall": GeckoDriver.prototype.uninstallAddon,
> +
> +  "reftest:start": GeckoDriver.prototype.startReftest,

This doesn’t really _start_ any tests, and for parity with reftest:end, I wonder if we should find a name that doesn’t imply this.  Maybe reftest:init or reftest:setup?

I like reftest:setup, which means you could rename reftest:end to reftest:teardown.

::: testing/marionette/reftest.js:43
(Diff revision 1)
> +  test();
> +}`;
> +
> +const PREF_E10S = "browser.tabs.remote.autostart";
> +
> +function Reftest(logger, driver) {

Please convert this to an ES6 class and assign it to the reftest module namespace:

> reftest.Runner = class { … }

::: testing/marionette/reftest.js:44
(Diff revision 1)
> +}`;
> +
> +const PREF_E10S = "browser.tabs.remote.autostart";
> +
> +function Reftest(logger, driver) {
> +  this.logger = logger;

No need to pass along the original logger.  Import Log.jsm and assign it to a const when the module loads.

::: testing/marionette/reftest.js:49
(Diff revision 1)
> +  this.SCREENSHOT_UNEXPECTED = 0;
> +  this.SCREENSHOT_FAIL = 1;
> +  this.SCREENSHOT_ALWAYS = 2;

Define these constants on :18 when you define the this.reftest object, like this:

> this.reftest = {
>   SCREENSHOT_UNEXPECTED: 0,
>   SCREENSHOT_FAIL: 1,
>   SCREENSHOT_ALWAYS: 2,
> };

::: testing/marionette/reftest.js:58
(Diff revision 1)
> +
> +Reftest.prototype = {
> +  setup: function* (urlCount, screenshot) {
> +    let win = assert.window(this.driver.getCurrentWindow());
> +
> +    this.screenshotMode = ["unexpected", "fail", "always"].indexOf(screenshot);

This will cause this.screenshotMode to be -1 if the screenshot argument is not one of these items.  Is this what you want?

I wonder if you might not want to define an “enum” with a lookup.  There are a number of examples of this in testing/marionette/action.js.

::: testing/marionette/reftest.js:63
(Diff revision 1)
> +    let reftestWin;
> +    let loadPromise = new Promise((resolve) => {
> +      reftestWin = win.openDialog(
> +        `data:text/xml,<window id="reftest" xmlns="${XUL_NS}" onload="dump('load\n'); window.arguments[0]()\n">abc</window>`,
> +        "reftest",
> +        "chrome,dialog,height=600,width=600,all",
> +        () => resolve());
> +    });

I would turn this promise into a separate function on the reftest module, along with all the attribute setup beneath.  When it is yielded, it would return reftestWin with all the state set up.

::: testing/marionette/reftest.js:64
(Diff revision 1)
> +
> +    this.urlCount = Object.keys(urlCount || {})
> +      .reduce((map, key) => map.set(key, urlCount[key]), new Map());
> +
> +    let reftestWin;
> +    let loadPromise = new Promise((resolve) => {

Drop the parenthesis around resolve.

::: testing/marionette/reftest.js:65
(Diff revision 1)
> +      reftestWin = win.openDialog(
> +        `data:text/xml,<window id="reftest" xmlns="${XUL_NS}" onload="dump('load\n'); window.arguments[0]()\n">abc</window>`,
> +        "reftest",
> +        "chrome,dialog,height=600,width=600,all",
> +        () => resolve());

Add a new .xul file and register it in testing/marionette/jar.mn.

::: testing/marionette/reftest.js:87
(Diff revision 1)
> +      browser.setAttribute("remote", "true");
> +      browser.setAttribute("remoteType", "web");
> +    }
> +    // Make sure the browser element is exactly 600x600, no matter
> +    // what size our window is
> +    browser.setAttribute("style", "padding: 0px; margin: 0px; border:none; min-width: 600px; min-height: 600px; max-width: 600px; max-height: 600px");

I suspect you can encode this into the new .xul document?

::: testing/marionette/reftest.js:97
(Diff revision 1)
> +    }
> +    doc.appendChild(browser);
> +    reftestWin.gBrowser = browser;
> +
> +    let found = this.driver.findWindow([reftestWin], () => true);
> +    let success = found.win === reftestWin;

This isn’t used.

::: testing/marionette/reftest.js:104
(Diff revision 1)
> +  run: function(testUrl,
> +                references,
> +                expected,
> +                timeout,
> +                result) {

Subsequent line indentation should be four characters.

::: testing/marionette/reftest.js:104
(Diff revision 1)
> +
> +    this.windowUtils = reftestWin.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDOMWindowUtils);
> +  },
> +
> +  run: function(testUrl,

API documentation required for this function.  I also feel it does a lot of things, and would encourage you to split it up so individual pieces are testable.

In particular, the documentation needs to say that the result object it receives is being mutated.

::: testing/marionette/reftest.js:125
(Diff revision 1)
> +
> +    //{url, references: [[{url, references}, relation], [..]]
> +
> +    // [url, [[==, ref1
> +    let stack = [];
> +    for (let i=references.length-1; i >= 0; i--) {

Spaces between =.

::: testing/marionette/reftest.js:137
(Diff revision 1)
> +    while (stack.length) {
> +      let [lhsUrl, rhsUrl, references, relation] = stack.pop();
> +
> +      let screenshots = {
> +        lhs: [],
> +        rhs: []

Comma after.

::: testing/marionette/reftest.js:140
(Diff revision 1)
> +      function recordScreenshot() {
> +        screenshotData.push([{url: lhsUrl, screenshot: screenshots.lhs[0].toDataURL().split(",")[1]},
> +                             relation,
> +                             {url:rhsUrl, screenshot: screenshots.rhs[0].toDataURL().split(",")[1]}]);
> +      }

Define an (inline?) function to extract the Base64.  This will also improve the indentation a bit.

> // I don’t know exactly what type |screenshot| is:
> const base64 = screenshot => screenshot.toDataURL().split(",")[1];
> 
> screenshotData.push([
>   {url: lhsUrl, screenshot: base64(screenshots.lhs[0]},
>   {url: rhsUrl, screenshot: base64(screenshots.rhs[0]},
> ]);

::: testing/marionette/reftest.js:146
(Diff revision 1)
> +      this.logger.info(`Testing ${lhsUrl} ${relation} ${rhsUrl}\n`);
> +      message += `Testing ${lhsUrl} ${relation} ${rhsUrl}\n`;

Duplication.

::: testing/marionette/reftest.js:158
(Diff revision 1)
> +      case "==":
> +        passed = differences === 0;
> +        break;
> +      case "!=":
> +        passed = differences !== 0;
> +        break;
> +      default:
> +        throw new InvalidArgumentError("Reftest operator should be '==' or '!='");

I disagree with this rule, but the cases must be indented.  There is a potential that this should be a separate function?

I would quite like to see unit xpcshell unit tests for this.

::: testing/marionette/reftest.js:174
(Diff revision 1)
> +        recordScreenshot();
> +      }
> +
> +      if (passed) {
> +        if (references.length) {
> +          for (let i=references.length - 1; i >= 0; i--) {

Space between =.

::: testing/marionette/reftest.js:197
(Diff revision 1)
> +      }
> +    }
> +
> +    result.status = status;
> +    if (screenshotData.length) {
> +        result.extra.reftest_screenshots = screenshotData;

Wrong indentation.

::: testing/marionette/reftest.js:202
(Diff revision 1)
> +        result.extra.reftest_screenshots = screenshotData;
> +    }
> +    result.message = message;
> +  },
> +
> +  screenshot: function* (win, url, timeout, rv) {

Please document and find a better name than rv.

::: testing/marionette/reftest.js:204
(Diff revision 1)
> +    result.message = message;
> +  },
> +
> +  screenshot: function* (win, url, timeout, rv) {
> +    let canvas = null;
> +    let remainingCount = (this.urlCount.get(url) || 1);

Drop parenthesis.

::: testing/marionette/reftest.js:222
(Diff revision 1)
> +      yield this.driver.executeAsyncScript({parameters: {script: REFTEST_SCRIPT,
> +                                                         args:[],
> +                                                         newSandbox: true,
> +                                                         sandbox: "default",
> +                                                         timeout: timeout}}, {body:{}});

Fix indentation here.  Maybe done by assigning object separately?

::: testing/marionette/reftest.js:229
(Diff revision 1)
> +      let flags = (ctxInterface.DRAWWINDOW_DRAW_CARET |
> +                   ctxInterface.DRAWWINDOW_USE_WIDGET_LAYERS |
> +                   ctxInterface.DRAWWINDOW_DRAW_VIEW);

Drop parenthesis and make subsequent line indentation only four spaces.

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8866340 [details]
Bug 1363428 - Support Marionette reftest implementation in wptrunner,

https://reviewboard.mozilla.org/r/137976/#review142190

::: testing/web-platform/harness/wptrunner/wptcommandline.py:186
(Diff revision 1)
>                               default=[], metavar="PREF=VALUE",
>                               help="Defines an extra user preference (overrides those in prefs_root)")
>      gecko_group.add_argument("--leak-check", dest="leak_check", action="store_true",
>                               help="Enable leak checking")
> +    gecko_group.add_argument("--internal-reftest", dest="internal_reftest", action="store_true",
> +                             default=False, help="Enable reftest runner implemented inside marionette")

s/marionette/Marionette/

	Josh Matthews [:jdm]
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8866341 [details]
Bug 1363428 - Switch wptrunner to use a deque for test groups,

https://reviewboard.mozilla.org/r/137978/#review142774

I've never looked at testloader.py before, but the changes look ok to me.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8866340 [details]
Bug 1363428 - Support Marionette reftest implementation in wptrunner,

https://reviewboard.mozilla.org/r/137976/#review142780

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8866342 [details]
Bug 1363428 - Add per-test-queue metadata to wptrunner,

https://reviewboard.mozilla.org/r/137980/#review142782

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 23 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review142172

> s/marionette/Marionette/g

Sorry, but what makes this window that special? Is it a chrome window or a popup with webcontent? Can you point me to the chrome URL of that page then? I'm a bit afraid to land such special code in Marionette itself. We don't need that anywhere yet, including the Firefox UI tests which are complex.

	James Graham [:jgraham] Assignee
	Comment 24 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review142172

> Sorry, but what makes this window that special? Is it a chrome window or a popup with webcontent? Can you point me to the chrome URL of that page then? I'm a bit afraid to land such special code in Marionette itself. We don't need that anywhere yet, including the Firefox UI tests which are complex.

https://reviewboard.mozilla.org/r/137974/diff/1#index_header testing/marionette/reftest.js line 65

	James Graham [:jgraham] Assignee
	Comment 25 • 7 years ago
mozreview-review-reply

Comment on attachment 8866334 [details]
Bug 1363428 - Allow passing flags when drawing to a canavs,

https://reviewboard.mozilla.org/r/137964/#review142156

> Same as per comment on last patch: Make this part of an options dictionary.
> 
> The default value of flags should also be ctx.DRAWWINDOW_DRAW_CARET.

I don't think this is worthwhile;  to be set as the default in the signature it would have to be `flags = win.CanvasRenderingContext2D.DRAWWINDOW_DRAW_CARET`, which would require almost 90 characters with indentation and still be less clear than an if condition in the function body.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148276

This way of handling the window is inappropriate. Basically because it's a problem in Marionette with handling non browser windows. I filed bug 1368965 to get this fixed.

Once this is done there shouldn't be a need to special the window at all. But you will have to work with chrome_window_handles to work with the window.

	James Graham [:jgraham] Assignee
	Comment 37 • 7 years ago

So I have a try run with reasonable-looking results, but some instability [1], including in tests that are originally from gecko. It is little enough that it might be possible to just disable those tests, but that's obviously not ideal. Looking at the reftest analyzer it seems that sometimes we have failed to complete painting e.g. [2]. As best I can tell the logic in this harness matches what's in the reftest harness, but with fewer features e.g. without support for scrolling, plugins, print mode, or invalidation.

Is there something I missed in the logic here, or something else I need to do in order to fix these problems? The wait-for-paint code I have is at [3].


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecc70dfea9edb56b2f9badc8f0aad264f020a635
[2] https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/MxlAuFdVTa2KBaNPoUwnZQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
[3] https://hg.mozilla.org/try/diff/48b933cd4ee8/testing/marionette/listener.js#l1.111

	Andreas Tolfsen ❲:ato❳
	Comment 38 • 7 years ago
mozreview-review

Comment on attachment 8866333 [details]
Bug 1363428 - Allow passing in an existing canvas to capture.canvas,

https://reviewboard.mozilla.org/r/137962/#review148390

I like how the new API turned out.  Thanks for making these changes.

	Andreas Tolfsen ❲:ato❳
	Comment 39 • 7 years ago
mozreview-review-reply

Comment on attachment 8866334 [details]
Bug 1363428 - Allow passing flags when drawing to a canavs,

https://reviewboard.mozilla.org/r/137964/#review142156

> I don't think this is worthwhile;  to be set as the default in the signature it would have to be `flags = win.CanvasRenderingContext2D.DRAWWINDOW_DRAW_CARET`, which would require almost 90 characters with indentation and still be less clear than an if condition in the function body.

You are right.

	Andreas Tolfsen ❲:ato❳
	Comment 40 • 7 years ago
mozreview-review

Comment on attachment 8866334 [details]
Bug 1363428 - Allow passing flags when drawing to a canavs,

https://reviewboard.mozilla.org/r/137964/#review148392

::: testing/marionette/capture.js:104
(Diff revision 2)
>   * @return {HTMLCanvasElement}
>   *     The canvas on which the selection from the window's framebuffer
>   *     has been painted on.
>   */
> -capture.canvas = function (win, left, top, width, height, {highlights = [], canvas = null} = {}) {
> +capture.canvas = function (win, left, top, width, height,
> +                           {highlights = [], canvas = null, flags = null} = {}) {

Second line indentation should be by four characters.

::: testing/marionette/capture.js:104
(Diff revision 2)
>   * @return {HTMLCanvasElement}
>   *     The canvas on which the selection from the window's framebuffer
>   *     has been painted on.
>   */
> -capture.canvas = function (win, left, top, width, height, {highlights = [], canvas = null} = {}) {
> +capture.canvas = function (win, left, top, width, height,
> +                           {highlights = [], canvas = null, flags = null} = {}) {

s/null/undefined/

::: testing/marionette/capture.js:114
(Diff revision 2)
>      canvas.width = width * scale;
>      canvas.height = height * scale;
>    }
>  
>    let ctx = canvas.getContext(CONTEXT_2D);
> -  let flags = ctx.DRAWWINDOW_DRAW_CARET;
> +  if (flags === null) {

typeof flags != "undefined"

::: testing/marionette/capture.js:115
(Diff revision 2)
> +      flags = ctx.DRAWWINDOW_DRAW_CARET;
>        // Disabled in bug 1243415 for webplatform-test failures due to out of view elements.
>        // Needs https://github.com/w3c/web-platform-tests/issues/4383 fixed.
>        // ctx.DRAWWINDOW_DRAW_VIEW;
>        // Bug 1009762 - Crash in [@ mozilla::gl::ReadPixelsIntoDataSurface]
>        // ctx.DRAWWINDOW_USE_WIDGET_LAYERS;

Indentation should be two characters, not four.

	Andreas Tolfsen ❲:ato❳
	Comment 41 • 7 years ago
mozreview-review

Comment on attachment 8866334 [details]
Bug 1363428 - Allow passing flags when drawing to a canavs,

https://reviewboard.mozilla.org/r/137964/#review148398

::: testing/marionette/capture.js:95
(Diff revision 2)
> + * @param {number} flags
> + *     Optional integer representing flags to pass to drawWindow; these are defined on
> + *     CanvasRenderingContext2D.

This should be:

> @param {number=} flags

The "=" means it is optional.

	Andreas Tolfsen ❲:ato❳
	Comment 42 • 7 years ago
mozreview-review

Comment on attachment 8866334 [details]
Bug 1363428 - Allow passing flags when drawing to a canavs,

https://reviewboard.mozilla.org/r/137964/#review148400

::: testing/marionette/capture.js:96
(Diff revision 2)
>   *     Optional array of nodes, around which a border will be marked to
>   *     highlight them in the screenshot.
>   * @param {HTMLCanvasElement} canvas
>   *     Optional canvas to reuse for the screenshot.
> + * @param {number} flags
> + *     Optional integer representing flags to pass to drawWindow; these are defined on

Break at ~72 characters.

	Andreas Tolfsen ❲:ato❳
	Comment 43 • 7 years ago
mozreview-review

Comment on attachment 8866333 [details]
Bug 1363428 - Allow passing in an existing canvas to capture.canvas,

https://reviewboard.mozilla.org/r/137962/#review148402

::: testing/marionette/capture.js:93
(Diff revision 2)
>   * @param {number} height
>   *     The height dimension of the rectangle to paint.
>   * @param {Array.<Node>=} highlights
>   *     Optional array of nodes, around which a border will be marked to
>   *     highlight them in the screenshot.
> + * @param {HTMLCanvasElement} canvas

Missing "=" character to indicate it is optional.  This should red:

> @param {HTMLCanvasElement=} canvas

::: testing/marionette/capture.js:100
(Diff revision 2)
>   *
>   * @return {HTMLCanvasElement}
>   *     The canvas on which the selection from the window's framebuffer
>   *     has been painted on.
>   */
> -capture.canvas = function (win, left, top, width, height, highlights = []) {
> +capture.canvas = function (win, left, top, width, height, {highlights = [], canvas = null} = {}) {

s/null/undefined/

::: testing/marionette/capture.js:103
(Diff revision 2)
>   *     has been painted on.
>   */
> -capture.canvas = function (win, left, top, width, height, highlights = []) {
> +capture.canvas = function (win, left, top, width, height, {highlights = [], canvas = null} = {}) {
>    let scale = win.devicePixelRatio;
>  
> -  let canvas = win.document.createElementNS(XHTML_NS, "canvas");
> +  if (canvas === null) {

if (typeof canvas != "undefined")

	Andreas Tolfsen ❲:ato❳
	Comment 44 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review142172

> https://reviewboard.mozilla.org/r/137974/diff/1#index_header testing/marionette/reftest.js line 65

> s/marionette/Marionette/g

This issue has been solved.  I suggest raising a new one to discuss what makes the window ‘special’.

	Andreas Tolfsen ❲:ato❳
	Comment 45 • 7 years ago
mozreview-review

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review148420

Many good improvements, but a few things left unanswered.

::: testing/marionette/driver.js:3350
(Diff revision 2)
> +/**
> + * Initialize the reftest mode
> + */
> +GeckoDriver.prototype.setupReftest = function* (cmd, resp) {
> +  this._reftest = new reftest.Runner(this);
> +  let {url_count, screenshot} = cmd.parameters;

Are you married to the input here?  I would prefer urlCount over url_count.

::: testing/marionette/driver.js:3356
(Diff revision 2)
> +
> +  if (screenshot === undefined) {
> +    screenshot = "unexpected";
> +  }
> +
> +  if (["always", "fail", "unexpected"].indexOf(screenshot) === -1) {

Use [].includes over indexOf.

::: testing/marionette/driver.js:3369
(Diff revision 2)
> +  if (typeof test != "string") {
> +    throw new InvalidArgumentError("Value of `test` should be of type 'string'");
> +  }
> +
> +  if (typeof expected != "string") {
> +    throw new InvalidArgumentError("Value of `expected` should be of type 'string'");
> +  }

Replace these with assert.string(…).

::: testing/marionette/driver.js:3377
(Diff revision 2)
> +  if (!Array.isArray(references)) {
> +      throw new InvalidArgumentError("Value of `references` should be of type 'Array'");
> +  }

Replace with assert.array.

::: testing/marionette/driver.js:3389
(Diff revision 2)
> +    extra: {}
> +  };
> +
> +  let timedOut = false;
> +
> +  timeout = new Promise((resolve) => {

Drop parenthesis.

::: testing/marionette/driver.js:3389
(Diff revision 2)
> +  timeout = new Promise((resolve) => {
> +    this.getCurrentWindow().setTimeout(() => {
> +      result.status = "TIMEOUT";
> +      resolve();
> +    }, timeout);
> +   });

You need to clear the timeout ID returned by setTimeout if it completes successfully.

::: testing/marionette/driver.js:3391
(Diff revision 2)
> +
> +  let timedOut = false;
> +
> +  timeout = new Promise((resolve) => {
> +    this.getCurrentWindow().setTimeout(() => {
> +      result.status = "TIMEOUT";

I think I would prefer "TIMEOUT", "ERROR", et al. to be constants on reftest.Status.{Timeout, Error} so we don’t accidentally mistype them.

::: testing/marionette/driver.js:3414
(Diff revision 2)
> +
> +/**
> + * End a reftest run
> + */
> +GeckoDriver.prototype.teardownReftest = function* (cmd, resp) {
> +  this._reftest = null;

Should reftest window be closed here?

::: testing/marionette/driver.js:3505
(Diff revision 2)
> +  "reftest:setup": GeckoDriver.prototype.setupReftest,
> +  "reftest:run": GeckoDriver.prototype.runReftest,
> +  "reftest:teardown": GeckoDriver.prototype.teardownReftest,

What is the rationale for having reftest:setup and reftest:teardown separate to reftest:run?

::: testing/marionette/listener.js:207
(Diff revision 2)
>    /**
>     * Callback for registered DOM events.
>     */
>    handleEvent: function (event) {
>      let location = event.target.baseURI || event.target.location.href;
> -    logger.debug(`Received DOM event "${event.type}" for "${location}"`);
> +//    logger.debug(`Received DOM event "${event.type}" for "${location}"`);

Debugging?

::: testing/marionette/listener.js:1776
(Diff revision 2)
>      default:
>        throw new TypeError("Unknown screenshot format: " + format);
>    }
>  }
>  
> +function flushRendering() {

Documentation, please.

Also, the indentation of this function is all wrong.  Not raising individual issues on it.

::: testing/marionette/listener.js:1780
(Diff revision 2)
>  
> +function flushRendering() {
> +    let content = curContainer.frame;
> +    let anyPendingPaintsGeneratedInDescendants = false;
> +
> +    dump("flushRendering\n");

Debug?

::: testing/marionette/listener.js:1810
(Diff revision 2)
> +    if (anyPendingPaintsGeneratedInDescendants &&
> +        !windowUtils.isMozAfterPaintPending) {
> +        logger.error("Internal error: descendant frame generated a MozAfterPaint event, but the root document doesn't have one!");
> +    }

If you want this to be fatal, you need to throw.

::: testing/marionette/listener.js:1816
(Diff revision 2)
> +        !windowUtils.isMozAfterPaintPending) {
> +        logger.error("Internal error: descendant frame generated a MozAfterPaint event, but the root document doesn't have one!");
> +    }
> +
> +
> +    dump(`flushRendering ${windowUtils.isMozAfterPaintPending}\n`);

Debug?

::: testing/marionette/listener.js:1828
(Diff revision 2)
> +
> +  let windowUtils = content.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDOMWindowUtils);
> +
> +  if (document.location.href != url || document.readyState != "complete") {
> +    let onloadPromise = new Promise(resolve => document.addEventListener(

You could assign/define the promise outside the if-condition and yield it inside, or yield it directly without assigning to onloadPromise to clean up this code.

::: testing/marionette/listener.js:1830
(Diff revision 2)
> +      .getInterface(Ci.nsIDOMWindowUtils);
> +
> +  if (document.location.href != url || document.readyState != "complete") {
> +    let onloadPromise = new Promise(resolve => document.addEventListener(
> +      "load",
> +      (event) => {

Drop parenthesis.

::: testing/marionette/listener.js:1831
(Diff revision 2)
> +        if (event.target == curContainer.frame.document &&
> +            curContainer.frame.document.location.href == url)
> +          document = curContainer.frame.document;

Missing curly braces.

::: testing/marionette/listener.js:1836
(Diff revision 2)
> +        if (event.target == curContainer.frame.document &&
> +            curContainer.frame.document.location.href == url)
> +          document = curContainer.frame.document;
> +        setTimeout(resolve, 0);
> +      },
> +      {once:true, capture:false}));

Spaces after colons.

::: testing/marionette/listener.js:1842
(Diff revision 2)
> +    let classListPromise = new Promise(resolve => {
> +      let observer = new win.MutationObserver(() => {
> +        if (!root.classList.contains("reftest-wait")) {
> +          observer.disconnect();
> +          setTimeout(resolve, 0);
> +        }
> +      });
> +      observer.observe(root, {attributes: true});
> +    });

Instead of waiting for the reftest-wait class to disappear, you could consider firing a CustomEvent.

::: testing/marionette/listener.js:1854
(Diff revision 2)
> +      observer.observe(root, {attributes: true});
> +    });
> +
> +    yield classListPromise;
> +
> +    dump("Waiting for rendering\n");

Debug?

::: testing/marionette/listener.js:1857
(Diff revision 2)
> +        let maybeResolve = () => {
> +            if (flushRendering()) {
> +                win.addEventListener("MozAfterPaint", maybeResolve, {once: true});
> +            } else {
> +                resolve();
> +            }
> +        };
> +        maybeResolve();

Wrong indentation in many places.

::: testing/marionette/reftest.js:28
(Diff revision 2)
> +  unexpected: 0,
> +  fail: 1,
> +  always: 2
> +};
> +
> +let reftest = {};

Please document what the reftest module is.

::: testing/marionette/reftest.js:33
(Diff revision 2)
> +    this.canvasCache = new Map();
> +    this.canvasCache.set(null, []);

This could be combined:

> this.canvasCache = new Map([[null, []]);

::: testing/marionette/reftest.js:54
(Diff revision 2)
> +    this.screenshotMode = SCREENSHOT_MODE[screenshotMode] ||
> +      SCREENSHOT_MODE["unexpected"];

Four lines for continued statements.

::: testing/marionette/reftest.js:57
(Diff revision 2)
> +    this.urlCount = Object.keys(urlCount || {})
> +      .reduce((map, key) => map.set(key, urlCount[key]), new Map());

Same.

::: testing/marionette/reftest.js:60
(Diff revision 2)
> +      SCREENSHOT_MODE["unexpected"];
> +
> +    this.urlCount = Object.keys(urlCount || {})
> +      .reduce((map, key) => map.set(key, urlCount[key]), new Map());
> +
> +    dump(`urlCount ${JSON.stringify(this.urlCount)}\n`);

Debug?

::: testing/marionette/reftest.js:69
(Diff revision 2)
> +    let found = this.driver.findWindow([reftestWin], () => true);
> +    yield this.driver.setWindowHandle(found, true);
> +
> +    this.windowUtils = reftestWin.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDOMWindowUtils);
> +

Superfluous line.

::: testing/marionette/reftest.js:88
(Diff revision 2)
> +    //    if (Preferences.get(PREF_E10S)) {
> +    browser.setAttribute("remote", "true");
> +    browser.setAttribute("remoteType", "web");
> +    //    }

Why is PREF_E10S commented out?

::: testing/marionette/reftest.js:92
(Diff revision 2)
> +    // Make sure the browser element is exactly 600x600, no matter
> +    // what size our window is
> +    browser.setAttribute("style", "padding: 0px; margin: 0px; border:none; min-width: 600px; min-height: 600px; max-width: 600px; max-height: 600px");

Could this be encoded directly in reftest.xul?

::: testing/marionette/reftest.js:192
(Diff revision 2)
> +      } else {
> +        // If we don't have any alternatives to try then this will be the last iteration,
> +        // so save the failing screenshots if required
> +        if (!stack.length &&
> +            (this.screenshotMode === SCREENSHOT_MODE.fail ||
> +             (this.screenshotMode === SCREENSHOT_MODE.unexpected && expected != status))) {
> +          recordScreenshot();
> +        }

You could make the !stack.length condition part of the else-condition on :192.

I was originally going to ask you to assign the screenshotMode comparisons to variables explaining what they do to reduce the complexity of the if-condition, but I think it will be good once !stack.length is not part of it.

::: testing/marionette/reftest.js:204
(Diff revision 2)
> +        }
> +      }
> +
> +      // Return any reusable canvases to the pool
> +      let canvasPool = this.canvasCache.get(null);
> +      [comparison.lhs, comparison.rhs].map((screenshot) => {

Drop parenthesis on screenshot.

::: testing/marionette/reftest.js:206
(Diff revision 2)
> +
> +      // Return any reusable canvases to the pool
> +      let canvasPool = this.canvasCache.get(null);
> +      [comparison.lhs, comparison.rhs].map((screenshot) => {
> +        if (screenshot.reuseCanvas) {
> +          logger.debug(`Pushing ${screenshot.canvas}`);

Do you want to have this in production?

::: testing/marionette/reftest.js:210
(Diff revision 2)
> +        if (screenshot.reuseCanvas) {
> +          logger.debug(`Pushing ${screenshot.canvas}`);
> +          canvasPool.push(screenshot.canvas);
> +        }
> +      });
> +      logger.debug(`Canvas pool is of length ${canvasPool.length}`);

Same here.

::: testing/marionette/reftest.js:233
(Diff revision 2)
> +    case "==":
> +      passed = differences === 0;
> +      break;
> +    case "!=":
> +      passed = differences !== 0;
> +      break;
> +    default:
> +      throw new InvalidArgumentError("Reftest operator should be '==' or '!='");

Unfortunately, in Gecko JS this is supposed to be indented.

::: testing/marionette/reftest.js:250
(Diff revision 2)
> +
> +  *screenshot(win, url, timeout) {
> +    let canvas = null;
> +    let remainingCount = this.urlCount.get(url) || 1;
> +    let cache = remainingCount > 1;
> +    logger.debug(`screenshot ${url} remainingCount: ${remainingCount} cache: ${cache}`);

Debug?

::: testing/marionette/reftest.js:253
(Diff revision 2)
> +    let remainingCount = this.urlCount.get(url) || 1;
> +    let cache = remainingCount > 1;
> +    logger.debug(`screenshot ${url} remainingCount: ${remainingCount} cache: ${cache}`);
> +    let reuseCanvas = false;
> +    if (this.canvasCache.has(url)) {
> +      logger.debug(`screenshot ${url} taken from cache`);

Debug?

::: testing/marionette/reftest.js:274
(Diff revision 2)
> +          ctxInterface.DRAWWINDOW_USE_WIDGET_LAYERS |
> +          ctxInterface.DRAWWINDOW_DRAW_VIEW;
> +
> +      yield this.driver.get({parameters: {url: url}}, null);
> +      yield this.driver.listener.reftestWait(url);
> +      logger.debug(`screenshot ${url} taken, reusing canvas: ${canvas !== null}`);

Debug?

::: testing/marionette/reftest.js:276
(Diff revision 2)
> +
> +      yield this.driver.get({parameters: {url: url}}, null);
> +      yield this.driver.listener.reftestWait(url);
> +      logger.debug(`screenshot ${url} taken, reusing canvas: ${canvas !== null}`);
> +
> +      canvas = capture.canvas(win, 0, 0, win.innerWidth, win.innerHeight,  {canvas, flags});

Two spaces before {.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 46 • 7 years ago

(In reply to James Graham [:jgraham] from comment #37)
> So I have a try run with reasonable-looking results, but some instability
> [1], including in tests that are originally from gecko. It is little enough
> that it might be possible to just disable those tests, but that's obviously
> not ideal. Looking at the reftest analyzer it seems that sometimes we have
> failed to complete painting e.g. [2]. As best I can tell the logic in this
> harness matches what's in the reftest harness, but with fewer features e.g.
> without support for scrolling, plugins, print mode, or invalidation.
> 
> Is there something I missed in the logic here, or something else I need to
> do in order to fix these problems? The wait-for-paint code I have is at [3].

In the Gecko reftest harness, we want to allow the tests to do stuff in onload and even (I think) in a setTimeout(0) that runs in their onload, so we start doing the interesting stuff after a setTimeout(0) from our onload handler (which I *think* is set up to run after the page's).  I guess that looks like what you're doing with the onloadPromise stuff.

It's also not clear to me where the calling code of that reftestWait function is, and whether it correctly handles that function yielding multiple promises by calling it again *after* the first promise has resolved.

I'd be a little concerned that you're not waiting for MozAfterPaint in enough situations; it looks like your code only does that when the root has class="reftest-wait", whereas in the existing reftest harness we call WaitForTestEnd in more situations.

But there could certainly be other things; I didn't look that carefully, and there's a good bit of logic in there.

I'm also confused by your mixing of the 'once' parameter to addEventListener with conditions that only consider it the right event some of the time.  How does that work if the conditions fail (e.g., an iframe onload fires)?

Also, why are you mixing 2-space and 4-space indent?

That said, it's hard to verify by looking at the code that the code does what it looks like it does.  Debugging may well be needed.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 47 • 7 years ago
mozreview-review

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148490

My review still stands on this one.

	James Graham [:jgraham] Assignee
	Comment 48 • 7 years ago
mozreview-review-reply

Comment on attachment 8866333 [details]
Bug 1363428 - Allow passing in an existing canvas to capture.canvas,

https://reviewboard.mozilla.org/r/137962/#review148402

> s/null/undefined/

I don't understand why. it seems technically bad. This way if you pass `undefined` or `null` it works in exactly the same way (because `undefined` will be coerced to `null`, so passing in `{canvas: null}`, `{canvas: undefined}` and `{}` are all equivalent).

> if (typeof canvas != "undefined")

Comparing `typeof` against a string (which can easily have a typo) seems strictly worse than making a type sensitive comparison against a value. What's the reason for this preference?

	James Graham [:jgraham] Assignee
	Comment 49 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review142172

> Make this part of the reftest module.

I can't really do that without reordering the patch, and it seems like this change might be dropped anyway.

	James Graham [:jgraham] Assignee
	Comment 50 • 7 years ago
mozreview-review-reply

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review148420

> What is the rationale for having reftest:setup and reftest:teardown separate to reftest:run?

Because you set up the harness and then call `run` multiple times; each call to `run` runs a single test.

> Documentation, please.
> 
> Also, the indentation of this function is all wrong.  Not raising individual issues on it.

This is taken directly from the other reftest harness; I'm not sure what you want me to write.

> If you want this to be fatal, you need to throw.

Taken directly from the other reftest harness which also doesn't throw so I"m happy with this.

> Instead of waiting for the reftest-wait class to disappear, you could consider firing a CustomEvent.

These are the defined sematics of reftests.

> Four lines for continued statements.

For the record, this is a very annoying convention. It means that the tab key doesn't provide the right indentation by default in this case, but does in all other cases.

> Why is PREF_E10S commented out?

Setting this didn't seem to work too wel. I'm not yet sure why.

> Could this be encoded directly in reftest.xul?

Duplicate of another issue.

> Same here.

Yeah, this seems potentially helpful for some debugging scenarios.

> Debug?

I think keeping this as debug level logging is fine.

> Debug?

Also this.

	James Graham [:jgraham] Assignee
	Comment 51 • 7 years ago
mozreview-review-reply

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review142174

> This will cause this.screenshotMode to be -1 if the screenshot argument is not one of these items.  Is this what you want?
> 
> I wonder if you might not want to define an “enum” with a lookup.  There are a number of examples of this in testing/marionette/action.js.

We already check this value is in the allowed range in `driver.js` with the other input validation. Anyway, changed.

> I suspect you can encode this into the new .xul document?

I copied it from the reftest harness. I'd rather leave it here in case there is some timing problem like the window manager deciding to resize the window after it opens.

> Subsequent line indentation should be four characters.

That is a horrible convention. Why would you want code to look like

```
function (testUrl,
    references,
    expected,
    […]):
```

I just put everthing on a single line instead.

> Duplication.

Not really, one is logged directly, one is returned to the client.

> Fix indentation here.  Maybe done by assigning object separately?

This code is gone.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 65 • 7 years ago
mozreview-review

Comment on attachment 8876230 [details]
Bug 1363428 - Add Ahem to wpt fonts directory,

https://reviewboard.mozilla.org/r/147680/#review152058

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 66 • 7 years ago
mozreview-review

Comment on attachment 8876231 [details]
Bug 1363428 - Copy the Ahem font into the bundled font directory,

https://reviewboard.mozilla.org/r/147682/#review152060

	Andreas Tolfsen ❲:ato❳
	Comment 67 • 7 years ago
mozreview-review

Comment on attachment 8876229 [details]
Bug 1363428 - Make it possible to close tabless content windows,

https://reviewboard.mozilla.org/r/147678/#review152370

	Andreas Tolfsen ❲:ato❳
	Comment 68 • 7 years ago
mozreview-review-reply

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review142174

> I copied it from the reftest harness. I'd rather leave it here in case there is some timing problem like the window manager deciding to resize the window after it opens.

OK, but then can you please move the CSS to a constant with new lines to make it easier to read?  The backtick JS template function should let you achieve the desired effect.

For example:

> const STYLING = `
> padding: 0px;
> margin: 0px;
> …`;

	Andreas Tolfsen ❲:ato❳
	Comment 69 • 7 years ago
mozreview-review

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review152372

::: testing/marionette/driver.js:3296
(Diff revision 3)
> +GeckoDriver.prototype.setupReftest = function* (cmd, resp) {
> +  this._reftest = new reftest.Runner(this);

There should be a check that reftest has not already been set up here, returning an error if it has.

::: testing/marionette/driver.js:3315
(Diff revision 3)
> +GeckoDriver.prototype.runReftest = function* (cmd, resp) {
> +  let {test, references, expected, timeout} = cmd.parameters;

Add a check that this._reftest has been set up first.

::: testing/marionette/driver.js:3333
(Diff revision 3)
> +GeckoDriver.prototype.teardownReftest = function* (cmd, resp) {
> +  this.close();
> +  this._reftest = null;
> +};

I feel there should be a state check here that reftest has actually been set up.  If teardownReftest is called without first calling setupReftest, it will close the current window.

You could return an error if this._reftest is null.

::: testing/marionette/reftest.js:97
(Diff revision 3)
> +    let reftestWin;
> +    yield new Promise(resolve => {
> +      reftestWin = this.parentWindow.openDialog("chrome://marionette/content/reftest.xul",
> +                                                "reftest",
> +                                                "chrome,dialog,height=600,width=600,all",
> +                                                () => resolve());

You could drop the anonymous function here.

::: testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py:186
(Diff revision 3)
> +                                      "dom.send_after_paint_to_content": True,
> +                                      "layout.interruptible-reflow.enabled": False})

Are these relevant also to WebDriver end-users?  If so they should be set in testing/geckodriver/src/prefs.rs and testing/marionette/server.js as well.

	Andreas Tolfsen ❲:ato❳
	Comment 70 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148276

I think probably we have to special-case reftest windows here because they have a tabbrowser but no tabs, as I understand it.  

We want the reftest window to be discoverable so it is possible to switch to it, but because it behaves fundamentally different to other content browser windows, special precautions need to made for it.

Please correct me if I’m wrong, jgraham?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 71 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148276

Well, if that is the case and it's not a browser window, accessing it should be done via the chrome window handle, but not the window handle. Right now `window_handles` sadly returns `chrome_window_handles` too, which I will fix in the above mentioned bug.

So please do not special case this type of window. If you need help please let me know, or check our Firefox UI tests in how this can be done.

	Andreas Tolfsen ❲:ato❳
	Comment 72 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148276

So the reftest window has the interesting characteristic that it’s not a chrome window, but a specialised content browser window without tabs.

We should indeed not return ChromeWindows from getWindowHandles, and you are correct to fix this.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 73 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148276

So it's a browser.xul based popup just without any chrome elements?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 74 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148276

So I found the following definition in a later??? changeset:

> <window id="reftest" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" onload="window.arguments[0]()"></window>

This is NOT a browser window but a custom chrome window type. And the difference to a normal browser window is that a `xul:browser` element is created manually. That's all. As such it has to be detected and handled via chrome window handles!

	Andreas Tolfsen ❲:ato❳
	Comment 75 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148276

> So I found the following definition in a later??? changeset:
> 
>> <window id="reftest" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" onload="window.arguments0"></window>
> 
> This is NOT a browser window but a custom chrome window type. And the
> difference to a normal browser window is that a xul:browser element
> is created manually. That's all. As such it has to be detected and
> handled via chrome window handles!

Content browser windows are also descendent of <window>.  The difference
between them and the reftest window is that the latter does not have a
tabbrowser or chrome.  In other words, what sets chrome- and content
windows apart is not their DOM structure or buildup, but what function
they have in a semantic context.

Now, I see arguments both for or against qualifying this as a content
window, but its primary function if you were to interact with it is
similar to that of a normal browsing tab.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 76 • 7 years ago
mozreview-review

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review148510

::: testing/marionette/driver.js:3348
(Diff revision 2)
>  }
>  
> +/**
> + * Initialize the reftest mode
> + */
> +GeckoDriver.prototype.setupReftest = function* (cmd, resp) {

Why can all those specific reftest related methods not live in reftests.js?

::: testing/marionette/listener.js:1092
(Diff revision 3)
>  
>    try {
>      if (typeof url == "string") {
>        try {
>          let requestedURL = new URL(url).toString();
> +        if (loadEventExpected === null) {

What is the purpose of this change? I don't see that any get() command is in use which would require the loadEventExpected to pass in.

::: testing/marionette/reftest.js:113
(Diff revision 3)
> +    browser.setAttribute("remote", "true");
> +    browser.setAttribute("remoteType", "web");
> +    //    }
> +    // Make sure the browser element is exactly 600x600, no matter
> +    // what size our window is
> +    browser.setAttribute("style", "padding: 0px; margin: 0px; border:none; min-width: 600px; min-height: 600px; max-width: 600px; max-height: 600px");

Why can all the initialization code above not be done in the xul file itself?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 77 • 7 years ago
mozreview-review-reply

Comment on attachment 8866338 [details]
Bug 1363428 - Handle top level reftest window specially in Marionette,

https://reviewboard.mozilla.org/r/137972/#review148276

This is all fine, but we should not special case just the reftest window. Any kind of chrome window with a xul:browser element would have to give the opportunity to work with the DOM. Whether the content runs in chrome or in content scope.

I will experiment with that tomorrow.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 91 • 7 years ago

Comment on attachment 8866341 [details]
Bug 1363428 - Switch wptrunner to use a deque for test groups,

I don't feel comfortable reviewing this. If getting my review is important, flag me again and I'll take the time to do a deep dive.

	Andreas Tolfsen ❲:ato❳
	Comment 107 • 7 years ago
mozreview-review

Comment on attachment 8879192 [details]
Bug 1363428 - Update expectation data for reftest changes,

https://reviewboard.mozilla.org/r/150528/#review155572

::: testing/web-platform/meta/mixed-content/imageset.https.sub.html.ini:1
(Diff revision 1)
>  [imageset.https.sub.html]
>    type: testharness
>    [Makes sure imageset blockable resources are not downloaded]
> -    expected: FAIL
> +    expected:
> +      if not debug and not stylo and e10s and (os == "mac") and (version == "OS X 10.10.5") and (processor == "x86_64") and (bits == 64): PASS
> +      FAIL

This is not a refest.

::: testing/web-platform/meta/webdriver/actions/key.py.ini:1
(Diff revision 1)
> +[key.py]
> +  type: wdspec
> +  expected:
> +    if not debug and not stylo and not e10s and (os == "linux") and (version == "Ubuntu 16.04") and (processor == "x86_64") and (bits == 64): TIMEOUT

Not a reftest.

	Andreas Tolfsen ❲:ato❳
	Comment 108 • 7 years ago
mozreview-review

Comment on attachment 8879193 [details]
Bug 1363428 - Use the internal reftest implementation by default on Linux and OSX,

https://reviewboard.mozilla.org/r/150530/#review155574

	Andreas Tolfsen ❲:ato❳
	Comment 109 • 7 years ago
mozreview-review-reply

Comment on attachment 8866333 [details]
Bug 1363428 - Allow passing in an existing canvas to capture.canvas,

https://reviewboard.mozilla.org/r/137962/#review148402

> I don't understand why. it seems technically bad. This way if you pass `undefined` or `null` it works in exactly the same way (because `undefined` will be coerced to `null`, so passing in `{canvas: null}`, `{canvas: undefined}` and `{}` are all equivalent).

So I took a closer look at how {foo = <default value>} = {} behaves
in arguments, and it turns out it has some pretty interesting
characteristics.  Notably, if the default value assigned to it is null
and you pass in {foo: undefined} it gets the default value of null.
This may seem obvious because it gets its populated value from an empty
{}.

I think I had a shotgun reaction of dealing with missing vs. null fields
in command parameters in driver.js, where this sort of distinction
matters _a lot_ and where you do not have default values.  There are
some commands, such as Execute Script, where the absence of a field has
semantic meaning different to null due to legacy Marionette-only APIs.

But given your explanation and the tests I ran (pasted for posterity
below), I agree with your assessment that using null here is the right
thing to do.  It will guarantee, as you said, that {canvas: null},
{canvas: undefined}, and {} are equivalent, and lets us use a strict
equality check (===) further down, which is much better than loose
equality.

> function foo ({canvas = null} = {}) {
>   console.log("canvas === null", canvas === null);
>   console.log("canvas == null", canvas == null);
>   console.log("typeof canvas == 'undefined'", typeof canvas == "undefined");
> }
> 
> console.log("{canvas: null}");
> foo({canvas: null});
> 
> console.log("{canvas: undefined}");
> foo({canvas: undefined});
> 
> console.log("{}");
> foo({});
> 
> console.log("(no args)");
> foo();
> 
> console.log("{canvas: 'foo'}");
> foo({canvas: "foo"});

	Andreas Tolfsen ❲:ato❳
	Comment 110 • 7 years ago
mozreview-review-reply

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review148510

> Why can all those specific reftest related methods not live in reftests.js?

I think it makes sense to put the command handling here for the moment.  This is in line with what we do for addon:* and locationzation:*.

At some point in the future I want to make it possible for modules to provide their own command handlers, but this work is scheduled for https://bugzilla.mozilla.org/show_bug.cgi?id=1330309.

> What is the purpose of this change? I don't see that any get() command is in use which would require the loadEventExpected to pass in.

There’s a call to GeckoDriver#get on reftest.js:334.

	Andreas Tolfsen ❲:ato❳
	Comment 111 • 7 years ago
mozreview-review-reply

Comment on attachment 8866339 [details]
Bug 1363428 - Add reftest-specific endpoints to Marionette,

https://reviewboard.mozilla.org/r/137974/#review148420

> Because you set up the harness and then call `run` multiple times; each call to `run` runs a single test.

Makes sense, but please see my other issues about calling reftest:run and reftest:teardown without first having called reftest:setup and vice versa.

> This is taken directly from the other reftest harness; I'm not sure what you want me to write.

The indentation looks correct in your latest revision, so I guess you figured it out.

The general formatting rules are two spaces for normal block indentation and four for statements that are wrapped and continued.  We try to not let lines go on much longer than 72 characters, but there are obvious exceptions.  We also want "function" to be followed by a space, i.e. "function (foo) { … }" (and not "function(foo) { … }") so it is obvious we’re not calling a function named "function".

I have a patch in progress to enable eslint for testing/marionette, but it has not been a high priority.  Sorry for this.

> For the record, this is a very annoying convention. It means that the tab key doesn't provide the right indentation by default in this case, but does in all other cases.

Annoying or not; it is the convention.  When you say that the tab key “doesn’t provide the right indentation”, I assume you mean that “Emacs does not provide the right indentation”.  The tab key, by default, produces a tabular character.

That aside, I am very aware this is a pain point working on Marionette code.  Other pieces of JS code in Gecko use eslint and we’ve been undergoing stylistic changes to Marionette source code to adopt devtools’ eslint style, which by this point is very similar to what you find in Marionette.

> Setting this didn't seem to work too wel. I'm not yet sure why.

I think you need to either figure out why it doesn’t, or require the new reftest service in Marionette to only support e10s.

In the latter case you need to return an error if trying to initialise it in a Firefox with e10s enabled.

> I think keeping this as debug level logging is fine.

Yes, I agree.  I’ve dropped all similar issues.  Just wanted to make sure.

	Andreas Tolfsen ❲:ato❳
	Comment 120 • 7 years ago
mozreview-review

Comment on attachment 8879192 [details]
Bug 1363428 - Update expectation data for reftest changes,

https://reviewboard.mozilla.org/r/150528/#review155622

	Pulsebot
	Comment 121 • 7 years ago

Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42e2ae612663
Allow passing in an existing canvas to capture.canvas, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/97db3b14fafd
Allow passing flags when drawing to a canavs, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e74e66ca3e
Skip element highlighting if there are no highlights, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d682cbc05f
Use an iterator for iterating windows, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/014b9aeb9ac2
Refactor Marionette switchToWindow implementation, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f9bdba146c
Handle top level reftest window specially in Marionette, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7086fa5816c
Make it possible to close tabless content windows, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5ad17f890a
Add reftest-specific endpoints to Marionette, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/f09573370d2f
Support Marionette reftest implementation in wptrunner, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a7985dda6a1
Switch wptrunner to use a deque for test groups, r=maja_zf, jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85d3f3544cb
Add per-test-queue metadata to wptrunner, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/11692e3c22eb
Add Ahem to wpt fonts directory, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea37712ed33
Copy the Ahem font into the bundled font directory, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1114f4af79a
Update expectation data for reftest changes, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/a927531f40db
Use the internal reftest implementation by default on Linux and OSX, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9a4960a48a
Disable some unstable reftests on macOS, a=testonly

	Pulsebot
	Comment 122 • 7 years ago

Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70ce0f808a66
Update some metadata for stylo-e10s-debug, a=testonly

	Carsten Book [:Tomcat]
	Comment 123 • 7 years ago

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=108854017&repo=mozilla-inbound

	Pulsebot
	Comment 124 • 7 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fec6f08ea5ff
Backed out 17 changesets for various testfailures

	Andreas Tolfsen ❲:ato❳
	Comment 142 • 7 years ago
mozreview-review

Comment on attachment 8866341 [details]
Bug 1363428 - Switch wptrunner to use a deque for test groups,

https://reviewboard.mozilla.org/r/137978/#review157160

I won’t try to pretend I understand all of this, but it does look like a simpler approach.

::: testing/web-platform/tests/tools/wptrunner/wptrunner/testloader.py:615
(Diff revision 7)
> +        for test in tests:
> +            idx = hash(test.id) % processes
> +            group = queues[idx]
>              group.append(test)

Clever.

	Andreas Tolfsen ❲:ato❳
	Comment 143 • 7 years ago
mozreview-review

Comment on attachment 8866342 [details]
Bug 1363428 - Add per-test-queue metadata to wptrunner,

https://reviewboard.mozilla.org/r/137980/#review157164

	Andreas Tolfsen ❲:ato❳
	Comment 144 • 7 years ago
mozreview-review

Comment on attachment 8880559 [details]
fixup! Bug 1363428 - Update expectation data for reftest changes,

https://reviewboard.mozilla.org/r/151892/#review157170

Squash this into the previous commit before sending to autoland.

	Andreas Tolfsen ❲:ato❳
	Comment 145 • 7 years ago
mozreview-review

Comment on attachment 8880560 [details]
Bug 1363428 - Use reftest wait for MathML tests that do click onload,

https://reviewboard.mozilla.org/r/151894/#review157172

	Mozilla Autoland
	Comment 162 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c3c3eb2cde3d -d 744f286690d7: rebasing 403752:c3c3eb2cde3d "Bug 1363428 - Allow passing in an existing canvas to capture.canvas, r=ato"
rebasing 403753:ecc50712cac5 "Bug 1363428 - Allow passing flags when drawing to a canavs, r=ato"
rebasing 403754:8ca69047cf0a "Bug 1363428 - Skip element highlighting if there are no highlights, r=ato"
rebasing 403755:f9c0471fb094 "Bug 1363428 - Use an iterator for iterating windows, r=ato"
merging testing/marionette/driver.js
rebasing 403756:2c0c1ca0bd08 "Bug 1363428 - Refactor Marionette switchToWindow implementation, r=ato"
merging testing/marionette/driver.js
rebasing 403757:4615b0f28086 "Bug 1363428 - Handle top level reftest window specially in Marionette, r=ato"
merging testing/marionette/browser.js
merging testing/marionette/driver.js
rebasing 403758:cdd8ef3be1af "Bug 1363428 - Make it possible to close tabless content windows, r=ato"
merging testing/marionette/browser.js
merging testing/marionette/driver.js
rebasing 403759:3beeb19db039 "Bug 1363428 - Add reftest-specific endpoints to Marionette, r=ato"
merging testing/marionette/driver.js
merging testing/marionette/jar.mn
merging testing/marionette/listener.js
warning: conflicts while merging testing/marionette/driver.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/marionette/listener.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)