1363428 - Implement a fast reftest backend for wptrunner

Status: RESOLVED FIXED

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

Description

[James Graham [:jgraham]]

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 1

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Allow passing in an existing canvas to capture.canvas,

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

Comment 2

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Allow passing flags when drawing to a canavs,

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

Comment 3

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Skip element highlighting if there are no highlights,

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

Comment 4

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Use an iterator for iterating windows,

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

Comment 5

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Refactor Marionette switchToWindow implementation,

This allows other commands to implicitly change the window handle, by
calling findWindow with appropriate arguments to get a window
properties object and then passing that to setWindowHandle.

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

Comment 6

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Handle top level reftest window specially in Marionette,

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.

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

Comment 7

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Add reftest-specific endpoints to Marionette,

This adds commands to start a reftest session, run a test, and end the
session. It as assumed that after you start a reftest session you will
just run reftests until you end the session. When starting a session
the user provides a string indicating when screenshots should be
taken, and an object mapping urls to a count of the number of times
that url is expected to be used in the session, to help with
caching. Running the tests takes a url to a test, an expected status,
a timeout, and a nested list of possible references, in which each
entry at a specific level is combined by OR and nested references are
combined by AND.

The implementation is heavilly inspired by the existing reftest
harness, starting a minimal window with no tabs, and loading the urls
directly in there. In order to get a screenshot in the e10s case we
have to pass the DRAW_VIEW and USE_WIDGET_LAYERS flags when taking the
screenshot.

For performance we heavily cache canvases; for references that will be
repeated we cache the full canvas with image, and we also cache a
single canvas to use for all other screenshots to avoid the overhead
of repeatedly creating a new canvas element.

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

Comment 8

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Support Marionette reftest implementation in wptrunner,

Add an InternalReftestImplmentation that runs reftests using the
built-in marionette reftest primitives rather than screenshots. This
is actiivated using the --internal-reftest switch, although that may
become the default in the future.

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

Comment 9

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Switch wptrunner to use a deque for test groups,

Initially wptrunner had a single test queue that was shared between
all processes. Then for --run-by-dir it changed to a queue of
queues. This change makes it a queue of deques, which is simpler,
since the test queues themselves are no longer shared between
processes. It also changes the implementation when we aren't using
--run-by-dir but are using multiple processes to pre-group the tests
into N queues rather than sharing a single queue between all
processes. This is necessary to use the deque of course, but
importantly anticipates a change in which we will pre-compute per
queue metdata for each queue; that doesn't work well with one shared
queue. The downside of this change is that there is no work stealing,
so it may be less efficient if we randomly assign many slow jobs to
one particular process.

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

Comment 10

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Add per-test-queue metadata to wptrunner,

This adds a metadata object associated with each test queue, and uses
it to pass cache information into the marionette internal reftest
implementation so that we are able to cache only those canvases that
will be reused.

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

Comment 11

[Andreas Tolfsen ❲:ato❳]

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.

Comment 12

[Andreas Tolfsen ❲:ato❳]

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.

Comment 13

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 14

[Andreas Tolfsen ❲:ato❳]

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.

Comment 15

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 16

[Andreas Tolfsen ❲:ato❳]

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)

Comment 17

[Andreas Tolfsen ❲:ato❳]

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.

Comment 18

[Andreas Tolfsen ❲:ato❳]

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.

Comment 19

[Andreas Tolfsen ❲:ato❳]

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 20

[Josh Matthews [:jdm]]

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.

Comment 21

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

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

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

Comment 22

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

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

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

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 24

[James Graham [:jgraham]]

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 25

[James Graham [:jgraham]]

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 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 27

[James Graham [:jgraham]]

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

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

Comment 28

[James Graham [:jgraham]]

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

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

Comment 29

[James Graham [:jgraham]]

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

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

Comment 30

[James Graham [:jgraham]]

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

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

Comment 31

