[webkit-reviews] review granted: [Bug 228023] [css-scroll-snap] Pass the full target point when selecting a snap offset : [Attachment 433676] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 21 01:45:36 PDT 2021
Frédéric Wang (:fredw) <fred.wang at free.fr> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 228023: [css-scroll-snap] Pass the full target point when selecting a snap
offset
https://bugs.webkit.org/show_bug.cgi?id=228023
Attachment 433676: Patch
https://bugs.webkit.org/attachment.cgi?id=433676&action=review
--- Comment #3 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 433676
--> https://bugs.webkit.org/attachment.cgi?id=433676
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.
More information about the webkit-reviews
mailing list