Bug 228023

Created attachment 433674 [details] Patch

Created attachment 433676 [details] Patch

Comment on attachment 433676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433676&action=review LGTM > Source/WebCore/page/scrolling/nicosia/ScrollingTreeScrollingNodeDelegateNicosia.cpp:176 > + auto offsetY = scrollingNode().snapOffsetsInfo().closestSnapOffset(ScrollEventAxis::Vertical, scrollableAreaSize(), newOffset, deltaY, originalOffset.y()).first; It seems before this patch we are passing LayoutUnits. Not sure that will change things much, but shouldn't you continue to pass a LayoutPoint rather than a FloatPoint? If so, the scaling/conversion matter might matter (see setNearestScrollSnapIndexForAxisAndOffset and below). > Source/WebCore/platform/ScrollAnimator.cpp:408 > + return axis == ScrollEventAxis::Horizontal ? newOffset.x() : newOffset.y(); This pattern seems to be repeated at several places in this patch. You may consider factoring this out in a helper function. > Source/WebCore/platform/ScrollAnimator.cpp:417 > + velocity = axis == ScrollEventAxis::Horizontal ? velocityInBothAxes.width() : velocityInBothAxes.height(); nit: It sounds to me more logical to rename the variables velocity = newOffset - currentOffset; and velocityOnScrollEventAxis = axis == ScrollEventAxis::Horizontal ? velocityInBothAxes.width() : velocityInBothAxes.height(); > Source/WebCore/platform/ScrollController.cpp:139 > + layoutScrollOffset.scale(1.0 / scaleFactor); I don't know if that makes a difference, but it looks like if we want to preserve the current logic this would be LayoutPoint layoutScrollOffset(scrollOffset.x() / scaleFactor, scrollOffset.y() / scaleFactor) where the scaling is performed before the conversion to LayoutUnit. > Source/WebCore/platform/ScrollController.cpp:170 > + layoutDestinationOffset.scale(1.0 / m_client.pageScaleFactor()); Same here, the scaling was done before the conversion to LayoutUnit in the old code. > Source/WebCore/platform/ScrollSnapAnimatorState.cpp:51 > + auto predictedScrollTarget = FloatPoint { m_momentumCalculator->predictedDestinationOffset() }; Why not just FloatPoint predictedScrollTarget { m_momentumCalculator->predictedDestinationOffset() }; ? > Source/WebCore/platform/ScrollSnapAnimatorState.cpp:101 > + predictedLayoutOffset.scale(1.0 / pageScale); Same here, previous code was doing the scaling before LayoutUnit conversion. And you still do that at the next line for startOffset.

Created attachment 433939 [details] Patch

Comment on attachment 433676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433676&action=review Thanks for the review. I've uploaded a new version of the change that addresses the issues you've raised. I'll land that now. >> Source/WebCore/page/scrolling/nicosia/ScrollingTreeScrollingNodeDelegateNicosia.cpp:176 >> + auto offsetY = scrollingNode().snapOffsetsInfo().closestSnapOffset(ScrollEventAxis::Vertical, scrollableAreaSize(), newOffset, deltaY, originalOffset.y()).first; > > It seems before this patch we are passing LayoutUnits. Not sure that will change things much, but shouldn't you continue to pass a LayoutPoint rather than a FloatPoint? If so, the scaling/conversion matter might matter (see setNearestScrollSnapIndexForAxisAndOffset and below). The ScrollSnapOffsets here are FloatScrollSnapOffsets, so even though the original code was creating a LayoutPoint it was silently converting the components from LayoutUnit to float for the call to closestSnapOffset. The new version removes this trip into LayoutUnits and back to floats. >> Source/WebCore/platform/ScrollAnimator.cpp:408 >> + return axis == ScrollEventAxis::Horizontal ? newOffset.x() : newOffset.y(); > > This pattern seems to be repeated at several places in this patch. You may consider factoring this out in a helper function. I did consider this, but ultimately didn't do it because I couldn't figure out where this helper would belong. >> Source/WebCore/platform/ScrollAnimator.cpp:417 >> + velocity = axis == ScrollEventAxis::Horizontal ? velocityInBothAxes.width() : velocityInBothAxes.height(); > > nit: It sounds to me more logical to rename the variables velocity = newOffset - currentOffset; and velocityOnScrollEventAxis = axis == ScrollEventAxis::Horizontal ? velocityInBothAxes.width() : velocityInBothAxes.height(); This makes sense to me. I've done this rename. >> Source/WebCore/platform/ScrollController.cpp:139 >> + layoutScrollOffset.scale(1.0 / scaleFactor); > > I don't know if that makes a difference, but it looks like if we want to preserve the current logic this would be > > LayoutPoint layoutScrollOffset(scrollOffset.x() / scaleFactor, scrollOffset.y() / scaleFactor) > > where the scaling is performed before the conversion to LayoutUnit. Okay. I can keep this more similar to the previous version. >> Source/WebCore/platform/ScrollController.cpp:170 >> + layoutDestinationOffset.scale(1.0 / m_client.pageScaleFactor()); > > Same here, the scaling was done before the conversion to LayoutUnit in the old code. Okay. I've changed this back. >> Source/WebCore/platform/ScrollSnapAnimatorState.cpp:51 >> + auto predictedScrollTarget = FloatPoint { m_momentumCalculator->predictedDestinationOffset() }; > > Why not just FloatPoint predictedScrollTarget { m_momentumCalculator->predictedDestinationOffset() }; ? Oh, this is much cleaner. Thanks! >> Source/WebCore/platform/ScrollSnapAnimatorState.cpp:101 >> + predictedLayoutOffset.scale(1.0 / pageScale); > > Same here, previous code was doing the scaling before LayoutUnit conversion. And you still do that at the next line for startOffset. Fixed.

Committed r280171 (239866@main): <https://commits.webkit.org/239866@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433939 [details].

<rdar://problem/80949086>