1074836 - Enable devtools/inspector tests with e10s

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Comment 1

[Patrick Brosset <:pbro>]

I may actually handle this in bug 985597. In any case, since I'm making a lot of inspector test changes right now, marking this bug as assigned to me so that noone starts working on this in parallel.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug1074836-inspector-e10s-tests v1.patch

Makes all inspector tests pass locally with and without e10s.
Of course it's a little bit early for this patch yet, I still need to land the new document canvasFrame API (bug 1020244) and then the new highlighter that uses this API (bug 985597).

Comment 3

[Chris Peterson [:cpeterson]]

Moving DevTools test bugs to e10s milestone M7 (i.e. not blocking e10s merge to Aurora).

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8510131 [details] [diff] [review]
bug1074836-inspector-e10s-tests v1.patch

Marking this old patch as obsolete. I'm about to upload a new series of patches that should fix the inspector tests with e10s.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1074836-e10s-inspector-1-using-connPrefix-to-find-highlighter v1.patch

Hey Mike, remember when I introduced a way to retrieve the highlighter actor via frame scripts during inspector tests? Well, turns out it needed a minor adjustment with e10s: the actorID alone was not enough to retrieve the right actor, we also need to have the debugger server connection prefix to retrieve the right connection first.
So this patch changes just this. Of course this implied lot's of changes throughout many tests, but these changes are simple.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug1074836-e10s-inspector-2-disable-gcli-test.patch

R+ this patch myself, since this is just disabling a gcli test with e10s, knowing that gcli isn't fully remoted yet (pending bug 1128988).

Comment 7

[Patrick Brosset <:pbro>]

Attachment: bug1074836-e10s-inspector-3-browser-ctx-menu v1.patch

This fixes the browser context-menu test with e10s.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug1074836-e10s-inspector-4-delete-iframe-during-load v1.patch

This fixes the test that created and deleted iframes quickly (this use to make the inspector go blank).
I had to introduce a new custom event for this, because simply listening to the window load event with e10s wasn't working. My listener was getting called before the test page load event handler was done.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug1074836-e10s-inspector-5-skip-scrolling-test.patch

R+'ing this patch myself since this is just disabling a test with e10s.
This test is simulating mouse wheel events in content, and according to bug 1035661, it's not really useful anyway, so instead of loosing time trying to make it work with e10s, I'm just disabling it for now.

Comment 10

[Patrick Brosset <:pbro>]

Attachment: bug1074836-e10s-inspector-6-search-field-test v1.patch

This fixes the inspector search field test with e10s. This test was accessing the raw DOMNode using `inspector.selection.node` which doesn't exist with e10s.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug1074836-e10s-inspector-7-reselect-node-test v1.patch

Same as the previous patch, this one fixes the node reselection test with e10s by making it access nodeFronts rather than raw DOMNodes.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug1074836-e10s-inspector-8-enable-tests.patch

And here is the last patch, which simply deletes the main |skip-if=e10s| line in browser.ini
R+ing this myself.

Comment 13

[Patrick Brosset <:pbro>]

Pending try build with all patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dd89db5ab2e

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8559024 [details] [diff] [review]
bug1074836-e10s-inspector-3-browser-ctx-menu v1.patch

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

I think there should be a new event listener in the frame script for opening the context menu on a node and clicking inspect element

