[webkit-reviews] review denied: [Bug 74203] [chromium] Create iterators for the RenderSurface-Layer tree : [Attachment 121123] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 74203: [chromium] Create iterators for the RenderSurface-Layer tree
https://bugs.webkit.org/show_bug.cgi?id=74203

Attachment 121123: Patch
https://bugs.webkit.org/attachment.cgi?id=121123&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121123&action=review


Looking close! I'm worried about the IteratorActionType memory, there's a "new"
here with no "delete" and no smart pointer type. Left some other comments as
well inline.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:37
> +// A RenderSurface $R$ may be referred to in one of two different contexts.
One RenderSurface is "current" at any time, for

i'm not sure what the $x$ naming convention means, but I find it makes this
paragraph hard to read - the $'s dominate the letter on the inside. What about
just "A RenderSurface R may be..." ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:62
> +// Create a stepping iterator and end iterator by calling
CCLayerIterator::begin() and CCLayerIterator::end() and passing in the
> +// list of layers owning target RenderSurfaces. Step through the tree by
incrementing the stepping iterator while it is != to
> +// the end iterator. At each step the iterator knows what the layer is
representing, and you can query the iterator to decide
> +// what actions to perform with the layer given what it represents.

some short example code would be really nice here showing how to pick an
iteration direction and how to actually get at the layers. maybe even put the
example above the detailed explanation

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:80
> +struct CCLayerIteratorPosition {

this guy takes a lot of lines of code and puts the start of the actual
CCLayerIterator interface down below line 100. can it get its own header?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:102
> +    static const int kInvalidTargetRenderSurfaceLayer = -1;
> +    // This must be -1 since the iterator action code assumes that this
value can be
> +    // reached by subtracting one from the position of the first layer in
the current
> +    // target surface's child layer list, which is 0.
> +    static const unsigned kLayerRepresentingTargetRenderSurface =
(unsigned)-1;

we don't normally use the "k" prefix for constants in WebKit

what do you mean by (unsigned)-1 ? unsigned types + negative numbers normally
means weird bugs. why the c-style cast?

do we really need a different set of these constants for every instantiation of
this template, or can these be hoisted out?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:106
> +    const Vector<RefPtr<LayerType> >* m_renderSurfaceLayerList;
> +    int m_targetRenderSurfaceLayer;
> +    unsigned m_currentLayer;

we don't use the m_ prefix (or any prefix) for public members of a struct

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:137
> +	   , m_actions(new IteratorActionType())

who owns the memory for m_actions? where's the delete?

> Source/WebKit/chromium/tests/CCLayerIteratorTest.cpp:235
> +    root2->setOpacity(0.5f); // Force the layer to own a new surface.

here and elsewhere: in webkit, we don't use the "f" suffix in cases like this
where the compiler can figure out what we mean. just "0.5"