Return isInTree in node grip preview to check if a Node is currently in the DOM tree

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Console
P3
enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jdescottes, Assigned: nchevobbe)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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

10 months ago
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
(Assignee)

Updated

10 months ago
Assignee: jdescottes → nchevobbe
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

10 months 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

10 months 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

10 months 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

10 months 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 10

10 months 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

10 months 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44d2f2d786a8
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.