[Webkit-unassigned] [Bug 155750] [GTK] Add kinetic scrolling

[bugzilla-daemon at webkit.org]

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

--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 276397
  --> https://bugs.webkit.org/attachment.cgi?id=276397
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276397&action=review

Some general comments only for now.

> Source/WTF/wtf/FeatureDefines.h:320
> +#if !defined(ENABLE_KINETIC_SCROLLING)
> +#define ENABLE_KINETIC_SCROLLING 1
> +#endif

We don't need any compile option for this, just add the ScrollAnimationKinetic.cpp to the GTK compilation.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:87
> +ScrollAnimationKinetic::PerAxisData::PerAxisData()
> +    : PerAxisData(0, 0, 0, 0)
> +{
> +}

Initialize the members when declared in the header file and remove the explicit constructor

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:96
> +    , m_equilibriumPosition(0)
> +    , m_t(0)

Initialize constant values in the header when declared.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:104
> +    m_t += timeDelta;

Do not use abbreviations for members, what does t mean?

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:138
> +    if (animationTimerActive())
> +        stop();

stop already checks if the animation is active.

>> Source/WebCore/platform/ScrollAnimationKinetic.cpp:144
>> +    m_scrollHistory.append(ScrollEvent(orientation, step * multiplier, currentTime));
> 
> What about letting ScrollAnimatorGtk handle the scrolling history and passing the computed inertia to ScrollAnimationKinetic when triggering inertial scrolling? This may help using the velocity values from the swipe gesture directly.

Sounds good.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:165
> +void ScrollAnimationKinetic::serviceAnimation()

serviceAnimation comes from RAf when not using a timer, but we are using a timer. See ScrollAnimatorSmooth, we are not actually using serviceAnimation in GTK port.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:172
> +    FloatPoint accumDelta(0, 0);

The default constructor already initializes its member to 0, so you don't need (0, 0);

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:186
> +    if (velocity == FloatPoint(0, 0))

Ditto.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:196
> +    animationTimerFired();

I'm not sure I understand this, serviceAnimation should be equivalent to animationTimerFired().

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:228
> +void ScrollAnimationKinetic::startNextTimer(double delay)
> +{
> +    m_animationTimer.startOneShot(delay);
> +}

We have this in ScrollAnimationSmooth because we support RAF without the timer option, but in this case we can just use m_animationTimer.startOneShot(delay) directly.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:233
> +bool ScrollAnimationKinetic::animationTimerActive() const
> +{
> +    return m_animationTimer.isActive();
> +}

Ditto.

>> Source/WebKit2/UIProcess/gtk/GestureController.cpp:218
>> +void GestureController::SwipeGesture::swipe(SwipeGesture* swipeGesture, double x, double y, GtkGesture* gesture)
> 
> x and y (which should probably be named velocityX and velocityY) could (should?) be used to trigger inertial scrolling.

Indeed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160425/5d45db1a/attachment-0001.html>