Closed Bug 1363428 Opened 2 years ago Closed 2 years ago

Implement a fast reftest backend for wptrunner

Categories

(Testing :: web-platform-tests, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jgraham, Assigned: jgraham)

References

(Blocks 1 open bug)

Details

Attachments

(16 files, 3 obsolete files)

59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
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).
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.
Attachment #8866333 - Flags: review?(ato) → 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.
Attachment #8866334 - Flags: review?(ato) → review-
Comment on attachment 8866335 [details]
Bug 1363428 - Skip element highlighting if there are no highlights,

https://reviewboard.mozilla.org/r/137966/#review142186
Attachment #8866335 - Flags: review?(ato) → 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.
Attachment #8866336 - Flags: review?(ato) → review-
Comment on attachment 8866336 [details]
Bug 1363428 - Use an iterator for iterating windows,

https://reviewboard.mozilla.org/r/137968/#review142188
Attachment #8866336 - Flags: review- → 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)
Attachment #8866337 - Flags: review?(ato) → 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.
Attachment #8866338 - Flags: review?(ato) → 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.
Attachment #8866339 - Flags: review?(ato) → 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/
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.
Attachment #8866341 - Flags: review?(josh) → review+
Comment on attachment 8866340 [details]
Bug 1363428 - Support Marionette reftest implementation in wptrunner,

https://reviewboard.mozilla.org/r/137976/#review142780
Attachment #8866340 - Flags: review?(mjzffr) → review+
Comment on attachment 8866342 [details]
Bug 1363428 - Add per-test-queue metadata to wptrunner,

