Bug 103513 – Provide the backend for exposing the layer tree to the Web Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Antoine Quint]

It would be very useful for developers to find out information about the layer tree, especially layers that are composited in hardware and have a high impact on both the rendering performance of the page as well as its memory footprint. This bug covers the necessary WebCore parts to enable such a feature.

[Antoine Quint]

<rdar://problem/12766602>

[Antoine Quint]

Created attachment 176477 [details]
Patch

[Dean Jackson]

Comment on attachment 176477 [details]
Patch

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

Looks good! I guess Simon or Tim or others might want to take a look.

> Source/WebCore/ChangeLog:18
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Remove this line.

> Source/WebCore/ChangeLog:20
> +        No new tests (OOPS!).

Is there any way to write a LayoutTest for this?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:57
> +#include "InspectorLayerTreeAgent.h"

The implementation of InspectorLayerTreeAgent is protected by USE(ACCELERATED_COMPOSITING). You probably need the same guard here then, and below (or guard in the .h too).

> Source/WebCore/inspector/InspectorInstrumentation.cpp:917
> +        if (InspectorLayerTreeAgent* layerTreeAgent = instrumentingAgents->inspectorLayerTreeAgent())
> +            layerTreeAgent->reset();

Ditto.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:53
> +namespace LayerTreeAgentState {
> +static const char layerTreeAgentEnabled[] = "layerTreeAgentEnabled";
> +};

