[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 #3 from Dean Jackson <dino at apple.com>  2012-11-28 07:51:03 PST ---
(From update of attachment 176477)
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.

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