[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





--- Comment #10 from Antoine Quint <graouts at apple.com>  2012-11-28 10:17:13 PST ---
> > 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!

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