Bug 110407 – Web Inspector: enhance the LayerTreeAgent protocol to report smarter information

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Antoine Quint]

In practice, we're only interested in showing layers displayed for nodes in a particular subtree. However, the LayerTreeAgent exposes a single method to get layers through .getLayerTree() which returns the entire document's layer tree. This can be a lot of data depending on the complexity of the document, and a lot of this data may turn out to be completely useless to the front-end. This bug covers replacing the .getLayerTree() method with a new .layersForNode() method which operates only on the scope of a particular Element. The functionality of .getLayerTree() should still be available by calling .layersForNode() with the root element or Document node as parameter.

Additionally, we should also make the change such that we expose the node id of the layer's associated node, even when that node hasn't been pushed to the front-end yet. This will allow to capture the cases where layers are trashed and recreated for the same node, allowing the front-end not to mark these changes as an update rather than a completely new layer, since the relevant object is the node rather than the layer.

Finally, it may not be necessary to return a tree structure but rather just a flat list of layers should suffice.

[Radar WebKit Bug Importer]

<rdar://problem/13259186>

[Antoine Quint]

Retitling bug to take into account further changes to accommodate other requirements from the UI.

First, we should expose a way to narrow down the type of RenderLayers we're interested in. Making the backend return all RenderLayers, including those aren't composited, will return a very large amount of data. The UI should be able to instruct the backend that we're only interested in composited layers, which will be more rare and a lot more interesting to inspect for authors.

Second, we should provide more information about layers that aren't directly associated to an element in the DOM but may be generated via CSS (::before and ::after) or due to the presentation of a reflection.

Finally, since we've exposed it via https://bugs.webkit.org/show_bug.cgi?id=110505, we should also return the list of reasons why a RenderLayer was composited.

[Antoine Quint]

Created attachment 190202 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 190202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190202&action=review

I know others will want to take a look at this (those that looked at the first layer patches). I'm sure this needs a test for "reasons".

> Source/WebCore/ChangeLog:29
> +        push a node by its id when the front-end needs to gether information about a

Typo: "gather" => "gather"

> Source/WebCore/inspector/Inspector.json:1715
> +                "description": "Unique DOM node identifier used to reference a node that has not yet been pushed to the front-end."

This isn't a guarantee that the node has not been pushed, it is just n indication to the front-end that it may not have been pushed and may need to be requested. It may already have been pushed.

> Source/WebCore/inspector/Inspector.json:3691
> +                    { "name": "isReflection", "type": "boolean", "optional": true, "description": "Indicates whether this layer was used to provide a reflection for the element." },

This value, more so then the others, feels very implementation specific. Is it likely the case that reflections will always be treated as special? Once this is in the protocol, it will be hard to remove it.

Maybe this parameters can move to a set of more abstract values. For instance a "specialization" enum that could be "reflection" or "new-reason". With an "additionalInfo" object property containing extra information about the reflection / new-reason. Similar to "TimelineEvent" which has a type and an opaque "data" object that has different properties depending on the type. Each time a new timeline event is added the protocol does not need to change to support it.

> Source/WebCore/inspector/Inspector.json:3721
> +                    { "name": "layerId", "$ref": "LayerId", "description": "The id of the layer for which we want to get the reason it was composited." }

Nit: In the descriptions you say "reason" but everywhere else you say "reasons". I'd stick with reasons everywhere.

> Source/WebCore/inspector/Inspector.json:3725
> +                    { "name": "reasons", "type": "integer", "description": "The reason why the layer was composited." }

I think "reasonsMask" might be clearer that this is a bit mask. Just having name:"reasons" and type:"integer" sounds like this is the # of reasons, not a mask of a different unique reasons.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:168
> +    if (isGenerated)
> +        node = renderer->generatingNode();
> +    else if (renderLayer->isReflection())
> +        node = renderer->parent()->node();

Can there be a reflection for generated content? In that case, I assume these both match.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:212
> +void InspectorLayerTreeAgent::reasonsForCompositingLayer(ErrorString* errorString, const String& layerId, int* reasons)

The compositing reasons part of this patch could have been a separate patch. As it is, I don't see any tests for reasons.

[Pavel Feldman]

Comment on attachment 190202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190202&action=review

Please split this change into DOMAgent refactoring and a Layer agent changes.

>> Source/WebCore/inspector/Inspector.json:1715
>> +                "description": "Unique DOM node identifier used to reference a node that has not yet been pushed to the front-end."
> 
> This isn't a guarantee that the node has not been pushed, it is just n indication to the front-end that it may not have been pushed and may need to be requested. It may already have been pushed.

This should be landed separately.

> Source/WebCore/inspector/Inspector.json:1992
> +                "name": "pushNodeByIdToFrontend",

ByBackendIdToFrontend

