Closed
Bug 1034601
Opened 9 years ago
Closed 9 years ago
Enable devtools/markupview tests with e10s
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 33
People
(Reporter: pbro, Assigned: pbro)
References
Details
Attachments
(1 file, 2 obsolete files)
86.21 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → pbrosset
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
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
Attachment #8454353 -
Attachment is obsolete: true
Attachment #8456756 -
Flags: review?(bgrinstead)
Comment 3•9 years ago
|
||
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
Attachment #8456756 -
Flags: review?(bgrinstead) → review+
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
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
Attachment #8456756 -
Attachment is obsolete: true
Attachment #8457459 -
Flags: review+
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4118687bbaeb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•