[webkit-reviews] review granted: [Bug 135768] Implement scroll snapping animations on Mac : [Attachment 236597] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 135768: Implement scroll snapping animations on Mac
https://bugs.webkit.org/show_bug.cgi?id=135768

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236597&action=review


> Source/WebCore/ChangeLog:18
> +	   Implementing the scroll snap animations required for snapping in
both overflow and mainframe areas on Mac. Since we receive a series
> +	   of discrete wheel events, we need to predict the distance we need to
traverse, compute the appropriate snap point, and then compute
> +	   an animation curve to that location. The snap animations are split
into two types: snapping, which handles letting go of the trackpad
> +	   with zero velocity, and gliding, which handles flick gestures. In
both cases, sinusoidal curves are used to ease animation to the target
> +	   location. In the former case, the initial velocity is low, and
increases to a maximum value during the middle of the animation before
> +	   decreasing to 0. In the latter case, the curve is computed such that
the initial velocity matches the user's scroll velocity, and the
> +	   final velocity matches a lower final velocity that is somewhat
arbitrarily computed. (See the FIXME in
> +	   AxisScrollSnapOffsets::initializeGlideParameters). How the equations
and constants were chosen is described in greater detail in the
> +	   comments above AxisScrollSnapOffsets::computeGlideDelta and
AxisScrollSnapOffsets::computeSnapDelta. Note that the final velocity should
> +	   ideally be equal to 0. However, with this particular curve, this
caused the animation to feel too slow near the snap point.
> +

Could you wrap this to a smaller column width?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:33
> +const float inertialScrollPredictionFactor = 16.6789;

Does this really need to have so many significant digits? It makes it look like
it is some scientific value we're not allowed to change. Would 16.7 be good
enough?