Closed Bug 1034601 Opened 7 years ago Closed 7 years ago

Enable devtools/markupview tests with e10s


(DevTools :: Inspector, defect)

Not set


(Not tracked)

Firefox 33


(Reporter: pbro, Assigned: pbro)




(1 file, 2 obsolete files)

No description provided.
Blocks: dte10s
Assignee: nobody → pbrosset
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
- 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:
Attachment #8454353 - Attachment is obsolete: true
Attachment #8456756 - Flags: review?(bgrinstead)
Blocks: 1030735
Comment on attachment 8456756 [details] [diff] [review]
bug1034601-enable-markupview-test-in-e10s-mode v2.patch

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


::: 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


::: 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([]".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
Attachment #8456756 - Flags: review?(bgrinstead) → review+
Thanks Brian for the review.
Here's v3 which addresses all your comments.

Pushed to fx-team:
Attachment #8456756 - Attachment is obsolete: true
Attachment #8457459 - Flags: review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.