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)

enhancement

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;
>    }
>  }
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee: jdescottes → nchevobbe
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)
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 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+
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/44d2f2d786a8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: