[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 #176477|review?                     |review-
               Flag|                            |




--- Comment #11 from Pavel Feldman <pfeldman at chromium.org>  2012-11-28 10:46:41 PST ---
(From update of attachment 176477)
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.

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

> Source/WebCore/inspector/Inspector.json:3246
> +        "domain": "LayerTree",

You should hide entire domain

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

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

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

type LayerId

> 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

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

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

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

You will not need these checks once you limit the instrumenting lifespan of the agent.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:115
> +    m_frontend->layerTreeWillChange();

Why is this signal needed?

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

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

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

>> Source/WebCore/inspector/InspectorLayerTreeAgent.h:69
>> +    bool enabled() { return m_enabled; }
> 
> Should be const.

Why is it public at all?

> 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

> Source/WebCore/rendering/RenderLayerCompositor.h:390
> +    InspectorLayerTreeAgent* layerTreeInspector();

WebCore is not allowed to talk to agents, it should be done via InspectorInstrumentation.

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