https://reviewboard.mozilla.org/r/137980/#review142782
Attachment #8866342 - Flags: review?(mjzffr) → review+
Attachment #8866338 - Flags: review?(hskupin)
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.
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
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.
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.
Attachment #8866338 - Flags: review?(hskupin) → review-
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
Flags: needinfo?(dbaron)
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.
Attachment #8866333 - Flags: review?(ato) → review+
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.
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.
Attachment #8866334 - Flags: review?(ato) → 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.
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.
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")
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’.
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 {.
Attachment #8866339 - Flags: review?(ato) → review-
(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.
Flags: needinfo?(dbaron)
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.
Attachment #8866338 - Flags: review-
Blocks: 1369596
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?
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.
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.
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.
Comment on attachment 8876230 [details]
Bug 1363428 - Add Ahem to wpt fonts directory,

https://reviewboard.mozilla.org/r/147680/#review152058
Attachment #8876230 - Flags: review?(mjzffr) → review+
Comment on attachment 8876231 [details]
Bug 1363428 - Copy the Ahem font into the bundled font directory,

https://reviewboard.mozilla.org/r/147682/#review152060
Attachment #8876231 - Flags: review?(mjzffr) → review+
Comment on attachment 8876229 [details]
Bug 1363428 - Make it possible to close tabless content windows,

https://reviewboard.mozilla.org/r/147678/#review152370
Attachment #8876229 - Flags: review?(ato) → review+
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;
> …`;
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.
Attachment #8866339 - Flags: review?(ato) → review+
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?
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.
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.
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?
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!
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.
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?
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.
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.
Attachment #8866341 - Flags: review?(mjzffr)
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.
Attachment #8879192 - Flags: review?(ato) → 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
Attachment #8879193 - Flags: review?(ato) → review+
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"});
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.
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.
Comment on attachment 8879192 [details]
Bug 1363428 - Update expectation data for reftest changes,

https://reviewboard.mozilla.org/r/150528/#review155622
Attachment #8879192 - Flags: review?(ato) → review+
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
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70ce0f808a66
Update some metadata for stylo-e10s-debug, a=testonly
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fec6f08ea5ff
Backed out 17 changesets for various testfailures
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.
Attachment #8866341 - Flags: review?(ato) → review+
Comment on attachment 8866342 [details]
Bug 1363428 - Add per-test-queue metadata to wptrunner,

https://reviewboard.mozilla.org/r/137980/#review157164
Attachment #8866342 - Flags: review?(ato) → 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.
Attachment #8880559 - Flags: review?(ato) → 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
Attachment #8880560 - Flags: review?(ato) → review+
Attachment #8880559 - Attachment is obsolete: true
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)
Attachment #8880871 - Attachment is obsolete: true
Attachment #8880872 - Attachment is obsolete: true
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/956d6c0a646e
Allow passing in an existing canvas to capture.canvas, r=ato
https://hg.mozilla.org/integration/autoland/rev/1d359561373c
Allow passing flags when drawing to a canavs, r=ato
https://hg.mozilla.org/integration/autoland/rev/f0b218979773
Skip element highlighting if there are no highlights, r=ato
https://hg.mozilla.org/integration/autoland/rev/b0604d88973f
Use an iterator for iterating windows, r=ato
https://hg.mozilla.org/integration/autoland/rev/60c1c95b46ca
Refactor Marionette switchToWindow implementation, r=ato
https://hg.mozilla.org/integration/autoland/rev/d1c03b96d270
Handle top level reftest window specially in Marionette, r=ato
https://hg.mozilla.org/integration/autoland/rev/c27b94038e71
Make it possible to close tabless content windows, r=ato
https://hg.mozilla.org/integration/autoland/rev/ec4a6b343b37
Add reftest-specific endpoints to Marionette, r=ato
https://hg.mozilla.org/integration/autoland/rev/a94d2c400b04
Support Marionette reftest implementation in wptrunner, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/829c13a75667
Switch wptrunner to use a deque for test groups, r=ato,jdm
https://hg.mozilla.org/integration/autoland/rev/7fa8e20fe001
Add per-test-queue metadata to wptrunner, r=ato,maja_zf
https://hg.mozilla.org/integration/autoland/rev/26ae2fd48587
Add Ahem to wpt fonts directory, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/582a8dce7932
Copy the Ahem font into the bundled font directory, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/858dc97498c7
Update expectation data for reftest changes, r=ato
https://hg.mozilla.org/integration/autoland/rev/a0687a63e7b9
Use the internal reftest implementation by default on Linux and OSX, r=ato
https://hg.mozilla.org/integration/autoland/rev/e86d6d5c2a25
Use reftest wait for MathML tests that do click onload, r=ato
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/07dea5f003af
Allow passing in an existing canvas to capture.canvas, r=ato
https://hg.mozilla.org/integration/autoland/rev/2e39f019d24b
Allow passing flags when drawing to a canavs, r=ato
https://hg.mozilla.org/integration/autoland/rev/329b56faf63a
Skip element highlighting if there are no highlights, r=ato
https://hg.mozilla.org/integration/autoland/rev/5caceeb8533e
Use an iterator for iterating windows, r=ato
https://hg.mozilla.org/integration/autoland/rev/5415a7f8d8f8
Refactor Marionette switchToWindow implementation, r=ato
https://hg.mozilla.org/integration/autoland/rev/900508554bc0
Handle top level reftest window specially in Marionette, r=ato
https://hg.mozilla.org/integration/autoland/rev/86b5f18bda26
Make it possible to close tabless content windows, r=ato
https://hg.mozilla.org/integration/autoland/rev/977f1506ce7f
Add reftest-specific endpoints to Marionette, r=ato
https://hg.mozilla.org/integration/autoland/rev/2981e39e3650
Support Marionette reftest implementation in wptrunner, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/f72ba3ff2fc7
Switch wptrunner to use a deque for test groups, r=ato,jdm
https://hg.mozilla.org/integration/autoland/rev/2aa7e487bfec
Add per-test-queue metadata to wptrunner, r=ato,maja_zf
https://hg.mozilla.org/integration/autoland/rev/73eb0b39e0c7
Add Ahem to wpt fonts directory, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/7b14625d67d2
Copy the Ahem font into the bundled font directory, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/34560c4d12c2
Update expectation data for reftest changes, r=ato
https://hg.mozilla.org/integration/autoland/rev/edb062bf8c35
Use the internal reftest implementation by default on Linux and OSX, r=ato
https://hg.mozilla.org/integration/autoland/rev/cee08328318a
Use reftest wait for MathML tests that do click onload, r=ato
Flags: needinfo?(james)
Blocks: 1378398
Nope!
Flags: needinfo?(james)
Assignee: nobody → james
You need to log in before you can comment on or make changes to this bug.