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

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


I haven't looked through everything in detail, but have found some issues.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:170
>      void scrollBy(const IntSize& scroll);
> +    void scrollBy(const FloatSize& scroll);

it's very rarely a good idea to override a function like this with such similar
types when the implementations do fairly different things. what's the story?

do we actually intend to support scrolling by non-integer amounts?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:245
> +    // Tracks if this layer or a descendant was scrolled since the last
sync.
> +    bool m_subtreeWasScrolled;

if i understand correctly, this is an optimization to avoid having to walk the
full layer tree every commit to check for scrolls on sublayers, correct? would
this be better done as a bit on the LTHI? this seems somewhat overdone for a
small win

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:262
> +    FloatSize m_scrollDeltaResidue;

what is the 'scroll delta residue'?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:485
> +    for (size_t i = 0; i < info.scrolls.size(); ++i) {
> +	   if (m_rootScrollLayer && info.scrolls[i].layerId ==
m_rootScrollLayer->id())
> +	      
m_client->applyRootLayerScrollAndScale(info.scrolls[i].scrollDelta,
info.pageScaleDelta);

this logic makes me wonder if trying to be generic about scroll on the root
layer is a mistake and if we should instead keep the root scroll + page scale
delta separate from scrolls on sublayers

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:335
> +CCInputHandlerClient::ScrollStatus
CCLayerTreeHostImpl::beginScrollingInnermostLayerAtPoint(CCLayerImpl*
layerImpl, const IntPoint& windowPoint)

what does "window" mean in "window point"? we don't have a defined notion of a
window in the compositor today. viewport, perhaps? is this scaled or unscaled?

> Source/WebKit/chromium/public/platform/WebLayerTreeViewClient.h:48
> +    // Applies a scroll delta to a non-root layer. This is triggered by
events
> +    // sent to the compositor thread through the WebCompositor interface.
> +    virtual void applyLayerScroll(int layerId, const WebSize& scrollDelta) =
0;

I don't think this makes sense on this interface - why would you talk about a
sublayer through the WebLayerTreeViewClient instead of via that layer? How
would you expect this API to be used?

We also do not (and should not) expose layer IDs via the public API.