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

Thu Nov 29 05:20:21 PST 2012

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

--- Comment #21 from Antoine Quint <graouts at apple.com>  2012-11-29 05:22:36 PST ---
(In reply to comment #11)
> (From update of attachment 176477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176477&action=review
> 
> Overall, looks like it is going the right way. Added a bunch of comments inline.

Thanks a lot for your feedback. I'm new to WebCore development, so I really appreciate this, and I have some more questions. Pardon me if some of this is obvious!

> > Source/WebCore/ChangeLog:3
> > +        Provide the backend for exposing the layer tree to the Web Inspector
> 
> Please use webkit.org/new-inspector-bug template for filing bugs so that active inspector contributors were getting into the cc.

Will do in the future.

> > Source/WebCore/inspector/Inspector.json:3246
> > +        "domain": "LayerTree",
> 
> You should hide entire domain

Will change.

> > Source/WebCore/inspector/Inspector.json:3265
> > +                    { "name": "id", "type": "integer", "description": "The unique id for this layer." },
> 
> Should be called layerId and be of type "LayerId" that is of type integer.

Good point, will change.

> > Source/WebCore/inspector/Inspector.json:3266
> > +                    { "name": "isRoot", "type": "boolean", "description": "Indicates whether this layer is the root." },
> 
> You might want to make it optional and imply that it is not root when omitted.

Per Andrey's feedback, I will remove this property as well as parentId.

> > Source/WebCore/inspector/Inspector.json:3267
> > +                    { "name": "parentId", "type": "integer", "description": "Id of the parent layer." },
> 
> type LayerId

Removing this property (see above).

> > Source/WebCore/inspector/Inspector.json:3268
> > +                    { "name": "nodeId", "type": "integer", "description": "Id of the node to which this layer is attached." },
> 
> type is $ref DOM.NodeId

Will change.

> > Source/WebCore/inspector/InspectorDOMAgent.h:202
> > +    int idForNode(Node*);
> 
> It is missing by design. Whoever converts node to id must realize that it is being pushed to the front-end, i.e. with the significant performance penalties.

OK. The intent is that the front-end should be able to get from a layer to its DOM node for inspection. I understand it's expensive to push the node for each and every node with a RenderLayer. Do you think it would be appropriate to add a new method on this agent to get the DOM id for a given RenderLayer? That way the front-end would request the id as needed only.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:61
> > +    m_instrumentingAgents->setInspectorLayerTreeAgent(this);
> 
> You should only add this agent into the instrumenting agents list from enable (and remove from disable). Only limited set of core agents are allowed to exist at all times.

Good point, will do.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:112
> > +    if (!m_enabled)
> 
> You will not need these checks once you limit the instrumenting lifespan of the agent.

Will remove.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:115
> > +    m_frontend->layerTreeWillChange();
> 
> Why is this signal needed?

It's really not, will remove.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:123
> > +    m_frontend->layerTreeDidChange(buildObjectForRootLayer());
> 
> Could this be done on a more granular basis? What is the average payload size here? We tend to send events to the interested parties only. I.e. only after the tree has been requested.

I honestly don't have enough data to provide you with a payload size. Right now, the agent is only enabled once the developer asks for the layer tree to be displayed (ie. once .enable() is called). I'm not entirely sure what the lifecycle of the agent is currently, but should we make it so that it's disabled once the relevant piece of UI for it is not visible thus limiting the amount of information sent through this agent?

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:147
> > +        .setNodeId(node ? m_instrumentingAgents->inspectorDOMAgent()->idForNode(node) : 0)
> 
> This might be expensive. How many layers do you expect? Pushing node to a front-end pushes it with the whole ancestors path.

There may be hundreds of layers. Like I said above, I think we should be able to provide the node id and push the nodes only on demand when the front-end asks for it (through the corresponding RenderLayer id). Sounds good?

> >> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:209
> >> +    id = ++m_lastLayerId;
> > 
> > So this tells me that 0 is not a valid id, which I think answers the question above.
> 
> Please use the (poorly named) IdentifiersFactory instead

I will look into this, thanks for the pointer.

> >> Source/WebCore/inspector/InspectorLayerTreeAgent.h:69
> >> +    bool enabled() { return m_enabled; }
> > 
> > Should be const.
> 
> Why is it public at all?

I'm removing this useless method.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:431
> > +    // Inform the inspector that we're starting a compositing update.
> 
> Should be InspectorInstrumentation::layerTreeWillChange.
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:519
> > +    if (layerTreeInspector())
> 
> ditto

Are you suggesting I add a new static method on InspectorInstrumentation to allow communication between WebCore and the agent? If so, I will do this, thanks for pointing it out.

> > Source/WebCore/rendering/RenderLayerCompositor.h:390
> > +    InspectorLayerTreeAgent* layerTreeInspector();
> 
> WebCore is not allowed to talk to agents, it should be done via InspectorInstrumentation.

Noted.

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