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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-08-12 10:09:44 PST ---
(From update of attachment 236408)
View in context: https://bugs.webkit.org/attachment.cgi?id=236408&action=review

Thanks for taking a look at this! There will be a more polished version of this up soon.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:77
>> +    float glideAmount() const;
> 
> "Amount" is vague here. Maybe "distance"?

Changed to compute(Snap|Glide)Delta.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:80
>> +    bool pushInitialMomentum(float);
> 
> What is the parameter? Velocity?

Changed names from "Momentum" to "WheelDelta" to better reflect the fact that I'm tracking wheel deltas over time.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:81
>> +    float averageInitialMomentum() const;
> 
> Again, what are the units?

Changed names from "Momentum" to "WheelDelta" to better reflect the fact that I'm tracking wheel deltas over time.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:92
>> +    static const float initialToFinalMomentumFactor;
> 
> Just have these in the cpp file as non-class consts.

Fixed. (although momentumWindowSize is still declared in the header, since I'll need it to initialize the momentum window).

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

Changed to m_numWheelDeltasTracked.

>> 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.

Got it.

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

Fixed.

>> 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?

Changed to scrollSnapAnimationUpdate()

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

Will do. I'm also putting this information in the (soon to come) ChangeLog.

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

Adding comments here too.

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