[webkit-reviews] review denied: [Bug 103513] Provide the backend for exposing the layer tree to the Web Inspector : [Attachment 176477] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 28 10:44:26 PST 2012


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 176477: Patch
https://bugs.webkit.org/attachment.cgi?id=176477&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.


More information about the webkit-reviews mailing list