1034601 - Enable devtools/markupview tests with e10s

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1034601-enable-markupview-test-in-e10s-mode v1.patch

v1
Tests still pass in non-e10s mode and now also pass in e10s mode.
Note that:
- browser_markupview_highlight_hover_01/2/3.js tests have been disabled in e10s as the highlighter doesn't work with e10s
- browser_markupview_pagesize_02.js is also disabled due to the markup-view bug (filed bug 1036409)
- browser_markupview_tag_edit_03.js disabled too as tagnames cannot be edited remotely (bug 1036421)
- browser_markupview_mutation_01.js seems to be leaking in e10s only
- exceptions appear in the console when tests end

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug1034601-enable-markupview-test-in-e10s-mode v2.patch

v2:
- No more exceptions are reported when tests end: turns out registerCleanupFunction is a Task, so it accepts a generator function that yields promises. Thanks to this, we can now make sure the toolbox is closed before the tab, avoiding all sorts of exceptions when the test ends.
- The leak I reported earlier is fixed. I ended up refactoring the test so it doesn't require a second tab to parse HTML. I don't exactly know why this leak came to be, but this test had always been a bit funky anyway, I think it makes more sense now.

Ready for review.
Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=8025880f02a3

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8456756 [details] [diff] [review]
bug1034601-enable-markupview-test-in-e10s-mode v2.patch

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

Nice!

::: browser/devtools/markupview/test/browser.ini
@@ +38,5 @@
>  [browser_markupview_search_01.js]
>  [browser_markupview_tag_edit_01.js]
>  [browser_markupview_tag_edit_02.js]
>  [browser_markupview_tag_edit_03.js]
> +skip-if = e10s # Bug 1036421 - Tag edition isn't remote-safe

s/edition/editing

::: browser/devtools/markupview/test/browser_markupview_css_completion_style_attribute.js
@@ +69,5 @@
>  
>    yield inspector.markup.expandAll();
>  
> +  let nodeFront = yield getNodeFront("#node14", inspector);
> +  let node = getContainerForNodeFront(nodeFront, inspector).editor;

I think it would be more clear to do

let container = getContainerForNodeFront(nodeFront, inspector);

then switch node.newAttr to container.editor.newAttr (the 'node' variable name here is misleading since it is set to an editor).

::: browser/devtools/markupview/test/browser_markupview_image_tooltip.js
@@ +61,1 @@
>    let isImg = node.tagName.toLowerCase() === "img";

Could use nodeFront.tagName and get rid of the node variable

::: browser/devtools/markupview/test/browser_markupview_mutation_01.js
@@ +20,5 @@
>        node1.setAttribute("newattr", "newattrval");
> +    },
> +    check: function*(inspector) {
> +      let {editor} = yield getContainerForSelector("#node1", inspector);
> +      ok([].some.call(editor.attrList.querySelectorAll(".attreditor"), attr => {

Doesn't matter one way or another, but I've generally been using [...editor.attrList.querySelectorAll(".attreditor")].some() which I think is a bit easier to read

::: browser/devtools/markupview/test/head.js
@@ +55,5 @@
>  function addTab(url) {
>    info("Adding a new tab with URL: '" + url + "'");
>    let def = promise.defer();
>  
> +  window.focus();

It appears that Bug 921935 should bring waitForFocus() support to e10s, which would probably cover the case of the test losing focus when the page is loading.  I don't think this should block landing this as it won't ever happen on the test servers, but something to keep in mind (and maybe add a comment about).

@@ -153,4 @@
>  }
>  
>  /**
>   * Set the inspector's current selection to a node or to the first match of the

You should be able to delete the function selectNode() since it is replaced with the function expression below?

@@ -302,2 @@
>  
> -  let container = getContainerForRawNode(nodeOrSelector, inspector);

You can delete the function declaration getContainerForRawNode in this file, since it is no longer used

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1034601-enable-markupview-test-in-e10s-mode v3.patch

Thanks Brian for the review.
Here's v3 which addresses all your comments.

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/4118687bbaeb

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4118687bbaeb