[webkit-reviews] review denied: [Bug 73350] [chromium] Allow scrolling non-root layers in the compositor thread : [Attachment 122229] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Sami Kyostila
<skyostil at google.com>'s request for review:
Bug 73350: [chromium] Allow scrolling non-root layers in the compositor thread
https://bugs.webkit.org/show_bug.cgi?id=73350

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

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


Overall I think this is nearly ready to go. I think we need to wait for
https://bugs.webkit.org/show_bug.cgi?id=76663 to land to clean up the delegate
stuff, and I'm hoping that the layer iterators can clean up some of the
iteration logic. Otherwise we should be good to go.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:63
> +    virtual void scrollBy(const IntSize& scrollDelta) = 0;

I'd really prefer that you not grow the interface required for every
LayerChromium.	In https://bugs.webkit.org/show_bug.cgi?id=76663 i'm getting
rid of CCLayerDelegate and moving the paintContents() down to
ContentLayerChromium. I think you can do the same thing with scrollBy(), the
only layers that we actually support scrolling are
GraphicsLayerChromium::m_layer's, right? That way you won't have to provide
empty implementations in so many classes (like WebLayerImpl) where scrollBy()
makes no sense

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:141
> +    static void collectLayersInPaintOrder(CCLayerList&, CCRenderSurface*);
> +    static void collectNonDrawingScrollableLayers(CCLayerList&,
CCLayerImpl*);
> +    static void collectAllLayers(CCLayerList&, CCLayerImpl*);

now that layer iterators have landed (http://trac.webkit.org/changeset/104626)
can we use those instead of adding a set of new utility functions?

if we do need these, please separate the static functions from the member
functions and add some documentation of what they do

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:877
> +    PlatformLayer* platformLayer = layer->platformLayer();

since we're in chromium specific code here the PlatformLayer typedef is
unnecessary abstraction here - just use LayerChromium here

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:888
> +    PlatformLayer* platformLayer = layer->platformLayer();

same here re: using LayerChromium