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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 236408: Patch
https://bugs.webkit.org/attachment.cgi?id=236408&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236408&action=review


r- for lack of changelog. I would also like to see a fairly high-level comment
somewhere that explains the states, and briefly describes how the math works.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:77
> +    float snapAmount() const;
> +    float glideAmount() const;

"Amount" is vague here. Maybe "distance"?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:80
> +    bool pushInitialMomentum(float);

What is the parameter? Velocity?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:81
> +    float averageInitialMomentum() const;

Again, what are the units?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:92
> +    static const int momentumWindowSize = 3;
> +    static const int snapMagnitudeMax;
> +    static const int snapMagnitudeMin;
> +    static const int snapThresholdHigh;
> +    static const int snapThresholdLow;
> +    static const float glideBoostMultiplier;
> +    static const float maxTargetVelocity;
> +    static const float minTargetVelocity;
> +    static const float initialToFinalMomentumFactor;

Just have these in the cpp file as non-class consts.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:103
> +    int m_momentumCount;

event count?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:54
> +	   finalVelocity = ((targetMultiplier * initialMomentum) / 2) * (1 +
cos(piFloat - acos((2 - targetMultiplier) / targetMultiplier)));

You need to explain this in a comment.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:61
> +	   else if (finalVelocity > targetFinalVelocity + 0.5)

No blank line before else.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:114
> +template<typename T> inline void clampToBounds(T& value, T min, T max)
> +{
> +    if (value < min)
> +	   value = min;
> +
> +    if (value > max)
> +	   value = max;
> +}

We normally use std::max(std::min(a, b), c) for this.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:182
> +void AxisScrollSnapAnimator::scrollUpdate()

Name is vague. Are you updating a scroll, or are you updating as the result of
a scroll?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:241
> +    float rawSnapAmount = 1 + magnitude * sin(piFloat * progress);

Comment?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:256
> +    float shift = ceil(((m_glideMultiplier * m_glideInitialMomentum) / 2) *
(1 + cos(piFloat * progress - m_glidePhaseShift)));

Comment!