Is it typical to have a namespace for this strings?

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:66
> +InspectorLayerTreeAgent::~InspectorLayerTreeAgent()
> +{
> +    m_instrumentingAgents->setInspectorLayerTreeAgent(0);

Use nullptr instead of 0 here.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:72
> +void InspectorLayerTreeAgent::setFrontend(InspectorFrontend* frontend)
> +{
> +    ASSERT(frontend);
> +    m_frontend = frontend->layertree();

Nit: frontEnd

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:78
> +    m_frontend = 0;
> +    m_instrumentingAgents->setInspectorLayerTreeAgent(0);

Use nullptr instead of 0 here.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:91
> +    m_lastLayerId = 0;

Is 0 a valid id? Maybe you want a constant for invalidId?

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:134
> +PassRefPtr<TypeBuilder::LayerTree::Layer> InspectorLayerTreeAgent::buildObjectForRootLayer()
> +{
> +    return buildObjectForLayer(m_inspectedPage->mainFrame()->contentRenderer()->compositor()->rootRenderLayer(), true);

Is anything along this chain ever null? I guess there always has to be a root render layer.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:149
> +    RefPtr<TypeBuilder::LayerTree::Layer> layerObject = TypeBuilder::LayerTree::Layer::create()
> +        .setId(bind(renderLayer))
> +        .setIsRoot(renderLayer->isRootLayer())
> +        .setParentId(renderLayer->parent() ? bind(renderLayer->parent()) : 0)
> +        .setNodeId(node ? m_instrumentingAgents->inspectorDOMAgent()->idForNode(node) : 0)
> +        .setBounds(buildObjectForIntRect(enclosingIntRect(renderLayer->localBoundingBox())))
> +        .setIsComposited(isComposited);

I know this is pretty common in inspector code, but I'm not a huge fan of chaining like this. It looks too much like JavaScript/JQuery code :)

But more importantly, it's a pain to debug because you have to step in and out lots of times (I should check if lldb is smart about line numbers there).

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:201
> +    return TypeBuilder::LayerTree::IntRect::create()
> +        .setX(rect.x())
> +        .setY(rect.y())
> +        .setWidth(rect.width())
> +        .setHeight(rect.height()).release();

Ditto on the chaining.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:209
> +    int id = m_documentLayerToIdMap.get(layer);
> +    if (id)
> +        return id;
> +    id = ++m_lastLayerId;

So this tells me that 0 is not a valid id, which I think answers the question above.

> Source/WebCore/inspector/InspectorLayerTreeAgent.h:34
> +#include "FrameView.h"

Do you need this?

> Source/WebCore/inspector/InspectorLayerTreeAgent.h:39
> +#include "ScriptState.h"

Or this?

> Source/WebCore/inspector/InstrumentingAgents.h:165
> +    InspectorLayerTreeAgent* m_inspectorLayerTreeAgent;

Maybe ditto the comment above about USE(ACCELERATED_COMPOSITING)

> Source/WebCore/rendering/RenderLayerCompositor.h:32
> +#include "InspectorLayerTreeAgent.h"

I think you can just do class InspectorLayerTreeAgent here.

[Dean Jackson]

Do the efl, qt and gtk ports all have ACCELERATED_COMPOSITING enabled? If not, then I guess my comments are wrong since they passed EWS. You could try building with it off.

[Simon Fraser (smfr)]

Comment on attachment 176477 [details]
Patch

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

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

Should be const.

> Source/WebCore/inspector/InspectorLayerTreeAgent.h:85
> +    PassRefPtr<TypeBuilder::LayerTree::Layer> buildObjectForLayer(RenderLayer*, bool);

Should be const RenderLayer*

> Source/WebCore/inspector/InspectorLayerTreeAgent.h:86
> +    PassRefPtr<TypeBuilder::LayerTree::IntRect> buildObjectForIntRect(IntRect);

const IntRect&

[Levi Weintraub]

Comment on attachment 176477 [details]
Patch

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

>> Source/WebCore/inspector/InspectorLayerTreeAgent.h:85
>> +    PassRefPtr<TypeBuilder::LayerTree::Layer> buildObjectForLayer(RenderLayer*, bool);
> 
> Should be const RenderLayer*

Boolean params :(  I'd rather a more descriptive parameter, or leave it out altogether since it's always passed as true in the code here.

[Antoine Quint]

(In reply to comment #6)
> (From update of attachment 176477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176477&action=review
> 
> >> Source/WebCore/inspector/InspectorLayerTreeAgent.h:85
> >> +    PassRefPtr<TypeBuilder::LayerTree::Layer> buildObjectForLayer(RenderLayer*, bool);
> > 
> > Should be const RenderLayer*
> 
> Boolean params :(  I'd rather a more descriptive parameter, or leave it out altogether since it's always passed as true in the code here.

I think it should be left out. A previous iteration of this patch allowed to get details without recursing through children but there's no use for this now.

[Timothy Hatcher]

Comment on attachment 176477 [details]
Patch

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

>> Source/WebCore/ChangeLog:20
>> +        No new tests (OOPS!).
> 
> Is there any way to write a LayoutTest for this?

Yes, you should write a protocol test. See LayoutTests/inspector-protocol.

[Antoine Quint]

(In reply to comment #8)
> (From update of attachment 176477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176477&action=review
> 
> >> Source/WebCore/ChangeLog:20
> >> +        No new tests (OOPS!).
> > 
> > Is there any way to write a LayoutTest for this?
> 
> Yes, you should write a protocol test. See LayoutTests/inspector-protocol.

Will do. Thanks for pointing that out.

[Antoine Quint]

> > Source/WebCore/ChangeLog:20
> > +        No new tests (OOPS!).
> 
> Is there any way to write a LayoutTest for this?

Per Timothy's comment, we can and I will look into this.
 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:57
> > +#include "InspectorLayerTreeAgent.h"
> 
> The implementation of InspectorLayerTreeAgent is protected by USE(ACCELERATED_COMPOSITING). You probably need the same guard here then, and below (or guard in the .h too).

I was wondering about this, but I noticed that RenderLayerCompositor, which has its implementation guarded by the same condition, has not such condition for its header. I simply copied that example, do you know why it is done this way with RenderLayerCompositor and why this should differ?
 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:917
> > +        if (InspectorLayerTreeAgent* layerTreeAgent = instrumentingAgents->inspectorLayerTreeAgent())
> > +            layerTreeAgent->reset();
> 
> Ditto.

Indeed, will change.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:53
> > +namespace LayerTreeAgentState {
> > +static const char layerTreeAgentEnabled[] = "layerTreeAgentEnabled";
> > +};
> 
> Is it typical to have a namespace for this strings?

I just mimicked how other agents do this type of thing, and the ones I looked at were using namespaces.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:66
> > +InspectorLayerTreeAgent::~InspectorLayerTreeAgent()
> > +{
> > +    m_instrumentingAgents->setInspectorLayerTreeAgent(0);
> 
> Use nullptr instead of 0 here.

Will do.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:72
> > +void InspectorLayerTreeAgent::setFrontend(InspectorFrontend* frontend)
> > +{
> > +    ASSERT(frontend);
> > +    m_frontend = frontend->layertree();
> 
> Nit: frontEnd

All other code in WebCore uses frontend / setFrontend.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:78
> > +    m_frontend = 0;
> > +    m_instrumentingAgents->setInspectorLayerTreeAgent(0);
> 
> Use nullptr instead of 0 here.

Will do.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:91
> > +    m_lastLayerId = 0;
> 
> Is 0 a valid id? Maybe you want a constant for invalidId?

You got the answer to that question further down.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:134
> > +PassRefPtr<TypeBuilder::LayerTree::Layer> InspectorLayerTreeAgent::buildObjectForRootLayer()
> > +{
> > +    return buildObjectForLayer(m_inspectedPage->mainFrame()->contentRenderer()->compositor()->rootRenderLayer(), true);
> 
> Is anything along this chain ever null? I guess there always has to be a root render layer.

In my testing, I didn't find a situation that something in this chain could be null. I don't know if that's absolutely true though.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:149
> > +    RefPtr<TypeBuilder::LayerTree::Layer> layerObject = TypeBuilder::LayerTree::Layer::create()
> > +        .setId(bind(renderLayer))
> > +        .setIsRoot(renderLayer->isRootLayer())
> > +        .setParentId(renderLayer->parent() ? bind(renderLayer->parent()) : 0)
> > +        .setNodeId(node ? m_instrumentingAgents->inspectorDOMAgent()->idForNode(node) : 0)
> > +        .setBounds(buildObjectForIntRect(enclosingIntRect(renderLayer->localBoundingBox())))
> > +        .setIsComposited(isComposited);
> 
> I know this is pretty common in inspector code, but I'm not a huge fan of chaining like this. It looks too much like JavaScript/JQuery code :)
> 
> But more importantly, it's a pain to debug because you have to step in and out lots of times (I should check if lldb is smart about line numbers there).

I agree with both your points, but it appears that all non-optional properties have to be set up this way. I had tried to make a call to create() and then have a .set() call per property but this lead to build failures. Maybe someone more familiar with the TypeBuilder system could shed more light on this issue.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:201
> > +    return TypeBuilder::LayerTree::IntRect::create()
> > +        .setX(rect.x())
> > +        .setY(rect.y())
> > +        .setWidth(rect.width())
> > +        .setHeight(rect.height()).release();
> 
> Ditto on the chaining.
> 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:209
> > +    int id = m_documentLayerToIdMap.get(layer);
> > +    if (id)
> > +        return id;
> > +    id = ++m_lastLayerId;
> 
> So this tells me that 0 is not a valid id, which I think answers the question above.
> 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.h:34
> > +#include "FrameView.h"
> 
> Do you need this?
> 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.h:39
> > +#include "ScriptState.h"
> 
> Or this?

Leftovers from the original agent I used as a model. I will check and likely remove.
 
> > Source/WebCore/inspector/InstrumentingAgents.h:165
> > +    InspectorLayerTreeAgent* m_inspectorLayerTreeAgent;
> 
> Maybe ditto the comment above about USE(ACCELERATED_COMPOSITING)
> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:32
> > +#include "InspectorLayerTreeAgent.h"
> 
> I think you can just do class InspectorLayerTreeAgent here.

Will change.

Thanks for the review!

[Pavel Feldman]

Comment on attachment 176477 [details]
Patch

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.

[Andrey Kosyakov]

Comment on attachment 176477 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:1582
> +    inspector/InspectorLayerTreeAgent.cpp

please keep the entries sorted

> Source/WebCore/GNUmakefile.list.am:3699
> +	Source/WebCore/inspector/InspectorLayerTreeAgent.cpp \
> +	Source/WebCore/inspector/InspectorLayerTreeAgent.h \

ditto

> Source/WebCore/Target.pri:759
> +    inspector/InspectorLayerTreeAgent.cpp \

ditto

> Source/WebCore/Target.pri:1901
> +    inspector/InspectorLayerTreeAgent.h \

ditto

> Source/WebCore/WebCore.gypi:2852
> +            'inspector/InspectorLayerTreeAgent.cpp',
> +            'inspector/InspectorLayerTreeAgent.h',

ditto

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

Is it necessary at all? It appears that parentId could be used to check for root instead. Actually, it looks like with current implementation you don't need either isRoot or parent, as these may be easily deduced from the tree that you return.

>> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:53
>> +};
> 
> Is it typical to have a namespace for this strings?

yes, at least for the rest of inspector.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:137
> +PassRefPtr<TypeBuilder::LayerTree::Layer> InspectorLayerTreeAgent::buildObjectForLayer(RenderLayer* renderLayer, bool deep)

is deep ever false?

>> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:149
>> +        .setIsComposited(isComposited);
> 
> I know this is pretty common in inspector code, but I'm not a huge fan of chaining like this. It looks too much like JavaScript/JQuery code :)
> 
> But more importantly, it's a pain to debug because you have to step in and out lots of times (I should check if lldb is smart about line numbers there).

It's the only practical way to do it -- the intermediate types that each setFoo() returns are not expected to appear explicitly in the client code (it would be an ugly template with an integer parameter set to a bitmask of not-yet-set fields -- we use this to assure at compile time that all fields are set before we send the object)

> Source/WebCore/inspector/InstrumentingAgents.h:43
> +class InspectorLayerTreeAgent;

sorting problem

[Pavel Feldman]

Btw, is there a mock for the front-end part?

[Timothy Hatcher]

We are not implementing the front-end side in WebKit. This is a backend only change to support a Safari Inspector feature.

[Pavel Feldman]

Hm. We typically (In reply to comment #14)
> We are not implementing the front-end side in WebKit. This is a backend only change to support a Safari Inspector feature.

I think we need to somehow agree on the policy wrt exposing inspector APIs for proprietary clients. Like this new agent clearly adds maintenance cost for WebKit without clear benefit to the port owners. Until this moment, every feature exposed via the protocol had a client living within the WebKit repository. I am not saying that we should have such a strict rule (I can clearly see automation and other commands added to the remote debugging protocol with no UI exposed in the inspector). But in this case, it sounds like exposing layer information would be useful for the Web Inspector users and I don't see why one of the ports would invest into the backend, but not the front-end for it. Should this be raised on webkit-dev?

[Timothy Hatcher]

(In reply to comment #15)
> Hm. We typically (In reply to comment #14)
> > We are not implementing the front-end side in WebKit. This is a backend only change to support a Safari Inspector feature.
> 
> I think we need to somehow agree on the policy 0 exposing inspector APIs for proprietary clients. Like this new agent clearly adds maintenance cost for WebKit without clear benefit to the port owners. Until this moment, every feature exposed via the protocol had a client living within the WebKit repository. I am not saying that we should have such a strict rule (I can clearly see automation and other commands added to the remote debugging protocol with no UI exposed in the inspector). But in this case, it sounds like exposing layer information would be useful for the Web Inspector users and I don't see why one of the ports would invest into the backend, but not the front-end for it. Should this be raised on webkit-dev?

In my opinion, this is not too different from the V8 centric features that have been added to the protocol. Those are changes that benefit only one port — Chromium — and add maintenance cost to the project as a whole. Your port does maintain those parts, but the burden is mostly on other ports to make them work for JSC.

Someone is free to implement a front-end using this new agent in WebKit, but we don't intend to do it. Antoine is implementing a front-end for our Inspector as he goes. We also intended to maintain it, just like you maintain the V8 centric features.

Testing at the protocol level and maintenance guarantees should be sufficient to grant this agent the privilege of being added.

Feel free to raise the issue on webkit-dev if you like.

[Pavel Feldman]

> In my opinion, this is not too different from the V8 centric features that have been added to the protocol. Those are changes that benefit only one port — Chromium — and add maintenance cost to the project as a whole. Your port does maintain those parts, but the burden is mostly on other ports to make them work for JSC.

I don't think this analogy applies. In case of V8 vs JSC:
- Both front-end and backend were open source, belonging to WebKit
- We were on the best-effort path wrt having JSC supported
- Most of the features are now supported in JSC

In this case, we are talking about:
- Proprietary front-end putting maintenance cost on the WebKit
- No intention to contribute front-end to WebKit
- No clear plan for these features to be ever supported in WebKit inspector

[Timothy Hatcher]

(In reply to comment #17)
> > In my opinion, this is not too different from the V8 centric features that have been added to the protocol. Those are changes that benefit only one port — Chromium — and add maintenance cost to the project as a whole. Your port does maintain those parts, but the burden is mostly on other ports to make them work for JSC.
> 
> I don't think this analogy applies. In case of V8 vs JSC:
> - Both front-end and backend were open source, belonging to WebKit
> - We were on the best-effort path wrt having JSC supported
> - Most of the features are now supported in JSC
> 
> In this case, we are talking about:
> - Proprietary front-end putting maintenance cost on the WebKit
> - No intention to contribute front-end to WebKit
> - No clear plan for these features to be ever supported in WebKit inspector

V8 is not in the WebKit repository, nor does it belong to WebKit. But I digress.

This is about adding API. Most API added to WebKit (CSS, DOM, etc.) does not have a client in WebKit using them. The Inspector protocol has been the exception to this, not the rule. I don't think everything added to the Inspector protocol API should require a client in WebKit as long as it is tested and maintained by interested parties.

[Antoine Quint]

(In reply to comment #5)
> (From update of attachment 176477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176477&action=review
> 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.h:69
> > +    bool enabled() { return m_enabled; }
> 
> Should be const.

I'm actually removing this method altogether, it's not used and serves no purpose.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.h:85
> > +    PassRefPtr<TypeBuilder::LayerTree::Layer> buildObjectForLayer(RenderLayer*, bool);
> 
> Should be const RenderLayer*

This proves problematic as there is a call to updateLayerListsIfNeeded() make to the RenderLayer in that method.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.h:86
> > +    PassRefPtr<TypeBuilder::LayerTree::IntRect> buildObjectForIntRect(IntRect);
> 
> const IntRect&

Will change.

[Antoine Quint]

(In reply to comment #12)
> (From update of attachment 176477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176477&action=review
> 
> > Source/WebCore/CMakeLists.txt:1582
> > +    inspector/InspectorLayerTreeAgent.cpp
> 
> please keep the entries sorted

I'll be updating this file and all the others you mentioned to preserve sorting.
 
> >> 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.
> 
> Is it necessary at all? It appears that parentId could be used to check for root instead. Actually, it looks like with current implementation you don't need either isRoot or parent, as these may be easily deduced from the tree that you return.

You're correct, I'm removing both .isRoot and .parentId.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:137
> > +PassRefPtr<TypeBuilder::LayerTree::Layer> InspectorLayerTreeAgent::buildObjectForLayer(RenderLayer* renderLayer, bool deep)
> 
> is deep ever false?

No, I'm removing this parameter.
 
Thanks for the useful feedback.

[Antoine Quint]

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

[Pavel Feldman]

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

Given that you maintain ids for your layers, adding protocol methods for resolving layer to a node and vice versa would make sense to me.

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

I guess you could start as is and add greater granularity as you go. An option would be to send "layerTreeUpdated" invalidation signal with no parameters and make client re-request the tree on demand. So that you did not load the protocol while say the tree is collapsed or offscreen.

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

Yep. You should ack like the other agents.

[Antoine Quint]

(In reply to comment #22)
> > 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.
> 
> Given that you maintain ids for your layers, adding protocol methods for resolving layer to a node and vice versa would make sense to me.

I'm adding a new "nodeIdForLayerId" method on the agent. There will likely be a need for the reverse, but I plan on addressing this in a future patch when it becomes a necessity.
 
> > 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?
> 
> I guess you could start as is and add greater granularity as you go. An option would be to send "layerTreeUpdated" invalidation signal with no parameters and make client re-request the tree on demand. So that you did not load the protocol while say the tree is collapsed or offscreen.

That sounds like a good plan.
 
> > 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.
> 
> Yep. You should ack like the other agents.

I got this working along with fixes for all your other comments, I plan on sending a revised patch for review later today.

[Antoine Quint]

Created attachment 176819 [details]
Patch

[Antoine Quint]

(In reply to comment #24)
> Created an attachment (id=176819) [details]
> Patch

This new patch should address all the comments made so far, save for one: no LayoutTests are added. I plan on doing this in a final patch tomorrow if this patch gets a positive review.

[Early Warning System Bot]

Comment on attachment 176819 [details]
Patch

Attachment 176819 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15036655

[Early Warning System Bot]

Comment on attachment 176819 [details]
Patch

Attachment 176819 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15059024

[EFL EWS Bot]

Comment on attachment 176819 [details]
Patch

Attachment 176819 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15055119

[Pavel Feldman]

Comment on attachment 176819 [details]
Patch

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.

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

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

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

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:119
> +    UNUSED_PARAM(error);

Replace ErrorString* error with ErrorString* above and remove this macro.

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

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

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 176977 [details]
Patch

[Antoine Quint]

(In reply to comment #31)
> Created an attachment (id=176977) [details]
> Patch

This new patch should address Pavel's latest comments and fix reported build failures on platforms other than Mac. Note that LayoutTests are still pending, I plan on doing this in a final patch once the basic WebCore patch gets a positive review.

[Pavel Feldman]

Comment on attachment 176977 [details]
Patch

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

Overall, it looks good, I think it is Ok to proceed with the test.

> Source/WebCore/inspector/Inspector.json:3264
> +                "hidden": true

nit: here and below, no need to hide methods of the hidden domain.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:218
> +    const RenderLayer* renderLayer = m_idToLayer.get(layerId);

this can be 0 and you should return an error for this.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:514
> +    InspectorInstrumentation::layerTreeDidChange(this->page());

just page() ?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:520
> +    InspectorInstrumentation::renderLayerDestroyed(this->page(), renderLayer);

ditto

[Andrey Kosyakov]

Comment on attachment 176977 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:514
>> +    InspectorInstrumentation::layerTreeDidChange(this->page());
> 
> just page() ?

So the layer tree managed by InspectorLayerTreeAgent has regular render layers, yet you only update it when in the accelerated compositing mode -- this looks a bit confusing. Isn't there use in the layer tree if the accelerated compositing is not used? Should we make it explicit that the tree is only available in accelerated compositing mode?

[Antoine Quint]

(In reply to comment #33)
> (From update of attachment 176977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176977&action=review
> 
> Overall, it looks good, I think it is Ok to proceed with the test.
> 
> > Source/WebCore/inspector/Inspector.json:3264
> > +                "hidden": true
> 
> nit: here and below, no need to hide methods of the hidden domain.

You're right, I should have cleaned that up when I made the entire domain hidden. Will make that change.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:218
> > +    const RenderLayer* renderLayer = m_idToLayer.get(layerId);
> 
> this can be 0 and you should return an error for this.

Right, I'll make the following change:

    // Send an error if there is no such registered layer id.
    if (!renderLayer) {
        *errorString = "Could not find a bound layer for the provided id";
        return;
    }

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:514
> > +    InspectorInstrumentation::layerTreeDidChange(this->page());
> 
> just page() ?
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:520
> > +    InspectorInstrumentation::renderLayerDestroyed(this->page(), renderLayer);
> 
> ditto

I was keeping in style with several this->page() in this file. I'm not sure what's best here.

[Antoine Quint]

(In reply to comment #34)
> (From update of attachment 176977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176977&action=review
> 
> >> Source/WebCore/rendering/RenderLayerCompositor.cpp:514
> >> +    InspectorInstrumentation::layerTreeDidChange(this->page());
> > 
> > just page() ?
> 
> So the layer tree managed by InspectorLayerTreeAgent has regular render layers, yet you only update it when in the accelerated compositing mode -- this looks a bit confusing. Isn't there use in the layer tree if the accelerated compositing is not used? Should we make it explicit that the tree is only available in accelerated compositing mode?

The entire agent is guarded with #if USE(ACCELERATED_COMPOSITING).

[Andrey Kosyakov]

(In reply to comment #36)
> (In reply to comment #34)
> > (From update of attachment 176977 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=176977&action=review
> > 
> > >> Source/WebCore/rendering/RenderLayerCompositor.cpp:514
> > >> +    InspectorInstrumentation::layerTreeDidChange(this->page());
> > > 
> > > just page() ?
> > 
> > So the layer tree managed by InspectorLayerTreeAgent has regular render layers, yet you only update it when in the accelerated compositing mode -- this looks a bit confusing. Isn't there use in the layer tree if the accelerated compositing is not used? Should we make it explicit that the tree is only available in accelerated compositing mode?
> 
> The entire agent is guarded with #if USE(ACCELERATED_COMPOSITING).

That was another thing that I was curious about -- does it have to be? My original question, though, was rather about the effect of run-time compositing mode, not the compile-time ability to do accelerated compositing.

[Antoine Quint]

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #34)
> > > (From update of attachment 176977 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=176977&action=review
> > > 
> > > >> Source/WebCore/rendering/RenderLayerCompositor.cpp:514
> > > >> +    InspectorInstrumentation::layerTreeDidChange(this->page());
> > > > 
> > > > just page() ?
> > > 
> > > So the layer tree managed by InspectorLayerTreeAgent has regular render layers, yet you only update it when in the accelerated compositing mode -- this looks a bit confusing. Isn't there use in the layer tree if the accelerated compositing is not used? Should we make it explicit that the tree is only available in accelerated compositing mode?
> > 
> > The entire agent is guarded with #if USE(ACCELERATED_COMPOSITING).
> 
> That was another thing that I was curious about -- does it have to be? My original question, though, was rather about the effect of run-time compositing mode, not the compile-time ability to do accelerated compositing.

If we go back to the bug's description, the real intent of this agent is provide information about layers composited in hardware. What we're trying to address in the Safari Web Inspector are issues related to the increased memory footprint caused by layers and the ability to easily see what gets composited in a hardware-backed RenderLayer. As such, we only want to show this feature when ACCELERATED_COMPOSITING is on. We can revisit this later as needed, but I think in the context of this bug we should keep the flags as they are.

[Antoine Quint]

Created attachment 177107 [details]
Patch

[Antoine Quint]

(In reply to comment #39)
> Created an attachment (id=177107) [details]
> Patch

This patch makes the small changes Pavel suggested in comment #33, which is to remove the unnecessary "hidden" properties in Inspector.json since the entire LayerTree domain is now hidden, as well as simply use `page()` in lieu of `this->page()` and finally to report an error in the case the RenderLayer isn't found in InspectorLayerTreeAgent::nodeIdForLayerId.

Finally, a test was added which exercises the agent's methods and event.

[WebKit Review Bot]

Comment on attachment 177107 [details]
Patch

Attachment 177107 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15103172

New failing tests:
inspector-protocol/layer-tree.html

[Antoine Quint]

Created attachment 177147 [details]
Patch

[Andrey Kosyakov]

> If we go back to the bug's description, the real intent of this agent is provide information about layers composited in hardware. What we're trying to address in the Safari Web Inspector are issues related to the increased memory footprint caused by layers and the ability to easily see what gets composited in a hardware-backed RenderLayer. As such, we only want to show this feature when ACCELERATED_COMPOSITING is on. We can revisit this later as needed, but I think in the context of this bug we should keep the flags as they are.

I realize this works as intended in your particular usage scenario. This may, however, appear confusing for the rest of the protocol clients. I think a more consistent approach would be either to expose render layers unconditionally on whether compositing is used, or just expose composited layers only.

[Andrey Kosyakov]

Comment on attachment 177147 [details]
Patch

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

> LayoutTests/inspector-protocol/layer-tree-expected.txt:322
> +          "memory": 4582080,

Do you expect memory to remain stable?

> LayoutTests/inspector-protocol/layer-tree-expected.txt:332
> +          "layerId": "0.11",

I wouldn't expect ids to remain stable.

> LayoutTests/inspector-protocol/layer-tree.html:8
> +    var element = document.createElement('div');

please use double quotes where possible.

> LayoutTests/inspector-protocol/layer-tree.html:16
> +    // Variables used throughout the test.

Here and below, please remove redundant/self-evident comments -- there's a convention in WebKit to avoid such.

> LayoutTests/inspector-protocol/layer-tree.html:94
> +        //

please remove

> LayoutTests/inspector-protocol/layer-tree.html:120
> +        if (!attributes.hasOwnProperty("id") || attributes.id != "last-element")

style: we use !==
hasOwnProperty looks redundant

> LayoutTests/inspector-protocol/layer-tree.html:144
> +        //

please remove

> LayoutTests/inspector-protocol/layer-tree.html:147
> +                return (oldKeys.indexOf(key) == -1);

style: we normally use ===

> LayoutTests/inspector-protocol/layer-tree.html:150
> +                return (newKeys.indexOf(key) == -1);

ditto

> LayoutTests/inspector-protocol/layer-tree.html:163
> +        })(layerTree);

please extract call into a separate statement

> LayoutTests/inspector-protocol/layer-tree.html:180
> +    function dumpObject(obj)
> +    {
> +        InspectorTest.log("\n" + JSON.stringify(obj, null, '  '));
> +    };

I think you can move this over to InspectorTest. Perhaps, we would eventually need to unify/merge it with the front-end test suite.

> LayoutTests/inspector-protocol/layer-tree.html:186
> +            if (checkFailure(test.name, messageObject))

I think it would be better readable if the logic of checkFailure() were inlined here.

> LayoutTests/inspector-protocol/layer-tree.html:222
> +  div {
> +    position: absolute;
> +    top: 0;
> +    left: 0;
> +  }

Here and below -- please fix indents to be a multiple of 4.

> LayoutTests/platform/chromium/TestExpectations:4107
> +# Layer tree inspection test only passes on Mac
> +webkit.org/b/103513 inspector-protocol/layer-tree.html [ Skip ]

Please file a bug for other platforms to support this and refer to this bug instead. BTW, could you please elaborate on why this is Mac-only?

[Antoine Quint]

(In reply to comment #43)
> > If we go back to the bug's description, the real intent of this agent is provide information about layers composited in hardware. What we're trying to address in the Safari Web Inspector are issues related to the increased memory footprint caused by layers and the ability to easily see what gets composited in a hardware-backed RenderLayer. As such, we only want to show this feature when ACCELERATED_COMPOSITING is on. We can revisit this later as needed, but I think in the context of this bug we should keep the flags as they are.
> 
> I realize this works as intended in your particular usage scenario. This may, however, appear confusing for the rest of the protocol clients. I think a more consistent approach would be either to expose render layers unconditionally on whether compositing is used, or just expose composited layers only.

Further work may indeed be done to enable all clients to expose the RenderLayer tree even when ACCELERATED_COMPOSITING. I do think this patch however has evolved in a way that it's useful as-is, technically sound and with a protocol that allows extension. I don't plan on making substantial changes to WebCore as part of this bug.

I will however update the test per your suggestions.

[Antoine Quint]

Created attachment 177472 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 177472 [details]
Patch

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

> LayoutTests/inspector-protocol/layer-tree.html:146
> +        function recurse (layer) {

recurse(layer)

> LayoutTests/inspector-protocol/layer-tree.html:168
> +        function replacer (key, value)

style: no space after function name

> LayoutTests/inspector-protocol/layer-tree.html:170
> +            return (key === "layerId") ? undefined : value;

style: please drop redundant parenthesis
we typically dump typeof value instead of value if value is non-stable -- this occasionally helps to catch regressions for values that are missing or of a wrong type.

[Timothy Hatcher]

Comment on attachment 177472 [details]
Patch

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

Post a version with some of these tweaks, especially the dumping typeof instead of dropping the replaced properties, and I'll r+ it.

> LayoutTests/inspector-protocol/layer-tree.html:92
> +            parameters: { "layerId": lastId },

No spaces inside {}.

> LayoutTests/inspector-protocol/layer-tree.html:103
> +            parameters: { "nodeId": id },

No spaces inside {}.

>> LayoutTests/inspector-protocol/layer-tree.html:146
>> +        function recurse (layer) {
> 
> recurse(layer)

The { should also be on a new line.

>> LayoutTests/inspector-protocol/layer-tree.html:170
>> +            return (key === "layerId") ? undefined : value;
> 
> style: please drop redundant parenthesis
> we typically dump typeof value instead of value if value is non-stable -- this occasionally helps to catch regressions for values that are missing or of a wrong type.

Should memory be replaced too? I don't think it will stay constant and might lead to flakiness. Unless it is always constant based on the width and height.

[Andrey Kosyakov]

Comment on attachment 177472 [details]
Patch

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

> LayoutTests/inspector-protocol/layer-tree-expected.txt:333
> +=== Obtain the node id for the layer with id 0.11 ===

remove the id?

> LayoutTests/inspector-protocol/layer-tree-expected.txt:337
> +=== Get attributes for node with id 9 ===

ditto

[Antoine Quint]

Created attachment 177565 [details]
Patch

[Antoine Quint]

(In reply to comment #47)
> (From update of attachment 177472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177472&action=review
> 
> > LayoutTests/inspector-protocol/layer-tree.html:146
> > +        function recurse (layer) {
> 
> recurse(layer)

Fixed in 7th patch.

> > LayoutTests/inspector-protocol/layer-tree.html:168
> > +        function replacer (key, value)
> 
> style: no space after function name

Fixed in 7th patch.

> > LayoutTests/inspector-protocol/layer-tree.html:170
> > +            return (key === "layerId") ? undefined : value;
> 
> style: please drop redundant parenthesis
> we typically dump typeof value instead of value if value is non-stable -- this occasionally helps to catch regressions for values that are missing or of a wrong type.

Good point. I fixed that in the 7th patch.

[Antoine Quint]

(In reply to comment #48)
> (From update of attachment 177472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177472&action=review
> 
> > LayoutTests/inspector-protocol/layer-tree.html:92
> > +            parameters: { "layerId": lastId },
> 
> No spaces inside {}.

Fixed in 7th patch.

> > LayoutTests/inspector-protocol/layer-tree.html:103
> > +            parameters: { "nodeId": id },
> 
> No spaces inside {}.

Fixed in 7th patch.

> >> LayoutTests/inspector-protocol/layer-tree.html:146
> >> +        function recurse (layer) {
> > 
> > recurse(layer)
> 
> The { should also be on a new line.

Fixed in 7th patch.

> >> LayoutTests/inspector-protocol/layer-tree.html:170
> >> +            return (key === "layerId") ? undefined : value;
> > 
> > style: please drop redundant parenthesis
> > we typically dump typeof value instead of value if value is non-stable -- this occasionally helps to catch regressions for values that are missing or of a wrong type.
> 
> Should memory be replaced too? I don't think it will stay constant and might lead to flakiness. Unless it is always constant based on the width and height.

Memory should be based purely on width and height and not flaky. I've preserved it in the updated patch.

[Antoine Quint]

(In reply to comment #49)
> (From update of attachment 177472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177472&action=review
> 
> > LayoutTests/inspector-protocol/layer-tree-expected.txt:333
> > +=== Obtain the node id for the layer with id 0.11 ===
> 
> remove the id?
> 
> > LayoutTests/inspector-protocol/layer-tree-expected.txt:337
> > +=== Get attributes for node with id 9 ===
> 
> ditto

Fixed in 7th patch, thanks for catching this.

[Simon Fraser (smfr)]

Memory for tiled layers can change dynamically.

[Pavel Feldman]

Comment on attachment 177565 [details]
Patch

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

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:67
> +    m_instrumentingAgents->setInspectorLayerTreeAgent(0);

There is no need to do this here.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:83
> +    enable(0);

This is wrong: enable will check for the state and will return early.

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:96
> +    m_enabled = true;

I thought we agreed that you did not need to mirror layerTreeAgentEnabled

[Antoine Quint]

(In reply to comment #54)
> Memory for tiled layers can change dynamically.

I'll update the test to not report memory usage.

[Antoine Quint]

(In reply to comment #55)
> (From update of attachment 177565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177565&action=review
> 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:67
> > +    m_instrumentingAgents->setInspectorLayerTreeAgent(0);
> 
> There is no need to do this here.

Will fix in updated patch.

> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:83
> > +    enable(0);
> 
> This is wrong: enable will check for the state and will return early.

I don't think this method is actually needed, will remove in new patch.
 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:96
> > +    m_enabled = true;
> 
> I thought we agreed that you did not need to mirror layerTreeAgentEnabled

We did, sloppy code removal on my part. I'll make sure m_enabled is fully gone in the updated patch.

[Antoine Quint]

Created attachment 177708 [details]
Patch

[WebKit Review Bot]

Comment on attachment 177708 [details]
Patch

Attachment 177708 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15160187

New failing tests:
inspector-protocol/layer-tree.html

[Pavel Feldman]

> I don't think this method is actually needed, will remove in new patch.

It actually is needed for the case when the process gets swapped. What I meant was that if the state has LayerTreeAgentState::layerTreeAgentEnabled set to true, you should in fact perform the enable routines. I'd suggest either a) calling enable from restore and removing the early return from enable or b) copying enable content into restore in case the bit is set. Sorry for not making it clear.

Rest of the patch looks good.

[Antoine Quint]

(In reply to comment #60)
> > I don't think this method is actually needed, will remove in new patch.
> 
> It actually is needed for the case when the process gets swapped. What I meant was that if the state has LayerTreeAgentState::layerTreeAgentEnabled set to true, you should in fact perform the enable routines. I'd suggest either a) calling enable from restore and removing the early return from enable or b) copying enable content into restore in case the bit is set. Sorry for not making it clear.
> 
> Rest of the patch looks good.

Thanks for the explanation, I will make the required changes later today and send a new patch out.

I'm a little worried about the test failure reported by the cr-linux bot when the same bot reported no such issue with the previous patch which had a fairly similar test, and cr-android being OK in both cases. Any idea what might be going wrong there? I couldn't see the text diff being reported by the bot, or I don't know where to look to find it.

[Antoine Quint]

Created attachment 177824 [details]
Patch

[WebKit Review Bot]

Comment on attachment 177824 [details]
Patch

Attachment 177824 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15170284

New failing tests:
inspector-protocol/layer-tree.html

[WebKit Review Bot]

Comment on attachment 177824 [details]
Patch

Attachment 177824 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15173137

New failing tests:
inspector-protocol/layer-tree.html

[Pavel Feldman]

Created attachment 178199 [details]
[DIFF] chromium actual diff

[Pavel Feldman]

Comment on attachment 177824 [details]
Patch

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

> LayoutTests/inspector-protocol/layer-tree-expected.txt:153
> +                        "width": 237,

Chromium fails with these numbers. What defines these values btw? Can we use something more predictable?

> LayoutTests/platform/efl/TestExpectations:1684
> +Bug(EFL) webkit.org/b/103513 inspector-protocol/layer-tree.html [ Skip ]

EFL sheriff should do this and file a bug for himself. Same goes for other ports.

[Antoine Quint]

(In reply to comment #66)
> (From update of attachment 177824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177824&action=review
> 
> > LayoutTests/inspector-protocol/layer-tree-expected.txt:153
> > +                        "width": 237,
> 
> Chromium fails with these numbers. What defines these values btw? Can we use something more predictable?

Yes, we can do that. I'll send an updated patch with more predictable output. 

> 
> > LayoutTests/platform/efl/TestExpectations:1684
> > +Bug(EFL) webkit.org/b/103513 inspector-protocol/layer-tree.html [ Skip ]
> 
> EFL sheriff should do this and file a bug for himself. Same goes for other ports.

I'm not familiar with the sheriff. Should I just not skip anything then and some bot will catch it and not prevent a commit via the commit-queue?

[Pavel Feldman]

> Yes, we can do that. I'll send an updated patch with more predictable output. 
> 
> > > +Bug(EFL) webkit.org/b/103513 inspector-protocol/layer-tree.html [ Skip ]
> 
> I'm not familiar with the sheriff. Should I just not skip anything then and some bot will catch it and not prevent a commit via the commit-queue?

You should either land by hand and let sheriffs handle the port-specific expectations or file a new bug claiming test does not work on EFL. You should add it into the "# EFL has accelerated compositing disabled" group in the TestExpectations with that fresh bug.

[Antoine Quint]

Created attachment 178217 [details]
Patch

[Pavel Feldman]

Comment on attachment 178217 [details]
Patch

Looks great, thanks for following up!

[Antoine Quint]

Created attachment 178219 [details]
Patch

[Antoine Quint]

(In reply to comment #70)
> (From update of attachment 178217 [details])
> Looks great, thanks for following up!

It was almost right, I hadn't moved the bug in the right place in the EFL TestExpectations, there's a newer patch doing this. I haven't actually ran the test, made a blind change. Hopefully it's right!

[WebKit Review Bot]

Comment on attachment 178219 [details]
Patch

Clearing flags on attachment: 178219

Committed r136959: <http://trac.webkit.org/changeset/136959>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It broke the !inspector platforms, see the Qt minimal bot for the details

[Brent Fulgham]

This patch broke the WinCairo build as well.

[Brent Fulgham]

I landed a patch under 136995 that should correct the build for people where USE(ACCELERATED_COMPOSITING) is false.

[Pavel Feldman]

Comment on attachment 178219 [details]
Patch

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

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:81
> +    enable(0);

This should have been 
if (m_state->getBoolean(LayerTreeAgentState::layerTreeAgentEnabled))
    enable(0);

Could you please follow up?

[Antoine Quint]

(In reply to comment #78)
> (From update of attachment 178219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178219&action=review
> 
> > Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:81
> > +    enable(0);
> 
> This should have been 
> if (m_state->getBoolean(LayerTreeAgentState::layerTreeAgentEnabled))
>     enable(0);
> 
> Could you please follow up?

Sure. One question though, since enable is only the following code right now:

void InspectorLayerTreeAgent::enable(ErrorString*)
{
    m_state->setBoolean(LayerTreeAgentState::layerTreeAgentEnabled, true);
    m_instrumentingAgents->setInspectorLayerTreeAgent(this);
}

Should we maybe just call this in restore()?

m_instrumentingAgents->setInspectorLayerTreeAgent(this);

[Pavel Feldman]

That would work. But I prefer registering agent in one place in the code + your enable routine might grow. A better pattern is typically to have innerEnable that is called from both. And enable could fast return in that case.

[Antoine Quint]

(In reply to comment #80)
> That would work. But I prefer registering agent in one place in the code + your enable routine might grow. A better pattern is typically to have innerEnable that is called from both. And enable could fast return in that case.

OK, I'll make the suggest changed now and submit a new patch.

[Antoine Quint]

(In reply to comment #81)
> (In reply to comment #80)
> > That would work. But I prefer registering agent in one place in the code + your enable routine might grow. A better pattern is typically to have innerEnable that is called from both. And enable could fast return in that case.
> 
> OK, I'll make the suggest changed now and submit a new patch.

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

[Roger Fong]

inspector-protocol/layer-tree.html fails on Windows.

In theory ACCELERATED_COMPOSITING is enabled on Windows. The test seems to think otherwise.
Any idea what's gong on?

Results are here:
http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r136961%20(30671)/inspector-protocol/layer-tree-pretty-diff.html