[webkit-reviews] review denied: [Bug 110407] Web Inspector: enhance the LayerTreeAgent protocol to report smarter information : [Attachment 190202] 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 110407: Web Inspector: enhance the LayerTreeAgent protocol to report
smarter information
https://bugs.webkit.org/show_bug.cgi?id=110407

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190202&action=review


Please split this change into DOMAgent refactoring and a Layer agent changes.

>> Source/WebCore/inspector/Inspector.json:1715
>> +		    "description": "Unique DOM node identifier used to
reference a node that has not yet been pushed to the front-end."
> 
> This isn't a guarantee that the node has not been pushed, it is just n
indication to the front-end that it may not have been pushed and may need to be
requested. It may already have been pushed.

This should be landed separately.

> Source/WebCore/inspector/Inspector.json:1992
> +		   "name": "pushNodeByIdToFrontend",

ByBackendIdToFrontend

> Source/WebCore/inspector/Inspector.json:3684
> +		       { "name": "nodeId", "$ref": "DOM.BackendNodeId",
"description": "The id for the node associated with this layer. Note that this
node id may not have been pushed yet." },

backendNodeId

> Source/WebCore/inspector/InspectorDOMAgent.cpp:315
> +    id = m_nodeToBackendIdMap.get(node);

We don't want backend ids for all nodes.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:320
> +	   m_idToNode.set(id, node);

You are risk of holding a pointer to the freed element which should never
happen.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:600
> +    int id = m_documentNodeToIdMap.get(node);

Please use typedef for BackendNodeId and never use the same value for node id
and backend id. I'd even keep backend node ids negative so that they were never
misused.