[webkit-reviews] review denied: [Bug 73350] [chromium] Allow scrolling non-root layers in the compositor thread : [Attachment 126307] 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 126307: Patch
https://bugs.webkit.org/attachment.cgi?id=126307&action=review

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


I like this patch and would like to see it move forward, but I think we'll want
to adjust the ScrollableArea-related hookups a bit (see
https://bugs.webkit.org/show_bug.cgi?id=78401).  Would you mind splitting those
bits out of this patch?

Also it seems that we're going to need to propagate more information about the
scroll down through the CCInputHandler if we want to be maximally efficient -
for instance, if one layer in the page listens to wheel events we need to know
that we can't accept scrolls if they are initiated by mouse wheel events *and*
we hit test in that layer.  We can address this separately if we need to.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:50
> +    virtual void wasScrolled(const IntSize&) = 0;

our naming convention for these sorts of calls is either "willXXX" or "didXXX"
- think this is a "didXXX"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:622
> +	      
static_cast<ContentLayerChromium*>(layer)->scrollBy(info.scrolls[i].scrollDelta
);

rather than downcast can we make this virtual on LayerChromium with a no-op
impl for the base class?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:203
> +    // During testing we may not have an active renderer.
> +    const int kDefaultMaxTextureSize = 256;

i would much rather fix tests rather than adding production code to deal with
this case. it's very easy to instantiate a real LRC in unit tests today. if
that's not an option in this case then we could do something like mock out
capabilities