[Webkit-unassigned] [Bug 103513] Provide the backend for exposing the layer tree to the Web Inspector

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=103513


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #176819|review?                     |review-
               Flag|                            |




--- Comment #29 from Pavel Feldman <pfeldman at chromium.org>  2012-11-29 15:40:52 PST ---
(From update of attachment 176819)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.