[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





--- Comment #30 from Antoine Quint <graouts at apple.com>  2012-11-30 08:47:43 PST ---
Thanks for the comments Pavel. A new patch will be out momentarily.

(In reply to comment #29)
> (From update of attachment 176819 [details])
> 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.

Agreed. The event should be just a notification and the front-end should call .getLayerTree() if interested in the result. I'll remove the parameter altogether.

> > 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 :)

I will remove the ASSERT altogether.

> > 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.

Good point, will change that.

> > 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.

Will change.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:119
> > +    UNUSED_PARAM(error);
> 
> Replace ErrorString* error with ErrorString* above and remove this macro.

Will change.

> > 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.

I will make a change such that when the RenderLayerCompositor removes a layer, the agent gets a notification and unbinds as a result.

> > 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.

I will add a new method to the DOM agent to push the node of a given RenderLayer instead, it'll be a lot easier.

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