> Source/WebCore/inspector/Inspector.json:3684
> +                    { "name": "nodeId", "$ref": "DOM.BackendNodeId", "description": "The id for the node associated with this layer. Note that this node id may not have been pushed yet." },

backendNodeId

> Source/WebCore/inspector/InspectorDOMAgent.cpp:315
> +    id = m_nodeToBackendIdMap.get(node);

We don't want backend ids for all nodes.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:320
> +        m_idToNode.set(id, node);

You are risk of holding a pointer to the freed element which should never happen.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:600
> +    int id = m_documentNodeToIdMap.get(node);

Please use typedef for BackendNodeId and never use the same value for node id and backend id. I'd even keep backend node ids negative so that they were never misused.

[Antoine Quint]

(In reply to comment #4)
> (From update of attachment 190202 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190202&action=review
> 
> > Source/WebCore/inspector/Inspector.json:3691
> > +                    { "name": "isReflection", "type": "boolean", "optional": true, "description": "Indicates whether this layer was used to provide a reflection for the element." },
> 
> This value, more so then the others, feels very implementation specific. Is it likely the case that reflections will always be treated as special? Once this is in the protocol, it will be hard to remove it.
> 
> Maybe this parameters can move to a set of more abstract values. For instance a "specialization" enum that could be "reflection" or "new-reason". With an "additionalInfo" object property containing extra information about the reflection / new-reason. Similar to "TimelineEvent" which has a type and an opaque "data" object that has different properties depending on the type. Each time a new timeline event is added the protocol does not need to change to support it.

This is a good idea.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:168
> > +    if (isGenerated)
> > +        node = renderer->generatingNode();
> > +    else if (renderLayer->isReflection())
> > +        node = renderer->parent()->node();
> 
> Can there be a reflection for generated content? In that case, I assume these both match.

As I understand it, a renderer's generatingNode can only be an element for which a ::before or ::after pseudo-class has been applied, or at the very least, a real Node object. However, when a RenderLayer is a reflection, it's not associated with a Node directly, it only lives in the RenderLayer hierarchy to provide a reflection. So I don't believe these two cases can be true.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:212
> > +void InspectorLayerTreeAgent::reasonsForCompositingLayer(ErrorString* errorString, const String& layerId, int* reasons)
> 
> The compositing reasons part of this patch could have been a separate patch. As it is, I don't see any tests for reasons.

You're right, I will split it off and provide additional testing coverage.

[Antoine Quint]

(In reply to comment #5)

> >> Source/WebCore/inspector/Inspector.json:1715
> >> +                "description": "Unique DOM node identifier used to reference a node that has not yet been pushed to the front-end."
> > 
> > This isn't a guarantee that the node has not been pushed, it is just n indication to the front-end that it may not have been pushed and may need to be requested. It may already have been pushed.
> 
> This should be landed separately.

OK. I originally didn't want to do that since there were API changes in that file that require changes to the LayerTreeAgent, but I will make a patch that just introduces pushNodeByBackendIdToFrontend and the new BackendId type without further changes.

> > Source/WebCore/inspector/InspectorDOMAgent.cpp:315
> > +    id = m_nodeToBackendIdMap.get(node);
> 
> We don't want backend ids for all nodes.

Any node could have had a backendId generated for it through the LayerTreeAgent at one point or another.

> > Source/WebCore/inspector/InspectorDOMAgent.cpp:600
> > +    int id = m_documentNodeToIdMap.get(node);
> 
> Please use typedef for BackendNodeId and never use the same value for node id and backend id. I'd even keep backend node ids negative so that they were never misused.

The reason I want to reuse the same IDs was for the front-end to be able to reference to nodes via a backendNodeId or a nodeId transparently. The layer tree front-end needs to be able to tell a layer is associated with a given node whether it's been pushed or not. So either the backendNodeId needs to be preserved as the nodeId upon pushing the node, or we'll need to have both backendNodeId and nodeId listed on the node when it's pushed so that the front-end can establish that a node that was previously unpushed and referred to with some backendNodeId is the same as the one that was pushed with a nodeId.

[Antoine Quint]

I have split the supporting InspectorDOMAgent changes off into a different bug: https://bugs.webkit.org/show_bug.cgi?id=110921.

[Antoine Quint]

Will send patches to get to the functionality discussed in this bug in 5 steps. The first step is to provide a clean slate to build a new set of APIs: https://bugs.webkit.org/show_bug.cgi?id=111251.

[Antoine Quint]

Second step is https://bugs.webkit.org/show_bug.cgi?id=111312.

[Antoine Quint]

Third step is https://bugs.webkit.org/show_bug.cgi?id=111419.

[Antoine Quint]

Fourth step is https://bugs.webkit.org/show_bug.cgi?id=111551.

[Antoine Quint]

Fifth and final step is https://bugs.webkit.org/show_bug.cgi?id=111703.

[Antoine Quint]

All subtasks are complete, closing.