[webkit-reviews] review denied: [Bug 103513] Provide the backend for exposing the layer tree to the Web Inspector : [Attachment 176819] 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 103513: Provide the backend for exposing the layer tree to the Web
Inspector
https://bugs.webkit.org/show_bug.cgi?id=103513

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

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


> Source/WebCore/inspector/Inspector.json:3314
> +		       { "name": "layerTree", "$ref": "Layer" }

I think this should either be empty of contain layerId of the changed layer
tree root.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:72
> +    ASSERT(frontend);

Agents know it is non-NULL. I'd push this check into the call site, but
frontend instance is created there :)

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:84
> +    m_enabled =
m_state->getBoolean(LayerTreeAgentState::layerTreeAgentEnabled);

You should call enable here so that
m_instrumentingAgents->setInspectorLayerTreeAgent(this); is executed.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:95
> +    if (m_enabled)

We only mirror m_state->setBoolean(LayerTreeAgentState::layerTreeAgentEnabled)
using m_enabled in the hot code. I.e. in core agents that are instrumenting at
all times and need to return fast. In your case, using m_state should suffice.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:119
> +    UNUSED_PARAM(error);

Replace ErrorString* error with ErrorString* above and remove this macro.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:202
> +void InspectorLayerTreeAgent::unbind(const RenderLayer* layer)

Given that you store raw pointers to the render layer in the map and don't call
this method, upon calling nodeIdForLayerId you are at rist of running into use
after free. You should unbind upon layer destruction.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:220
> +    int documentNodeId =
inspectorDOMAgent->boundNodeId(node->ownerDocument());

This will not work for the iframes - you will get 0 here.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:221
> +    *resultNodeId = inspectorDOMAgent->pushNodeToFrontend(errorString,
documentNodeId, node);

You should not use protocol methods when interacting between agents. In this
case you need InspectorDOMAgent::pushNodePathToFrontend which is private. You
should add public method that pushes node for the RenderLayer* into the
InspectorDOMAgent instead. We don't want to make pushNodePathToFrontend(Node*)
public to make sure that it is not misused (i.e. not called unless the user
explicitly asked for document from the DOM domain). Using RenderLayer* in the
API might stop such a misuse.