::: browser/devtools/inspector/test/browser_inspector_initialization.js
@@ +112,5 @@
>    is(button.getAttribute("tooltiptext"), expectedText, "Crumb refers to the right node");
>  }
>  
>  function* clickOnInspectMenuItem(node) {
> +  info("Showing the contextual menu on node " + node);

This whole set of things all the way through to the contextMenu.hiding(); call looks like something that should happen in a frame script - Test:InspectElementWithContextMenu.  Then we wouldn't need the try / catch and expectation that the popupNode property is not used in e10s.

::: browser/devtools/inspector/test/doc_frame_script.js
@@ +227,5 @@
>   * - {Number} x
>   * - {Number} y
>   * - {Boolean} center If set to true, x/y will be ignored and
>   *             synthesizeMouseAtCenter will be used instead
> + * - {Object} options Other event options

Depending on which options we care about / want to support (I've only seend 'type' and 'button'), it may be simpler for the callers if you just flattened those two things out into the msg object.  Remembering which ones belong in options and which belong in the msg may be a cause for headaches later.  But then again, it does match the synthesizeMouse params so no big deal either way.

Comment 15

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> ::: browser/devtools/inspector/test/browser_inspector_initialization.js
> @@ +112,5 @@
> >    is(button.getAttribute("tooltiptext"), expectedText, "Crumb refers to the right node");
> >  }
> >  
> >  function* clickOnInspectMenuItem(node) {
> > +  info("Showing the contextual menu on node " + node);
> 
> This whole set of things all the way through to the contextMenu.hiding();
> call looks like something that should happen in a frame script -
> Test:InspectElementWithContextMenu.  Then we wouldn't need the try / catch
> and expectation that the popupNode property is not used in e10s.
The problem with this is that the contextmenu DOM lives in the parent process, inside the browser chrome.
On this line for instance:
  let contentAreaContextMenu = document.querySelector("#contentAreaContextMenu");
document is actually the browser document, and #contentAreaContextMenu is a node in the browser chrome.
I wouldn't be able to do this from the frame script, in the content process.

Comment 16

[Patrick Brosset <:pbro>]

NI?ing Brian for comment 15.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > ::: browser/devtools/inspector/test/browser_inspector_initialization.js
> > @@ +112,5 @@
> > >    is(button.getAttribute("tooltiptext"), expectedText, "Crumb refers to the right node");
> > >  }
> > >  
> > >  function* clickOnInspectMenuItem(node) {
> > > +  info("Showing the contextual menu on node " + node);
> > 
> > This whole set of things all the way through to the contextMenu.hiding();
> > call looks like something that should happen in a frame script -
> > Test:InspectElementWithContextMenu.  Then we wouldn't need the try / catch
> > and expectation that the popupNode property is not used in e10s.
> The problem with this is that the contextmenu DOM lives in the parent
> process, inside the browser chrome.
> On this line for instance:
>   let contentAreaContextMenu =
> document.querySelector("#contentAreaContextMenu");
> document is actually the browser document, and #contentAreaContextMenu is a
> node in the browser chrome.
> I wouldn't be able to do this from the frame script, in the content process.

Ah, right - missed the context of that document

Comment 18

[Patrick Brosset <:pbro>]

fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/96f90a6ae1d8
remote:   https://hg.mozilla.org/integration/fx-team/rev/44098ae1e629
remote:   https://hg.mozilla.org/integration/fx-team/rev/c7b35c86dc91
remote:   https://hg.mozilla.org/integration/fx-team/rev/07f0a2371de3
remote:   https://hg.mozilla.org/integration/fx-team/rev/8e63c27b3f55
remote:   https://hg.mozilla.org/integration/fx-team/rev/a938bb2c39cc
remote:   https://hg.mozilla.org/integration/fx-team/rev/5e9e37a5c8f9
remote:   https://hg.mozilla.org/integration/fx-team/rev/082a3a45d24e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96f90a6ae1d8
https://hg.mozilla.org/mozilla-central/rev/44098ae1e629
https://hg.mozilla.org/mozilla-central/rev/c7b35c86dc91
https://hg.mozilla.org/mozilla-central/rev/07f0a2371de3
https://hg.mozilla.org/mozilla-central/rev/8e63c27b3f55
https://hg.mozilla.org/mozilla-central/rev/a938bb2c39cc
https://hg.mozilla.org/mozilla-central/rev/5e9e37a5c8f9
https://hg.mozilla.org/mozilla-central/rev/082a3a45d24e