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

[bugzilla-daemon at webkit.org]

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

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

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


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

sorry to reverse myself earlier here but now that I think about this more I
don't think this is right. just finding the first most-nested layer is not
going to find what the user really wants, especially if the tree order doesn't
match the z-order very well

instead of iterating through the layer tree in tree order, you really need to
walk in z-order here by going through the CCRenderSurface list

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:488
> +    IntPoint
contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint
));

if the layer's screenSpaceTransform is not invertible, inverse() will return an
identity transform which will probably _not_ do what you expect here. if the
screenSpaceTransform is not invertible we probably do not want to let it
scroll, since that means that some dimension got mapped to zero and the layer
isn't actually visible

also do you need to consider layer backfaces here?