[Webkit-unassigned] [Bug 135768] Implement scroll snapping animations on Mac

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135768


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #236408|review?                     |review-
               Flag|                            |




--- Comment #5 from Simon Fraser (smfr) <simon.fraser at apple.com>  2014-08-11 16:47:34 PST ---
(From update of attachment 236408)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.