[e10s] Add AsyncUtils, an e10s version of EventUtils

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: billm, Assigned: enndeakin)

Tracking

unspecified
mozilla40
x86_64
Linux
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(e10s+, firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch async-utils (obsolete) — Splinter Review
It's currently possible to use EventUtils in e10s tests, but it only sorta works. Event dispatch is disabled from CPOWs, so functions like synthesizeMouseAtPoint don't actually work.

This patch adds async versions of these methods. The async methods don't wait for the event to actually be dispatched. For a lot of tests, that's fine.
Attachment #8562379 - Flags: review?(mconley)
It might be useful to have the methods return a promise that is resolved after the event has been dispatched. Doing this by sending an async method from the child after dispatching would mean any messages sent by event listeners in content would be dispatched first.
Bill, I'm working on something slightly related right now in Bug 1093566. Currently I'm loading a "BTU" (BrowserTestUtils) variable into the scope which will contain a lot of the methods present in the head.js files scattered throughout the tree. Example snippet:
> BrowserTestUtils.prototype = {
>   /**
>    * @param browser A xul:browser.
>    *
>    * @param ignoreSubFrames A boolean indicating if loads from subframes
>    *                        should be ignored.
>    *
>    * @return A Promise which resolves when a load event is triggered
>    *         for browser.
>    */
>   promiseBrowserLoaded(browser, ignoreSubFrames=true) {
>     return new Promise(resolve => {
>       browser.messageManager.addMessageListener("browser-test-utils:loadEvent",
>                                                  function onLoad(msg) {
>         if (!ignoreSubFrames || !msg.data.subframe) {
>           browser.messageManager.removeMessageListener(
>             "browser-test-utils:loadEvent", onLoad);
>           resolve();
>         }
>       });
>     });
>   },

I'm wondering if it'd be worth combining these into the same library, or if you think they should just be separate.
Gonna hold off on review until we hear back from billm on comment 1 and comment 2.
Flags: needinfo?(wmccloskey)
Attachment #8562379 - Flags: review?(mconley)
Posted patch Updated patch (obsolete) — Splinter Review
This version implements this with returning a promise that resolves when the mouse event is finished. I also just put it in BrowserTestUtils.jsm instead of a new file. I changed the syntax of this version of synthesizeMouse so that it can also take a selector string instead of an element to avoid using wrappers.

I'm going to try it out on a few tests before reviewing.
Depends on: 933103
Assignee: wmccloskey → enndeakin
Attachment #8562379 - Attachment is obsolete: true
Attachment #8589685 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(wmccloskey)
Attachment #8590929 - Flags: review?(mconley)
Blocks: 1100703
Comment on attachment 8590929 [details] [diff] [review]
Updated patch, improve the test a bit

Review of attachment 8590929 [details] [diff] [review]:
-----------------------------------------------------------------

My only major concern is the openNewForegroundTab bit - I'm not familiar with that method, and dxr is coming up empty. If you swap withNewTab for that, I think you're all good here, assuming the test passes.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +282,5 @@
> +   *  Versions of EventUtils.jsm synthesizeMouse functions that synthesize a
> +   *  mouse event in a child process and return promises that resolve when the
> +   *  event has fired and completed. Instead of a window, a browser is required
> +   *  to be passed to this function.
> +   * 

Nit - trailing whitespace. Same on lines 295 and 297.

@@ +321,5 @@
> +                          {object: cpowObject});
> +    });
> +  },
> +
> +  synthesizeMouseAtCenter(target, event, browser)

Nit - let's add a brief comment that the caller should refer to the synthesizeMouse documentation for arguments.

@@ +328,5 @@
> +    event.centered = true;
> +    return BrowserTestUtils.synthesizeMouse(target, 0, 0, event, browser);
> +  },
> +
> +  synthesizeMouseAtPoint(offsetX, offsetY, event, browser)

Same as above.

::: testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js
@@ +25,5 @@
> +
> +  let left = data.x;
> +  let top = data.y;
> +  if (target) {
> +    var rect = target.getBoundingClientRect();

let, not var

::: testing/mochitest/tests/browser/browser_BrowserTestUtils.js
@@ +6,5 @@
> +}
> +
> +add_task(function* () {
> +  let onClickEvt = 'document.getElementById("out").textContent = event.target.localName + "," + event.clientX + "," + event.clientY;'
> +  const url = "<body onclick='" + onClickEvt + "' style='margin: 0'>" +

We could probably make this more readable with a template string - example:

const url = `
<body onclick="${onClickEvt}" style="margin: 0;">
  <button id="one" style="margin: 0; margin-left: 16px; margin-top: 15px; width: 30px; height: 40px;">
    Test
  </button>
  <div onmousedown="event.preventDefault();" style="margin: 0; width: 80px; height: 60px;">
    Other
  </div>
  <span id="out"></span>
</body>`;

@@ +10,5 @@
> +  const url = "<body onclick='" + onClickEvt + "' style='margin: 0'>" +
> +              "<button id='one' style='margin: 0; margin-left: 16px; margin-top: 14px; width: 30px; height: 40px;'>Test</button>" +
> +              "<div onmousedown='event.preventDefault()' style='margin: 0; width: 80px; height: 60px;'>Other</div>" +
> +              "<span id='out'></span></body>";
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "data:text/html," + url);

I'm not sure where openNewForegroundTab is defined...

Since you don't care about the tab itself, you might want to use BrowserTestUtils.withNewTab, which will open and close the tab for you, along with making sure the tab is loaded.

@@ +34,5 @@
> +  yield BrowserTestUtils.synthesizeMouseAtCenter("body > div", {}, browser);
> +  details = yield getLastEventDetails(browser);
> +  is(details, "div,40,84", "synthesizeMouseAtCenter with complex selector");
> +
> +  let result = yield BrowserTestUtils.synthesizeMouseAtCenter("body > div", { type: "mousedown" }, browser);

Let's rename result to "cancelled", to make it clearer what we're getting back.
Attachment #8590929 - Flags: review?(mconley) → review+
No longer depends on: 933103
https://hg.mozilla.org/mozilla-central/rev/4ddadc870ef6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Hi Neil, can you provide a point value.
Iteration: --- → 40.2 - 27 Apr
Flags: needinfo?(enndeakin)
Flags: firefox-backlog+
Points: --- → 3
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.