[webkit-reviews] review denied: [Bug 110921] Web Inspector: allow referencing of nodes that have not been pushed to the front-end : [Attachment 190386] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 110921: Web Inspector: allow referencing of nodes that have not been pushed
to the front-end
https://bugs.webkit.org/show_bug.cgi?id=110921

Attachment 190386: Patch
https://bugs.webkit.org/attachment.cgi?id=190386&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.