NodeListActor's items() response has unnecessary nesting

RESOLVED FIXED in Firefox 25

Status

DevTools
Inspector
RESOLVED FIXED
5 years ago
a month ago

People

(Reporter: harth, Assigned: dcamp)

Tracking

unspecified
Firefox 25

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Response from an items() request:

{"nodes":{"nodes":[{"actor":"conn3.domnode16","parent":"conn3.domnode19","nodeType":1,"nodeName":"DIV","numChildren":0,"attrs":[{"name":"class","value":"item"},{"name":"id","value":"test1"}]},{"actor":"conn3.domnode17","parent":"conn3.domnode19","nodeType":1,"nodeName":"DIV","numChildren":5,"attrs":[{"name":"class","value":"item"},{"name":"id","value":"test2"}]},{"actor":"conn3.domnode18","parent":"conn3.domnode19","nodeType":1,"nodeName":"DIV","numChildren":0,"attrs":[{"name":"class","value":"item"},{"name":"id","value":"test3"}]}],"newNodes":[{"actor":"conn3.domnode19","parent":"conn3.domnode20","nodeType":1,"nodeName":"SECTION","numChildren":7,"attrs":[{"name":"id","value":"test-section"}]},{"actor":"conn3.domnode20","parent":"conn3.domnode21","nodeType":1,"nodeName":"MAIN","numChildren":3,"attrs":[]},{"actor":"conn3.domnode21","parent":"conn3.domnode22","nodeType":1,"nodeName":"BODY","numChildren":3,"attrs":[]},{"actor":"conn3.domnode22","parent":"conn3.domnode13","nodeType":1,"nodeName":"HTML","numChildren":3,"attrs":[],"isDocumentElement":true}]},"from":"conn3.domnodelist15"}

Notice the nodes.nodes and nodes.newNodes. Seems like it could just be nodes and newNodes.

I wish we had a single field for the return value across all the actors, like "result"

What is newNodes for?
(Assignee)

Comment 1

5 years ago
Yeah, we should get rid of the useless extra nesting.

`nodes` is an array of nodes that answer the question being asked (like the querySelector nodes).

To make things sane for the client, we make sure that when you have a node reference on the client side, you also have a reference to all of its parents (so that parentNode can be sync).  newNodes fills in any parents that the client doesn't know about yet.
(Reporter)

Comment 2

5 years ago
(In reply to Dave Camp (:dcamp) from comment #1)
> To make things sane for the client, we make sure that when you have a node
> reference on the client side, you also have a reference to all of its
> parents (so that parentNode can be sync).  newNodes fills in any parents
> that the client doesn't know about yet.

Ah, maybe newNodes should be named something else?

I'm curious about this use case. Why you'd need it to be sync over other things like getting children that are async?
(Assignee)

Comment 3

5 years ago
(In reply to Heather Arthur [:harth] from comment #2)
> Ah, maybe newNodes should be named something else?

Yeah, any suggestions?

> I'm curious about this use case. Why you'd need it to be sync over other
> things like getting children that are async?

It's mostly related to the algorithm we use to release memory for disconnected nodes - when a node is disconnected from the dom tree we want to be able to free the client objects for all the children nodes.

So to be able to answer "all the children of a given node that we have seen on the client side", we guarantee that every time we've seen a node, we connect it up through its parents.

Clients don't need to implement the collection algorithm.  In that case they're free to ignore the newNodes (maybe newParents? unseenParents?).

I'll submit a patch that puts this documentation in the file somewhere.
(Assignee)

Comment 4

5 years ago
Created attachment 778866 [details] [diff] [review]
v1`
Attachment #778866 - Flags: review?(fayearthur)
(Reporter)

Comment 5

5 years ago
Comment on attachment 778866 [details] [diff] [review]
v1`

Review of attachment 778866 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I see.

Looks good, thanks!
Attachment #778866 - Flags: review?(fayearthur) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/05ff12c00114
Assignee: nobody → dcamp
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/05ff12c00114
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.