Closed
Bug 1347490
Opened 7 years ago
Closed 7 years ago
Return isInTree in node grip preview to check if a Node is currently in the DOM tree
Categories
(DevTools :: Console, enhancement, P3)
DevTools
Console
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jdescottes, Assigned: nchevobbe)
Details
Attachments
(1 file)
Currently, the node reps expect the list of actorIds that are attached to the DOM tree to know if a given node should be decorated with an "open in inspector" icon. Any node rep consumer has to build and provide this list which can be quite complicated to build. Alternatively, we could add a "isInTree" boolean to the node grip preview. The node grip is build in server/actors/object.js : http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/devtools/server/actors/object.js#1645-1692 The implementation could be along the following lines > // Is element in tree. > preview.isInTree = false; > let doc = rawObj.ownerDocument; > if (doc && doc.defaultView) { > let bindingParent = getRootBindingParent(rawObj); > if (doc.documentElement.contains(bindingParent)) { > preview.isInTree = true; > } > }
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Assignee: jdescottes → nchevobbe
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8854732 [details] Bug 1347490 - Return isConnected in node grip to indicate if a node is in the DOM tree. https://reviewboard.mozilla.org/r/126692/#review129346 Looks good overall but we can simplify this patch. ::: devtools/server/actors/object.js:1656 (Diff revision 2) > > let preview = grip.preview = { > kind: "DOMNode", > nodeType: rawObj.nodeType, > nodeName: rawObj.nodeName, > + isInTree: false, I imagine you may replace all that with: https://bugzilla.mozilla.org/show_bug.cgi?id=1261002 isInTree: rawObj.isConnected I'm also wondering if that would be better to stick with this name and name the preview attribute isConnected as well? ::: devtools/server/tests/mochitest/test_webconsole-node-grip.html:37 (Diff revision 2) > + info("Testing isInTree property on a ElementNode not in the DOM tree"); > + let {result} = await evaluateJS(consoleClient, "document.createElement(\"div\")"); > + ok(Object.keys(result.preview).includes("isInTree") === true, > + "There is a isInTree property in the preview object, as expected"); > + ok(result.preview.isInTree === false, > + "isInTree is false since we only created the element"); (Here and in next functions) There is no need to do two checks, this will be enough: is(result.preview.isInTree, false, "..."); ::: devtools/server/tests/mochitest/webconsole-helpers.js:31 (Diff revision 2) > +var gAttachCleanups = []; > +SimpleTest.registerCleanupFunction(function () { > + for (let cleanup of gAttachCleanups) { > + cleanup(); > + } > +}); It will be easier to call SimpleTest.registerCleanupFunction everywhere you are using gAttachCleanups instead of doing that. ::: devtools/server/tests/mochitest/webconsole-helpers.js:52 (Diff revision 2) > + let win = window.open(url, "_blank"); > + let client = null; > + > + let cleanup = () => { > + if (client) { > + client.close(); You may want to yield on client.close to ensure the client is completely destroyed before going to the next test. (you will have to make cleanup be a generator function) ::: devtools/server/tests/mochitest/webconsole-helpers.js:93 (Diff revision 2) > + return new Promise((resolve, reject) => { > + consoleClient.evaluateJS(text, packet => { > + resolve(packet); > + }); > + }); > +} This is useless now. consoleClient.evaluateJS should now return a promise. So you should be able to use it diretly in your test.
Attachment #8854732 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8854732 [details] Bug 1347490 - Return isConnected in node grip to indicate if a node is in the DOM tree. https://reviewboard.mozilla.org/r/126692/#review129346 > I imagine you may replace all that with: > https://bugzilla.mozilla.org/show_bug.cgi?id=1261002 > isInTree: rawObj.isConnected > > I'm also wondering if that would be better to stick with this name and name the preview attribute isConnected as well? yes, it seems better to keep the same name has in spec > You may want to yield on client.close to ensure the client is completely destroyed before going to the next test. > (you will have to make cleanup be a generator function) okay > This is useless now. consoleClient.evaluateJS should now return a promise. > So you should be able to use it diretly in your test. nice
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8854732 [details] Bug 1347490 - Return isConnected in node grip to indicate if a node is in the DOM tree. https://reviewboard.mozilla.org/r/126692/#review129474 ::: devtools/server/actors/object.js:13 (Diff revision 3) > > const { Cu, Ci } = require("chrome"); > const { GeneratedLocation } = require("devtools/server/actors/common"); > const { DebuggerServer } = require("devtools/server/main"); > const DevToolsUtils = require("devtools/shared/DevToolsUtils"); > +const { getRootBindingParent } = require("devtools/shared/layout/utils"); You can now get rid of that import.
Attachment #8854732 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8854732 [details] Bug 1347490 - Return isConnected in node grip to indicate if a node is in the DOM tree. https://reviewboard.mozilla.org/r/126692/#review129474 > You can now get rid of that import. good catch, thanks !
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
TRY seems good https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dad42f7c42433efa5c8dd63b0f2a20fcb3e8e96&selectedJob=88886754 , let's push that
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8854732 [details] Bug 1347490 - Return isConnected in node grip to indicate if a node is in the DOM tree. https://reviewboard.mozilla.org/r/126692/#review129636
Attachment #8854732 -
Flags: review+
Comment 11•7 years ago
|
||
Pushed by chevobbe.nicolas@gmail.com: https://hg.mozilla.org/integration/autoland/rev/44d2f2d786a8 Return isConnected in node grip to indicate if a node is in the DOM tree. r=ochameau
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44d2f2d786a8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•