Bug 74196

Created attachment 118606 [details] Patch

Created attachment 119033 [details] Patch - Rebased after latest changes to bug 73350.

Comment on attachment 119033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119033&action=review Other than one small thing, this is looking good to me. Thanks for the tests! > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:341 > + IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint)); > + // The content point for non-root layers must be scaled with the page scale. Note that the > + // page scale delta is already embedded in each layer's screen space transform. > + if (layerImpl != m_scrollLayerImpl) > + contentPoint.scale(m_pageScale, m_pageScale); I know you're just moving code here, but I don't follow this logic and why the scroll layer behaves differently. Would you mind explaining why this is? Don't all layers (root or no) have the page scale applied to them? Also, do you need to apply the page scale delta? Maybe I'd feel more comfortable if there were more tests for this function that covered page scale.

Comment on attachment 119033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119033&action=review Setting as review- until the preceding patch layer scrolling is updated. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:341 > > I know you're just moving code here, but I don't follow this logic and why the scroll layer behaves differently. Would you mind explaining why this is? Don't all layers (root or no) have the page scale applied to them? > > Also, do you need to apply the page scale delta? Maybe I'd feel more comfortable if there were more tests for this function that covered page scale. Turns out the code here is not entirely correct, so I'll rework it in the preceding patch. Actually there is no need to differentiate between layers or scale the content point, but instead visibleLayerRect() must be scaled with the inverse of the page scale to get unscaled css coordinates that match the coordinate system of contentPoint. The reason why page scale delta is not needed is that while we are zooming, visibleLayerRect() is recomputed in terms of the original page scale. It never includes m_pageScaleDelta so there is no need to undo that here. I'm not really sure whether this is intended, though.

Created attachment 119287 [details] Patch

This seems to have quite a few merge conflicts on ToT, does it depend on https://bugs.webkit.org/show_bug.cgi?id=73350 ?

(In reply to comment #6) > This seems to have quite a few merge conflicts on ToT, does it depend on https://bugs.webkit.org/show_bug.cgi?id=73350 ? Yes, this patch should be applied after the one you linked. Sorry, I should have mentioned that here.

Created attachment 119684 [details] Patch - Rebased after changes to bug 73350 (no functional changes)

Comment on attachment 119684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119684&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:486 > + IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint)); if this layer's screen space transform is not invertible, you need to return false here - just calling .inverse().mapPoint() will map the point through an identity transform

Created attachment 120064 [details] Patch - Updating according to layer walking order changes in bug 73350.

Created attachment 120068 [details] Patch - Rebased.

Comment on attachment 120068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120068&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:480 > + if (!layerImpl) > + return false; when can this parameter be null? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:489 > + if (!layerImpl->screenSpaceTransform().isInvertible()) > + return false; > + > + IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint)); > + if (layerImpl->drawsContent() && !isContentPointWithinLayer(layerImpl, contentPoint)) > + return false; > + > + if (layerImpl->isInsideScrollableRegion(contentPoint)) there's not a whole lot (if anything) going on here that the CCLayerTreeHostImpl can do and the layer itself can't - all of the logic here is based on member functions of layerImpl. should this whole thing just be a member function of CCLayerImpl? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:526 > + if (!layerImpl->drawsContent() && (layerImpl->scrollable() || !layerImpl->scrollableRegion().isEmpty())) at this point, this probably deserves at least a comment explaining why a given layer should or shouldn't be in the assembled list since it's no longer a list of non-drawable scrollable layers

*** This bug has been marked as a duplicate of bug 79155 ***