[James Graham [:jgraham]]

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

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

Comment 32

[James Graham [:jgraham]]

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

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

Comment 33

[James Graham [:jgraham]]

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

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

Comment 34

[James Graham [:jgraham]]

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

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

Comment 35

[James Graham [:jgraham]]

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

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

Comment 36

[James Graham [:jgraham]]

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

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

Comment 37

[James Graham [:jgraham]]

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

Comment 38

[Andreas Tolfsen ❲:ato❳]

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.

Comment 39

[Andreas Tolfsen ❲:ato❳]

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 40

[Andreas Tolfsen ❲:ato❳]

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.

Comment 41

[Andreas Tolfsen ❲:ato❳]

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 42

[Andreas Tolfsen ❲:ato❳]

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 43

[Andreas Tolfsen ❲:ato❳]

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 44

[Andreas Tolfsen ❲:ato❳]

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 45

[Andreas Tolfsen ❲:ato❳]

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 {.

Comment 46

[David Baron :dbaron:]

(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.

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 48

[James Graham [:jgraham]]

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 49

[James Graham [:jgraham]]

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 50

[James Graham [:jgraham]]

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 51

[James Graham [:jgraham]]

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 52

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Make it possible to close tabless content windows,

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

Comment 53

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Add Ahem to wpt fonts directory,

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

Comment 54

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Copy the Ahem font into the bundled font directory,

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

Comment 55

[James Graham [:jgraham]]

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

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

Comment 56

[James Graham [:jgraham]]

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

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

Comment 57

[James Graham [:jgraham]]

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

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

Comment 58

[James Graham [:jgraham]]

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

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

Comment 59

[James Graham [:jgraham]]

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

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

Comment 60

[James Graham [:jgraham]]

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

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

Comment 61

[James Graham [:jgraham]]

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

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

Comment 62

[James Graham [:jgraham]]

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

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

Comment 63

[James Graham [:jgraham]]

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

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

Comment 64

[James Graham [:jgraham]]

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

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

Comment 65

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

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

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

Comment 66

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

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

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

Comment 67

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 68

[Andreas Tolfsen ❲:ato❳]

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 69

[Andreas Tolfsen ❲:ato❳]

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.

Comment 70

[Andreas Tolfsen ❲:ato❳]

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 71

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 72

[Andreas Tolfsen ❲:ato❳]

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 73

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 74

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 75

[Andreas Tolfsen ❲:ato❳]

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 76

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 77

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 78

[James Graham [:jgraham]]

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

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

Comment 79

[James Graham [:jgraham]]

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

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

Comment 80

[James Graham [:jgraham]]

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

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

Comment 81

[James Graham [:jgraham]]

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

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

Comment 82

[James Graham [:jgraham]]

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

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

Comment 83

[James Graham [:jgraham]]

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

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

Comment 84

[James Graham [:jgraham]]

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

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

Comment 85

[James Graham [:jgraham]]

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

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

Comment 86

[James Graham [:jgraham]]

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

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

Comment 87

[James Graham [:jgraham]]

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

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

Comment 88

[James Graham [:jgraham]]

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

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

Comment 89

[James Graham [:jgraham]]

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

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

Comment 90

[James Graham [:jgraham]]

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

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

Comment 91

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

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.

Comment 92

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Update expectation data for reftest changes,

The internal reftest harness changes some results, but it's not clear
that it's overall better or worse than the old one, so just update the
expectation metadata for now.

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

Comment 93

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Use the internal reftest implementation by default on Linux and OSX,

This is faster and works better in some cases (e.g. with SVG). However
it doesn't work correctly on Windows yet, so remains disabled there by default.

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

Comment 94

[James Graham [:jgraham]]

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

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

Comment 95

[James Graham [:jgraham]]

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

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

Comment 96

[James Graham [:jgraham]]

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

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

Comment 97

[James Graham [:jgraham]]

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

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

Comment 98

[James Graham [:jgraham]]

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

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

Comment 99

[James Graham [:jgraham]]

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

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

Comment 100

[James Graham [:jgraham]]

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

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

Comment 101

[James Graham [:jgraham]]

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

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

Comment 102

[James Graham [:jgraham]]

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

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

Comment 103

[James Graham [:jgraham]]

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

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

Comment 104

[James Graham [:jgraham]]

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

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

Comment 105

[James Graham [:jgraham]]

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

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

Comment 106

[James Graham [:jgraham]]

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

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

Comment 107

[Andreas Tolfsen ❲:ato❳]

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.

Comment 108

[Andreas Tolfsen ❲:ato❳]

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

Comment 109

[Andreas Tolfsen ❲:ato❳]

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 110

[Andreas Tolfsen ❲:ato❳]

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 111

[Andreas Tolfsen ❲:ato❳]

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 112

[James Graham [:jgraham]]

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

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

Comment 113

[James Graham [:jgraham]]

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

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

Comment 114

[James Graham [:jgraham]]

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

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

Comment 115

[James Graham [:jgraham]]

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

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

Comment 116

[James Graham [:jgraham]]

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

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

Comment 117

[James Graham [:jgraham]]

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

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

Comment 118

[James Graham [:jgraham]]

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

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

Comment 119

[James Graham [:jgraham]]

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

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

Comment 120

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 121

[Pulsebot]

Pushed by [e-mail]:
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

Comment 122

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70ce0f808a66
Update some metadata for stylo-e10s-debug, a=testonly

Comment 123

[Carsten Book [:Tomcat]]

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

Comment 124

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fec6f08ea5ff
Backed out 17 changesets for various testfailures

Comment 125

[James Graham [:jgraham]]

Attachment: fixup! Bug 1363428 - Update expectation data for reftest changes,

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

Comment 126

[James Graham [:jgraham]]

Attachment: Bug 1363428 - Use reftest wait for MathML tests that do click onload,

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

Comment 127

[James Graham [:jgraham]]

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

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

Comment 128

[James Graham [:jgraham]]

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

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

Comment 129

[James Graham [:jgraham]]

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

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

Comment 130

[James Graham [:jgraham]]

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

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

Comment 131

[James Graham [:jgraham]]

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

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

Comment 132

[James Graham [:jgraham]]

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

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

Comment 133

[James Graham [:jgraham]]

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

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

Comment 134

[James Graham [:jgraham]]

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

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

Comment 135

[James Graham [:jgraham]]

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

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

Comment 136

[James Graham [:jgraham]]

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

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

Comment 137

[James Graham [:jgraham]]

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

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

Comment 138

[James Graham [:jgraham]]

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

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

Comment 139

[James Graham [:jgraham]]

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

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

Comment 140

[James Graham [:jgraham]]

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

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

Comment 141

[James Graham [:jgraham]]

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

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

Comment 142

[Andreas Tolfsen ❲:ato❳]

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.

Comment 143

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 144

[Andreas Tolfsen ❲:ato❳]

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.

Comment 145

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 146

[James Graham [:jgraham]]

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

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

Comment 147

[James Graham [:jgraham]]

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

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

Comment 148

[James Graham [:jgraham]]

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

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

Comment 149

[James Graham [:jgraham]]

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

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

Comment 150

[James Graham [:jgraham]]

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

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

Comment 151

[James Graham [:jgraham]]

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

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

Comment 152

[James Graham [:jgraham]]

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

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

Comment 153

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137974/diff/7-8/

Comment 154

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137976/diff/7-8/

Comment 155

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137978/diff/7-8/

Comment 156

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137980/diff/7-8/

Comment 157

[James Graham [:jgraham]]

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

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

Comment 158

[James Graham [:jgraham]]

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

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

Comment 159

[James Graham [:jgraham]]

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

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

Comment 160

[James Graham [:jgraham]]

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

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

Comment 161

[James Graham [:jgraham]]

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

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

Comment 162

[Mozilla Autoland]

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)

Comment 163

[James Graham [:jgraham]]

Attachment: No bug, Automated HSTS preload list update from host bld-linux64-spot-304 - a=hsts-update

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

Comment 164

[James Graham [:jgraham]]

Attachment: No bug, Automated HPKP preload list update from host bld-linux64-spot-304 - a=hpkp-update

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

Comment 165

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137962/diff/7-8/

Comment 166

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137964/diff/7-8/

Comment 167

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137966/diff/7-8/

Comment 168

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137968/diff/7-8/

Comment 169

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137970/diff/7-8/

Comment 170

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137972/diff/7-8/

Comment 171

[James Graham [:jgraham]]

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

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

Comment 172

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137974/diff/8-9/

Comment 173

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137976/diff/8-9/

Comment 174

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137978/diff/8-9/

Comment 175

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137980/diff/8-9/

Comment 176

[James Graham [:jgraham]]

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

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

Comment 177

[James Graham [:jgraham]]

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

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

Comment 178

[James Graham [:jgraham]]

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

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

Comment 179

[James Graham [:jgraham]]

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

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

Comment 180

[James Graham [:jgraham]]

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

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

Comment 181

[Pulsebot]

Pushed by [e-mail]:
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

Comment 182

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out in https://hg.mozilla.org/integration/autoland/rev/43e7c38004a8b8f23489d9076f84544b1b4b9f8a for Wr unexpected passes like https://treeherder.mozilla.org/logviewer.html#?job_id=109641312&repo=autoland

Comment 183

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137962/diff/8-9/

Comment 184

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137964/diff/8-9/

Comment 185

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137966/diff/8-9/

Comment 186

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137968/diff/8-9/

Comment 187

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137970/diff/8-9/

Comment 188

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137972/diff/8-9/

Comment 189

[James Graham [:jgraham]]

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

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

Comment 190

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137974/diff/9-10/

Comment 191

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137976/diff/9-10/

Comment 192

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137978/diff/9-10/

Comment 193

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137980/diff/9-10/

Comment 194

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147680/diff/7-8/

Comment 195

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147682/diff/7-8/

Comment 196

[James Graham [:jgraham]]

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

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

Comment 197

[James Graham [:jgraham]]

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

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

Comment 198

[James Graham [:jgraham]]

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

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

Comment 199

[Pulsebot]

Pushed by [e-mail]:
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

Comment 200

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07dea5f003af
https://hg.mozilla.org/mozilla-central/rev/2e39f019d24b
https://hg.mozilla.org/mozilla-central/rev/329b56faf63a
https://hg.mozilla.org/mozilla-central/rev/5caceeb8533e
https://hg.mozilla.org/mozilla-central/rev/5415a7f8d8f8
https://hg.mozilla.org/mozilla-central/rev/900508554bc0
https://hg.mozilla.org/mozilla-central/rev/86b5f18bda26
https://hg.mozilla.org/mozilla-central/rev/977f1506ce7f
https://hg.mozilla.org/mozilla-central/rev/2981e39e3650
https://hg.mozilla.org/mozilla-central/rev/f72ba3ff2fc7
https://hg.mozilla.org/mozilla-central/rev/2aa7e487bfec
https://hg.mozilla.org/mozilla-central/rev/73eb0b39e0c7
https://hg.mozilla.org/mozilla-central/rev/7b14625d67d2
https://hg.mozilla.org/mozilla-central/rev/34560c4d12c2
https://hg.mozilla.org/mozilla-central/rev/edb062bf8c35
https://hg.mozilla.org/mozilla-central/rev/cee08328318a

Comment 201

[Kartikaya Gupta (email:[e-mail])]

http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/layout/tools/reftest/reftest-content.js#450

Is this dump intentional?

Comment 202

[James Graham [:jgraham]]

Nope!