[Webkit-unassigned] [Bug 103513] Provide the backend for exposing the layer tree to the Web Inspector
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 28 10:15:00 PST 2012
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.
More information about the webkit-unassigned
mailing list