[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]

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.