In order to support https://bugs.webkit.org/show_bug.cgi?id=110407, the front-end will need to receive some information related to a node before that node is pushed, with a simple "backend node identifier" used to identify the node. The front-end should then be able to request the node with the given backend node id to be pushed to the front-end and subsequently, a node pushed to the front-end should list both its backend node id and its regular node id, as it will be necessary for the LayerTreeAgent to be able to identify a node for the given backend node id has now been pushed.
<rdar://problem/13300008>
Created attachment 190386 [details] Patch
Comment on attachment 190386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190386&action=review > Source/WebCore/inspector/Inspector.json:1732 > + { "name": "backendNodeId", "$ref": "BackendNodeId", "optional": true, "description": "Node identifier used to identify this node prior to being pushing to the front-end. This identifier expires as soon as the node is pushed and should only be used to update id references for nodes that have been referenced, prior to pushing, by a backend node identifier." } Since you get a hold of Node object, it means that this id is useless. Why sending it here? Callers should manage them and tear them down in the callbacks on their own. > Source/WebCore/inspector/InspectorDOMAgent.h:248 > + HashMap<RefPtr<Node>, BackendNodeId> m_nodeToBackendIdMap; You don't want to retain nodes by backend ids - eventually you blow the heap. There even is no way to dispose them. You should keep the raw pointer and clean it up upon node deletion.
(In reply to comment #3) > (From update of attachment 190386 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190386&action=review > > > Source/WebCore/inspector/Inspector.json:1732 > > + { "name": "backendNodeId", "$ref": "BackendNodeId", "optional": true, "description": "Node identifier used to identify this node prior to being pushing to the front-end. This identifier expires as soon as the node is pushed and should only be used to update id references for nodes that have been referenced, prior to pushing, by a backend node identifier." } > > Since you get a hold of Node object, it means that this id is useless. Why sending it here? Callers should manage them and tear them down in the callbacks on their own. I'm not sure how that can be done in all cases. Imagine you call pushNodeByBackendIdToFrontend which could be made to return the nodeId, what if an ancestor of that node has a backendId as well and get pushed in that call? How will the front-end be able to establish that node with backendNodeId -3 is the same as node with nodeId 22 that was pushed but not explicitly requested? > > > Source/WebCore/inspector/InspectorDOMAgent.h:248 > > + HashMap<RefPtr<Node>, BackendNodeId> m_nodeToBackendIdMap; > > You don't want to retain nodes by backend ids - eventually you blow the heap. There even is no way to dispose them. You should keep the raw pointer and clean it up upon node deletion. Will fix, thanks for catching this.
> I'm not sure how that can be done in all cases. Imagine you call pushNodeByBackendIdToFrontend which could be made to return the nodeId, what if an ancestor of that node has a backendId as well and get pushed in that call? How will the front-end be able to establish that node with backendNodeId -3 is the same as node with nodeId 22 that was pushed but not explicitly requested? Lets discard backendId upon explicit pushNodeByBackendIdToFrontend then? I am sure that most of the call sites would like to not think about whether node with given backend id was already sent to the front-end. They would not mind making a roundtrip for the conversion and would operate backend ids only.
(In reply to comment #5) > > I'm not sure how that can be done in all cases. Imagine you call pushNodeByBackendIdToFrontend which could be made to return the nodeId, what if an ancestor of that node has a backendId as well and get pushed in that call? How will the front-end be able to establish that node with backendNodeId -3 is the same as node with nodeId 22 that was pushed but not explicitly requested? > > Lets discard backendId upon explicit pushNodeByBackendIdToFrontend then? I am sure that most of the call sites would like to not think about whether node with given backend id was already sent to the front-end. They would not mind making a roundtrip for the conversion and would operate backend ids only. So we wouldn't have a backendNodeId member on the Node object sent to the front-end and instead calling pushNodeByBackendIdToFrontend on a node that has already been pushed would just return the nodeId and then discard the backendNodeId. Am I reading this right? This sounds good to me.
Created attachment 190757 [details] Patch
Comment on attachment 190757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190757&action=review I don't see the code to clean up m_backendIdToNode when Node is removed. Something in InspectorDOMAgent::unbind is likely needed. > Source/WebCore/inspector/InspectorDOMAgent.cpp:1746 > + if (Node * node = m_backendIdToNode.get(backendNodeId)) { Style: Node* node = ...
Comment on attachment 190757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190757&action=review >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1746 >> + if (Node * node = m_backendIdToNode.get(backendNodeId)) { > > Style: Node* node = ... What if the node was deleted? You should clean up your binding upon node deletion.
Created attachment 190769 [details] Patch
(In reply to comment #9) > (From update of attachment 190757 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190757&action=review > > >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1746 > >> + if (Node * node = m_backendIdToNode.get(backendNodeId)) { > > > > Style: Node* node = ... > > What if the node was deleted? You should clean up your binding upon node deletion. I think this is addressed in the newer patch where we check for a backendNodeId for the node passed to unbind().
Comment on attachment 190769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190769&action=review > Source/WebCore/inspector/InspectorDOMAgent.cpp:324 > + if (m_nodeToBackendIdMap.contains(node)) { This method will only call itself recursively in case of mapped nodes / childrenRequested, etc. At the same time, your node might be in the removed subtree. There is a couple of solutions I see depending on what operation you want to be fast: 1) in case backend map is not empty, traverse the entire subtree being removed here 2) when backendId is being created, generate backendids for the entire path from node to root. Then know for sure whether you need to traverse nodes here.
This is no longer relevant, https://bugs.webkit.org/show_bug.cgi?id=110407 will be addressed differently.
Reopening to attach new patch.
Created attachment 195552 [details] Patch
This will be needed for https://bugs.webkit.org/show_bug.cgi?id=113398. Patch rebaselined, and comment addressed. Please, take a look. (In reply to comment #12) > (From update of attachment 190769 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190769&action=review > > > Source/WebCore/inspector/InspectorDOMAgent.cpp:324 > > + if (m_nodeToBackendIdMap.contains(node)) { > > This method will only call itself recursively in case of mapped nodes / childrenRequested, etc. At the same time, your node might be in the removed subtree. > > There is a couple of solutions I see depending on what operation you want to be fast: > 1) in case backend map is not empty, traverse the entire subtree being removed here > 2) when backendId is being created, generate backendids for the entire path from node to root. Then know for sure whether you need to traverse nodes here. Used option 2 here. I do generate backendId for all parents of the node, and so I'm guaranteed that recursive unbind will process all nodes with backendId.
Created attachment 195555 [details] Patch
Comment on attachment 195555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195555&action=review We need to test this. > Source/WebCore/inspector/Inspector.json:1722 > + "description": "Unique DOM node identifier used to reference a node that may not have been pushed to the front-end." "hidden": true > Source/WebCore/inspector/InspectorDOMAgent.cpp:326 > + m_backendIdToNode.remove(backendId); Thinking about the future... If we want to re-use ids after say timeline was stopped, we might want these nodes retained. I.e. we might want to have a named object group that will be discarded either on demand or when all its backend ids are resolved to node ids. Just thinking out loud. > Source/WebCore/inspector/InspectorDOMAgent.cpp:353 > + if (nodesMap->contains(node)) You don't need to change this. > Source/WebCore/inspector/InspectorDOMAgent.cpp:630 > + current = innerParentNode(current); You don't need to retain parent path. > Source/WebCore/inspector/InspectorDOMAgent.cpp:1762 > + *nodeId = pushNodePathToFrontend(node); You should discard backend id at this point (clean up the map)
Comment on attachment 195555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195555&action=review > Source/WebCore/inspector/InspectorDOMAgent.cpp:627 > + BackendNodeId id = m_lastBackendNodeId--; nit: it's either --m_lastBackendNodeId, or m_nextBackendNodeId--
Created attachment 195749 [details] Patch
Comment on attachment 195555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195555&action=review >> Source/WebCore/inspector/Inspector.json:1722 >> + "description": "Unique DOM node identifier used to reference a node that may not have been pushed to the front-end." > > "hidden": true Done. >> Source/WebCore/inspector/InspectorDOMAgent.cpp:627 >> + BackendNodeId id = m_lastBackendNodeId--; > > nit: it's either --m_lastBackendNodeId, or m_nextBackendNodeId-- Done.
Comment on attachment 195749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195749&action=review > Source/WebCore/inspector/InspectorDOMAgent.cpp:631 > +void InspectorDOMAgent::releaseBackendIdsForNodeGroup(const String& nodeGroup) This method should be exposed via the protocol. > Source/WebCore/inspector/InspectorDOMAgent.h:250 > + HashMap<String, OwnPtr<NodeToBackendIdMap> > m_nodeGroupToBackendIdMap; Lets store by value instead.
Created attachment 195968 [details] Patch
Comment on attachment 195749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195749&action=review >> Source/WebCore/inspector/InspectorDOMAgent.cpp:631 >> +void InspectorDOMAgent::releaseBackendIdsForNodeGroup(const String& nodeGroup) > > This method should be exposed via the protocol. Done. >> Source/WebCore/inspector/InspectorDOMAgent.h:250 >> + HashMap<String, OwnPtr<NodeToBackendIdMap> > m_nodeGroupToBackendIdMap; > > Lets store by value instead. Done.
Comment on attachment 195968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195968&action=review > Source/WebCore/inspector/Inspector.json:2022 > + "name": "releaseBackendIdsForNodeGroup", releaseBackendNodeIds > Source/WebCore/inspector/InspectorDOMAgent.cpp:616 > + m_nodeGroupToBackendIdMap.set(nodeGroup, NodeToBackendIdMap()); You don't need this, object will be created automatically upon access. > Source/WebCore/inspector/InspectorDOMAgent.cpp:619 > + BackendNodeId id = map.get(node); id = m_nodeGroupToBackendIdMap[nodeGroup].get(node); > Source/WebCore/inspector/InspectorDOMAgent.cpp:623 > + m_backendIdToNode.set(id, std::make_pair(node, nodeGroup)); m_nodeGroupToBackendIdMap[nodeGroup].set(id, ...); > Source/WebCore/inspector/InspectorDOMAgent.cpp:632 > + NodeToBackendIdMap& map = *m_nodeGroupToBackendIdMap.find(nodeGroup).values(); ditto
Comment on attachment 195968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195968&action=review > Source/WebCore/inspector/InspectorDOMAgent.cpp:618 > + NodeToBackendIdMap& map = *m_nodeGroupToBackendIdMap.find(nodeGroup).values(); "values()" -> "->value"
Comment on attachment 195968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195968&action=review >> Source/WebCore/inspector/Inspector.json:2022 >> + "name": "releaseBackendIdsForNodeGroup", > > releaseBackendNodeIds Done. >> Source/WebCore/inspector/InspectorDOMAgent.cpp:618 >> + NodeToBackendIdMap& map = *m_nodeGroupToBackendIdMap.find(nodeGroup).values(); > > "values()" -> "->value" Done.
Created attachment 196110 [details] Patch
Comment on attachment 196110 [details] Patch Clearing flags on attachment: 196110 Committed r147428: <http://trac.webkit.org/changeset/147428>
All reviewed patches have been landed. Closing bug.