Bug 155750 – [GTK] Add kinetic scrolling

• Component: UI Events
• Status: RESOLVED FIXED

[Adrien Plazas]

We should enable kinetic scrolling when using touch enabled input devices (touchpads, touchscreens...).

[Adrien Plazas]

Created attachment 274652 [details]
WIP patch

Any thought on this WIP implementation?

[Adrien Plazas]

What about replacing mentions to 'kinetic' as 'inertial' as kinetic scrolling was actually already implemented in WebKitGTK+, what this patch adds is inertial scrolling (which extends kinetic scrolling), if the second paragraph here is correct: https://en.wikipedia.org/wiki/Scrolling#Computing

[Frédéric Wang (:fredw)]

Comment on attachment 274652 [details]
WIP patch

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

> Source/WebCore/platform/ScrollAnimationKinetic.h:73
> +

For this three functions you have space before the (), but not for others. I don't remember what is WebKit convention, but maybe you should try running check-webkit-style.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:131
> +

extra new line?

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:138
> +

extra new line?

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:77
> +

Do you really want two new lines?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1811
> +//            return;

Do you mean to remove that code rather than just commenting it out?

[Adrien Plazas]

Comment on attachment 274652 [details]
WIP patch

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1811
>> +//            return;
> 
> Do you mean to remove that code rather than just commenting it out?

Actually I'm not sure what to do about these lines: they prevent some important events notifying the beginning or the end of a scrolling sequence from continuing their way down in due time, making the inertial scrolling work imperfectly.

[Carlos Garcia Campos]

Comment on attachment 274652 [details]
WIP patch

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

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:35
> +static const double overshootFriction = 20;

This is only about kinetic scrolling for now, this patch doesn't really support overshot animations, so let's keep that out for now.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:50
> +    , m_dx(orientation == HorizontalScrollbar ? step * multiplier : 0)
> +    , m_dy(orientation == VerticalScrollbar ? step * multiplier : 0)

Do not use abbreviations for variables, use m_delatX, m_deltaY for example instead of m_dx, m_dy. Or even better FloatSize m_delta

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:54
> +ScrollAnimationKinetic::KineticScrolling::KineticScrolling ()

Space between KineticScrolling and ()

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:59
> +ScrollAnimationKinetic::KineticScrolling::KineticScrolling (double lower, double upper, double initialPosition, double initialVelocity)

Space between KineticScrolling and (

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:60
> +    : m_phase(Phase::Decelerating)

This should be the only phase for now, since we only implement deceleration in this patch.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:63
> +    , m_overshootWidth(0)

Initialize constant values in class declaration.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:64
> +    , m_decelFriction(decelFriction)

Why do we need a member for a constant value?

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:67
> +    , m_c1(0)
> +    , m_c2(0)

Do not use abbreviations, and move initialization to the class declaration.

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

Ditto.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:73
> +    if(initialPosition < lower)

Missing space between if and (

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:81
> +        m_t = 0;

This is already 0

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:83
> +        m_position = initialPosition;
> +        m_velocity = initialVelocity;

And these also have their own initial values already.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:87
> +bool ScrollAnimationKinetic::KineticScrolling::tick (double time_delta)

tick (-> tick(
time_delta -> timeDelta

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:93
> +        double exp_part = exp (-m_decelFriction * m_t);

exp_part -> whatever it stands for without _

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:150
> +#if USE(REQUEST_ANIMATION_FRAME_TIMER)

Since this will be supported only by GTK+ for now, and you are not going to test the !REQUEST_ANIMATION_FRAME_TIMER case, let's get rid of this, and use always a timer.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:167
> +    m_scrollHistory.append(ScrollEvent(orientation, step, multiplier, monotonicallyIncreasingTime()));

This can grow indefinitely, it seems GTK+ removes events older than a constant threshold (150ms). ScrollEvent doesn't really need both step and multiplier, so you can pass the delta directly, and the current time can also be obtained by the ScrollEvent constructor instead of passing it. But I don't think it's the current time what we want, but the event time.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:169
> +    FloatPoint currentPosition = m_position;

This is not actually used, you can use m_position below, since you are not changing it.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:200
> +void ScrollAnimationKinetic::setCurrentPosition(const FloatPoint& p)

p -> position

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:207
> +void ScrollAnimationKinetic::startDeceleration() {

the { should be in the next line.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:214
> +void ScrollAnimationKinetic::cancelDeceleration() {
> +    stop();
> +}

Do we need this?

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:230
> +    double accum_dx = 0;
> +    double accum_dy = 0;

Don't use abbreviations nor _

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:231
> +    for (auto i = m_scrollHistory.begin(); i != m_scrollHistory.end(); i++) {

for (const auto& scrollEvent : m_scrollHistory)
    scrollEvent.dx();

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:236
> +    double first = m_scrollHistory.begin()->time();

You can use m_scrollHistory[0].time()

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:260
> +    if (!m_scrollHistory.isEmpty())
> +        initDeceleration();

initDeceleration already checks if scroll history is empty. I don't understand why the animation fire is starting the deceleration, I think it should be the other way around, startDeceleration should schedule the timer animation.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:266
> +    if (m_horizontalData.tick(deltaToNextFrame))

Use animateScroll or decelerate instead of tick

> Source/WebCore/platform/ScrollAnimationKinetic.h:31
> +#if ENABLE(SMOOTH_SCROLLING)

This should not depend on SMOOTH_SCROLLING

> Source/WebCore/platform/ScrollAnimationKinetic.h:35
> +#if !ENABLE(REQUEST_ANIMATION_FRAME)
> +#error "SMOOTH_SCROLLING requires REQUEST_ANIMATION_FRAME to be enabled."
> +#endif

Remove this also.

> Source/WebCore/platform/ScrollAnimationKinetic.h:58
> +    class ScrollEvent {
> +    public:
> +        ScrollEvent(ScrollbarOrientation, float step, float multiplier, double time);
> +        double time() const { return m_time; }
> +        double dx() const { return m_dx; }
> +        double dy() const { return m_dy; }
> +
> +    private:
> +        double m_time;
> +        double m_dx;
> +        double m_dy;
> +
> +    };

This could probably be a struct

> Source/WebCore/platform/ScrollAnimationKinetic.h:60
> +    // KineticScrolling is a port of GtkKineticScrolling as of GTK+ 3.20, mimicking its API and its behavior.

We can mention in the cpp that the whole implementation is based on GTK+ code.

> Source/WebCore/platform/ScrollAnimationKinetic.h:97
> +        Phase m_phase;
> +        double m_lower;
> +        double m_upper;
> +        double m_overshootWidth;
> +        double m_decelFriction;
> +        double m_overshootFriction;
> +
> +        double m_c1;
> +        double m_c2;
> +        double m_equilibriumPosition;
> +
> +        double m_t;
> +        double m_position;
> +        double m_velocity;
> +    };

And we don't probably need a class for this either.

> Source/WebCore/platform/ScrollAnimationKinetic.h:109
> +    enum class Curve {
> +        Linear,
> +        Quadratic,
> +        Cubic,
> +        Quartic,
> +        Bounce
> +    };

Copy paste leftover, I guess.

> Source/WebCore/platform/ScrollAnimationKinetic.h:112
> +    void updateVisibleLengths() override;

Since this is pure virtual, but unimplemented, you can add { } here instead of adding the empty implementation in the cpp file.

> Source/WebCore/platform/ScrollAnimationKinetic.h:115
> +    void startDeceleration();
> +    void cancelDeceleration();

These could be just start() and stop()

> Source/WebCore/platform/ScrollAnimator.cpp:129
> +#elif PLATFORM(GTK)
> +    // GDK scroll stop events are used to notify that the scrolling came to an end and that deceleration can be triggered.
> +    if (e.isStop()) {
> +        startDeceleration();
> +        return true;
> +    }
> +    else
> +        cancelDeceleration();

We should override handleWheelEvent in ScrollAnimatorGtk and handle this there instead.

> Source/WebCore/platform/ScrollAnimator.h:94
> +    virtual void startDeceleration() { }
> +    virtual void cancelDeceleration() { }

I don't think this belongs here, ScrollAnimatorGtk can start/stop its animations internally

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:90
> +#if GTK_CHECK_VERSION(3, 19, 7)
> +    m_isStop = event->is_stop;
> +#else
> +    m_isStop = false;
> +#endif

Hmm, I'm surprised this depends on GTK+ 3.19, kinetic scrolling was supported before, there should be another way.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:80
> +    if (m_kinetic_animation)

m_kinetic_animation -> m_kineticAnimation

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:93
> +    ensureKineticScrollingAnimation();

We don't need the ensure method here, since this is created unconditionally in the constructor.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:97
> +    m_kinetic_animation->scroll(orientation, granularity, step, multiplier);
> +
>      if (!m_scrollableArea.scrollAnimatorEnabled() || granularity == ScrollByPrecisePixel)
>          return ScrollAnimator::scroll(orientation, granularity, step, multiplier);

How does smooth and kinetic work together? I guess we should check if there's a deceleration in progress and only do the smooth when there isn't (if enabled, of course)

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:109
>      if (m_animation)

We should rename this now to m_smoothAnimation

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:185
> +#if GTK_CHECK_VERSION(3, 19, 7)
> +    bool isStop = gdk_event_is_scroll_stop_event (event);
> +#else
> +    bool isStop = false;
> +#endif

Maybe we can guess this, if direction is smooth and delta x = delta y = 0 then it's stop. Not sure that's always true, though.

> Source/WebKit2/UIProcess/gtk/GestureController.cpp:92
> +    scrollEvent->scroll.is_stop = false;

false -> FALSE

> Source/WebKit2/UIProcess/gtk/GestureController.cpp:111
> +    scrollEvent->scroll.is_stop = false;

Ditto.

> Source/WebKit2/UIProcess/gtk/GestureController.cpp:130
> +    scrollEvent->scroll.is_stop = true;

Ditto.

> Source/WebKit2/UIProcess/gtk/GestureController.h:74
> +        void stopDrag(const GdkEvent*);

Shouldn't we use a swipe gesture instead?

[Adrien Plazas]

Created attachment 276397 [details]
WIP patch

Fixed all (most?) of the comments except for turning KineticScrolling (now renamed as PerAxisData) into a struct.

I also fixed several other details and made the scrolling _really_ work as intended.

[Adrien Plazas]

Comment on attachment 276397 [details]
WIP patch

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

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

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

[Carlos Garcia Campos]

Comment on attachment 276397 [details]
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.

[Adrien Plazas]

Comment on attachment 276397 [details]
WIP patch

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

>> Source/WTF/wtf/FeatureDefines.h:320
>> +#endif
> 
> We don't need any compile option for this, just add the ScrollAnimationKinetic.cpp to the GTK compilation.

Do you mean I should:
- remove the changes made to Source/WebCore/PlatformGTK.cmake and instead
- add platform/ScrollAnimationKinetic.cpp to either "list(APPEND WebCore_SOURCES" or "list(APPEND WebCorePlatformGTK_SOURCES..." in Source/WebCore/PlatformGTK.cmake and
- remove any mention to ENABLE(KINETIC_SCROLLING)?

If yes, to which list should I add ScrollAnimationKinetic.cpp?

[Adrien Plazas]

Created attachment 277468 [details]
WIP patch

This patch uses the velocity from the swiping gesture to trigger the inertial scrolling, the scroll animator computes the velocity (previously the animation was doing it) when a "stop" event triggers the inertial scrolling, the "KINETIC_SCROLLING" guard has been removed and lots of smaller fixes as been introduced.

[Carlos Garcia Campos]

Comment on attachment 277468 [details]
WIP patch

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

Some more general comments

> Source/WebCore/platform/PlatformWheelEvent.h:32
> +#if PLATFORM(GTK)
> +#include "FloatPoint.h"
> +#endif

Don't use platform ifdefs to include cross-platform headers, just include the header if it's really needed.

> Source/WebCore/platform/PlatformWheelEvent.h:73
> +    enum PlatformWheelEventPhase {
> +        PlatformWheelEventPhaseNone        = 0,
> +        PlatformWheelEventPhaseStop        = 1 << 0,
> +        PlatformWheelEventPhaseSwipe       = 1 << 1,

Do not line up this enum with the previous one. I wonder if we could reuse the cocoa enum even if we don't use all the flags, or are they too different?

> Source/WebCore/platform/PlatformWheelEvent.h:95
> +            , m_phase(PlatformWheelEventPhaseNone)

Initialize this below when the member is delcared and you don't need to do this in every constructor.

> Source/WebCore/platform/PlatformWheelEvent.h:96
> +            , m_swipeVelocity(FloatPoint())

This is not needed

> Source/WebCore/platform/PlatformWheelEvent.h:120
> +            , m_phase(PlatformWheelEventPhaseNone)
> +            , m_swipeVelocity(FloatPoint())

Ditto.

> Source/WebCore/platform/PlatformWheelEvent.h:182
> +        FloatPoint swipeVelocity() const { return m_swipeVelocity; }

This should return a const reference instead of a copy.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:112
> +    , m_horizontalData(0, 0, 0, 0)
> +    , m_verticalData(0, 0, 0, 0)

These are not needed.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:123
> +    return true;

Why is this empty now?

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

I would do if (!velocity.x() && !velocity.y()) instead of creating a zero FloatPoint just to compare it.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:53
> +    // FIXME Should this be moved to the initializer list even if it's a bit complex?

I think it's fine here in the body.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:55
> +        updatePosition(FloatPoint(position));

You should move the position, updatePosition(WTFMove(position));

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:59
> +#if ENABLE(SMOOTH_SCROLLING)
> +        if (m_smoothAnimation)
> +            m_smoothAnimation->setCurrentPosition(position);
> +#endif

Maybe updatePosition() should set the current position on both animations to keep everything in sync whenever the position is updated?

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:111
> +const FloatPoint ScrollAnimatorGtk::computeVelocity()

Since we are returning a new FloatPoint, why making it const?

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:131
> +bool ScrollAnimatorGtk::handleWheelEvent(const PlatformWheelEvent& e)

e -> event

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:136
> +        return e.timestamp() - event.timestamp() > 150;

Use a global static const for this 150 instead of magic number.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:58
> +
> +#if ENABLE(SMOOTH_SCROLLING)
> +    void ensureSmoothScrollingAnimation();
>  #endif
> +    const FloatPoint computeVelocity();
> +
> +    void updatePosition(FloatPoint&&);

Move this below after the virtual methods

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:93
> +    std::function<void(FloatPoint&&)> m_positionChangedCallback;

What is this?

> Source/WebKit2/Shared/WebEvent.h:223
> +    WebCore::FloatPoint swipeVelocity() const { return m_swipeVelocity; }

This should return a const reference.

> Source/WebKit2/Shared/WebEventConversion.cpp:162
> +        m_phase = static_cast<WebCore::PlatformWheelEventPhase>(webEvent.phase());

We normally try to avoid casts to convert from/to enums, because it's very fragile in case any of the enum changes. Better use to/from conversion functions.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:195
> +#if GTK_CHECK_VERSION(3, 19, 7)

Since 3.20 was already released, let's check for 3, 20, 0 better instead of using an unstable release number.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:199
> +#elif GTK_CHECK_VERSION(3, 3, 18)

I think 3.6 is the minimum GTK+ version we support, so we don't need to check for 3.3 version.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1835
> +#if PLATFORM(GTK)
> +#if GTK_CHECK_VERSION(3, 19, 7)
> +        bool isScrollStopEvent = gdk_event_is_scroll_stop_event(event.nativeEvent());
> +#else
> +        const GdkEventScroll& scroll = event.nativeEvent()->scroll;
> +        bool isScrollStopEvent = scroll.direction == GDK_SCROLL_SMOOTH && scroll.delta_x && scroll.delta_y;
> +#endif
> +        if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold && !isScrollStopEvent)
> +            return;
> +#else
>          if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold)
>              return;
> +#endif

We need a better way to handle this than this ifdef mess here :-) We could have a method in the page client, for example, to check if a given wheel event should never be discarded, with this implementation in the GTK+ page client and returning false for all other ports.

[Adrien Plazas]

Comment on attachment 277468 [details]
WIP patch

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

>> Source/WebCore/platform/PlatformWheelEvent.h:73
>> +        PlatformWheelEventPhaseSwipe       = 1 << 1,
> 
> Do not line up this enum with the previous one. I wonder if we could reuse the cocoa enum even if we don't use all the flags, or are they too different?

What about using PlatformWheelEventPhaseEnded to represent both Stop and Swipe, the animator could then compute the velocity from the history if the one from the "Ended" event is 0:0.

>> Source/WebCore/platform/ScrollAnimationKinetic.cpp:123
>> +    return true;
> 
> Why is this empty now?

I don't really know what to do about this one, a "scroll" method doesn't really make sense here as we don't feed this animation scrolling values but a position to scroll from and an inertia.

In previous patches it was used to store the event and compute the velocity from the history but it was suboptimal as we needed the timestamp of the event, which "scroll"'s prototype don't allow us to have. This is now handled by the scroll animator which handles it better as it actually handles the raw events.

Also when using the velocity from the swipe gesture, "scroll" is completly useless.

>> Source/WebCore/platform/ScrollAnimationKinetic.cpp:142
>> +    if (velocity == FloatPoint())
> 
> I would do if (!velocity.x() && !velocity.y()) instead of creating a zero FloatPoint just to compare it.

What about adding a "bool FloatPoint::isOrigin() const" method? We could do  "if (!velocity.isOrigin())" instead, it would be more clear and avoid one method call.

We could also use it to check if we need to compute the velocity as I suggested in a previous comment.

>> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:59
>> +#endif
> 
> Maybe updatePosition() should set the current position on both animations to keep everything in sync whenever the position is updated?

setCurrentPosition()'s semantic on both animations is the one of a non animated jump (if I'm correct), stopping the animation, so a callback from the inertial scrolling would instantaneously stop it. We would need a method to just set the position without stopping the animation, does it sound OK to you to add one?

>> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:93
>> +    std::function<void(FloatPoint&&)> m_positionChangedCallback;
> 
> What is this?

A leftover from a test I forgot to remove.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1835
>> +#endif
> 
> We need a better way to handle this than this ifdef mess here :-) We could have a method in the page client, for example, to check if a given wheel event should never be discarded, with this implementation in the GTK+ page client and returning false for all other ports.

Makes sense, thanks for the advice!

[Adrien Plazas]

Created attachment 277635 [details]
WIP patch

[Adrien Plazas]

Created attachment 281091 [details]
Add inertial scrolling to WebKitGTK+

Would you like me to split this patch into several smaller units to speed up the review process?

[Carlos Garnacho]

(In reply to comment #14)
> Created attachment 281091 [details]
> Add inertial scrolling to WebKitGTK+
> 
> Would you like me to split this patch into several smaller units to speed up
> the review process?

There is no need, and apologies that it took this much long on my side... I'm dropping some comments on the patch, but it actually looks quite good already to me. Maybe some of my comments get into code style, but take these with a pinch of salt though, I'd trust more Carlos Garcia's opinion on those than mine :).

[Carlos Garnacho]

Comment on attachment 281091 [details]
Add inertial scrolling to WebKitGTK+

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

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:71
> +    return (x > high) ? high : ((x < low) ? low : x);

I'd suggest here an assert for low <= high. Shouldn't happen from reading the code, but IMO better to protect against values that'd make this function not work as it'd be expected.

> Source/WebCore/platform/graphics/FloatPoint.h:124
> +    bool isOrigin() const { return !m_x && !m_y; }

I'm usually a bit wary to use ! to check for floats/doubles being 0. Perhaps C++ will do the right thing here, but '!' suggests binary-level operation to me, which might not turn out as expected even if the number is smaller than FLT_EPSILON.

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:93
> +    m_phase = event->direction == GDK_SCROLL_SMOOTH && !m_deltaX && !m_deltaY ?

same comment here about checking for doubles being equal to 0.0.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:154
> +    WebWheelEvent::Phase phase = event->scroll.direction == GDK_SCROLL_SMOOTH && !deltaX && !deltaY ?

same here with deltaX/Y :)

> Source/WebKit2/UIProcess/gtk/GestureController.cpp:80
> +void GestureController::DragGesture::startDrag(const GdkEvent* event)

It seems this file has now three functions that create a scroll event out of a touch event: this one, startMomentumScroll() a bit further in this patch, and handleDrag() already in code. Maybe a common helper function could be done for these 3?

[Adrien Plazas]

About the several comments on floating point value: you're right it's cleaner to check that way, thanks!

> > Source/WebKit2/UIProcess/gtk/GestureController.cpp:80
> > +void GestureController::DragGesture::startDrag(const GdkEvent* event)
> 
> It seems this file has now three functions that create a scroll event out of
> a touch event: this one, startMomentumScroll() a bit further in this patch,
> and handleDrag() already in code. Maybe a common helper function could be
> done for these 3?

I just added this:
static GUniquePtr<GdkEvent> newScrollEvent(const GdkEvent* event, double x, double y, double deltaX, double deltaY, gboolean isStop)
{
    GUniquePtr<GdkEvent> scrollEvent(gdk_event_new(GDK_SCROLL));
    scrollEvent->scroll.time = event->touch.time;
    scrollEvent->scroll.x = x;
    scrollEvent->scroll.y = y;
    scrollEvent->scroll.x_root = event->touch.x_root;
    scrollEvent->scroll.y_root = event->touch.y_root;
    scrollEvent->scroll.direction = GDK_SCROLL_SMOOTH;
    scrollEvent->scroll.delta_x = deltaX;
    scrollEvent->scroll.delta_y = deltaY;
    scrollEvent->scroll.state = event->touch.state;
#if GTK_CHECK_VERSION(3, 20, 0)
    scrollEvent->scroll.is_stop = isStop;
#endif
    return scrollEvent;
}

I'll soon (tomorrow?) publish a hopefully final version then, thanks for the review!

[Adrien Plazas]

Created attachment 281354 [details]
[GTK] Add kinetic scrolling

This patch only has minor changes fixing the points raised by Carlos Garnacho and update the changelogs.

Should tests be added for this patch and are the changelogs good enough?

[WebKit Commit Bot]

Attachment 281354 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andre Moreira Magalhaes]

Created attachment 281711 [details]
Patch - new features + bugfix

I've used this patch in an internal project and made some modifications that you may find useful. I am posting the updated patch without ChangeLogs, feel free to update it or let me know if you prefer if I do it.

The changes included in this patch (on top of latest one from Adrien):
- overshoot animations support (ScrollAnimationKinetic.cpp)
- bugfix for a crash when starting a drag while scrolling (GestureController.cpp)
- build fix for gtk < 3.20 (PlatformWheelEventGtk.cpp)
- bugfix for gtk < 3.20 to properly copy NativeWebWheelEvent (is_stop is not available in gtk < 3.20 and the phase/momentumPhase were not being properly copied on the NativeWebWheelEvent copy constructor, making the page "jump" during scroll instead of starting momentum scroll - NativeWebWheelEventGtk.cpp)
- respect scrollbar policy (ScrollAnimationKinetic.cpp + ScrollAnimatorGtk.cpp)

That should be it, please let me know what you think.

[Carlos Garcia Campos]

Comment on attachment 281354 [details]
[GTK] Add kinetic scrolling

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

Sorry for the delay reviewing this. Looks quite good in general, but there are still several minor issues to solve. Also, please check the EWS after submitting a patch to make sure it builds.

> Source/WebCore/platform/PlatformWheelEvent.h:168
> +        bool useLatchedEventElement() const { return false; }
> +        bool isEndOfNonMomentumScroll() const;
> +        bool isTransitioningToMomentumScroll() const;

We should reorder what we share with cocoa instead of duplicating it

> Source/WebCore/platform/PlatformWheelEvent.h:198
> +#elif PLATFORM(GTK)
> +        PlatformWheelEventPhase m_phase { PlatformWheelEventPhaseNone };
> +        PlatformWheelEventPhase m_momentumPhase { PlatformWheelEventPhaseNone };
>  #endif

Ditto.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:126
> +bool ScrollAnimationKinetic::scroll(ScrollbarOrientation, ScrollGranularity, float, float)
> +{
> +    // This method isn't needed and shouldn't do anything as 'scroll' isn't
> +    // how this animation needs to be used, use 'start' instead.
> +    return true;
> +}

We could move this to the header then, with the comment before the function, or even consider making scroll not pure virtual, I guess I assumed all animations would have to implement this when I added it.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:136
> +    m_position = position;

I wonder if we really need to save the current position, since we are not actually scrolling, and the initial position is always passed to start()

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:152
> +        m_position.x(), velocity.x());
> +    m_verticalData = PerAxisData(m_scrollableArea.minimumScrollPosition().y(),
> +        m_scrollableArea.maximumScrollPosition().y(),
> +        m_position.y(), velocity.y());

We could use initialPosition.x(), initialPosition.y() here

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:173
> +    m_position = FloatPoint(m_horizontalData.position(), m_verticalData.position());
> +
> +    m_notifyPositionChangedFunction(FloatPoint(m_position));

And pass the position directly to the callback

> Source/WebCore/platform/ScrollAnimationKinetic.h:57
> +        double m_lower { 0 };
> +        double m_upper { 0 };
> +
> +        double m_coef1 { 0 };
> +        double m_coef2 { 0 };
> +
> +        double m_elapsedTime { 0 };
> +        double m_position { 0 };
> +        double m_velocity { 0 };

We don't use m_ prefix for struct members.

> Source/WebCore/platform/ScrollAnimationKinetic.h:67
> +    bool scroll(ScrollbarOrientation, ScrollGranularity, float step, float multiplier) override;
> +    void stop() override;
> +    void updateVisibleLengths() override { }
> +    void setCurrentPosition(const FloatPoint&) override;

I don't think these should be public.

> Source/WebCore/platform/ScrollAnimationKinetic.h:77
> +    PerAxisData m_horizontalData { PerAxisData(0, 0, 0, 0) };
> +    PerAxisData m_verticalData { PerAxisData(0, 0, 0, 0) };

You don't need to initialize these, all PerAxisData members are initialized.

> Source/WebCore/platform/ScrollAnimationKinetic.h:81
> +

Extra line here.

> Source/WebCore/platform/graphics/FloatPoint.h:125
> +    bool isOrigin() const { return m_x == 0.0 && m_y == 0.0; }
> +

We don't compare to 0 in WebKit. I would leave this change to FloatPoint to a separate patch, since the rest of the patch is GTK+ specific, and just check x, y in the code. Also note that IntPoint has this methods as isZero(), so we should be consistent here.

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:91
> +    m_phase = event->is_stop ?
> +        PlatformWheelEventPhaseEnded :
> +        PlatformWheelEventPhaseChanged;

I don't understand this. When is it None then? Why is initially changed?

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:93
> +    m_phase = event->direction == GDK_SCROLL_SMOOTH && m_deltaX == 0.0 && m_deltaY = 0.0 ?

We don't compare to 0 in WebKit, see https://webkit.org/code-style-guidelines/#null-false-and-zero

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:98
> +    m_phase = PlatformWheelEventPhaseChanged;

Do we really need this? m_pahse is initialized in the header and will not be used in GTK+2 for sure.

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:101
> +    m_momentumPhase = PlatformWheelEventPhaseNone;

This is already None as initialized in the header.

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:116
> +    return isTransitioningToMomentumScroll() ? FloatPoint(m_wheelTicksX, m_wheelTicksY) : FloatPoint();

FloatPoint() -> { }

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:79
> +        updatePosition(FloatPoint(position));

updatePosition(WTFMove(position));

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:94
> +#if ENABLE(SMOOTH_SCROLLING)
>      if (!m_scrollableArea.scrollAnimatorEnabled() || granularity == ScrollByPrecisePixel)
>          return ScrollAnimator::scroll(orientation, granularity, step, multiplier);
>  
>      ensureSmoothScrollingAnimation();
> -    return m_animation->scroll(orientation, granularity, step, multiplier);
> +    return m_smoothAnimation->scroll(orientation, granularity, step, multiplier);
> +#else
> +    return ScrollAnimator::scroll(orientation, granularity, step, multiplier);
> +#endif

I think we could just keep this inside the #if ENABLE(SMOOTH_SCROLLING) as it was before, since you are not adding anything for non smooth scrolling.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:108
> +    updatePosition(FloatPoint(position));

updatePosition(WTFMove(position));

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:114
> +        return FloatPoint();

return { };

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:120
> +        return FloatPoint();

Ditto.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:128
> +    return FloatPoint(accumDelta.x() * -1000 / (last - first), accumDelta.y() * -1000 / (last - first));

And here you could also use { }

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:135
> +    m_scrollHistory.removeAllMatching([&] (PlatformWheelEvent& otherEvent) -> bool {

Capture explicitly event, instead of using &

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:140
> +        m_kineticAnimation->start(m_currentPosition, computeVelocity());

Don't we need to add the event to the history in this case too?

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:141
> +

Extra line here.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:147
> +

Ditto.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:34
> +#include "ScrollAnimationKinetic.h"

Why do you need to include this in the header?

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:48
> -#if ENABLE(SMOOTH_SCROLLING)
>      bool scroll(ScrollbarOrientation, ScrollGranularity, float step, float multiplier) override;

scroll is still specific to smooth scrolling

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:81
> +    std::unique_ptr<ScrollAnimationKinetic> m_kineticAnimation;

This should be ScrollAnimation not ScrollAnimationKinetic

> Source/WebKit2/Shared/NativeWebWheelEvent.h:55
> +    NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase, WebWheelEvent::Phase momentumPhase);

Maybe instead of adding a new constructor we could add the phase parameters with default values

> Source/WebKit2/Shared/WebEvent.h:218
> +#elif PLATFORM(GTK)
> +    Phase phase() const { return static_cast<Phase>(m_phase); }
> +    Phase momentumPhase() const { return static_cast<Phase>(m_momentumPhase); }
>  #endif

We should reorder things to share with coca, instead of duplicating.

> Source/WebKit2/Shared/WebEvent.h:241
> +#elif PLATFORM(GTK)
> +    uint32_t m_phase { Phase::PhaseNone }; // Phase
> +    uint32_t m_momentumPhase { Phase::PhaseNone }; // Phase
>  #endif

Ditto. Comments here // Phase don't really add anything, so I would remove them.

> Source/WebKit2/Shared/WebEventConversion.cpp:151
> +#if PLATFORM(GTK)
> +inline WebCore::PlatformWheelEventPhase& operator|=(WebCore::PlatformWheelEventPhase& self, WebCore::PlatformWheelEventPhase other)
> +{
> +    return self = static_cast<WebCore::PlatformWheelEventPhase>(self | other);
> +}
> +
> +static WebCore::PlatformWheelEventPhase toPlatformWheeleventPhase(WebWheelEvent::Phase phase)
> +{
> +    WebCore::PlatformWheelEventPhase result = WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseNone;
> +
> +    if (phase & WebWheelEvent::Phase::PhaseBegan)
> +        result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseBegan;
> +    if (phase & WebWheelEvent::Phase::PhaseStationary)
> +        result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseStationary;
> +    if (phase & WebWheelEvent::Phase::PhaseChanged)
> +        result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseChanged;
> +    if (phase & WebWheelEvent::Phase::PhaseEnded)
> +        result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseEnded;
> +    if (phase & WebWheelEvent::Phase::PhaseCancelled)
> +        result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseCancelled;
> +    if (phase & WebWheelEvent::Phase::PhaseMayBegin)
> +        result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseMayBegin;
> +
> +    return result;
> +}
> +#endif

I agree we shouldn't cast directly between WebKit and WebCore types, but this code should be shared with mac. Since they are casting, I suggest to share their current code for now to make this patch simpoler, and propose a new patch to do conversions instead of casts.

> Source/WebKit2/Shared/WebEventConversion.cpp:191
> +#elif PLATFORM(GTK)
> +        m_phase = toPlatformWheeleventPhase(webEvent.phase());
> +        m_momentumPhase = toPlatformWheeleventPhase(webEvent.momentumPhase());
>  #endif

And then we could reuse this code too, instead of duplicate.

> Source/WebKit2/Shared/WebWheelEvent.cpp:105
> +#elif PLATFORM(GTK)
> +    encoder << m_phase;
> +    encoder << m_momentumPhase;
>  #endif

Reorder things here too share the code with cocoa

> Source/WebKit2/Shared/WebWheelEvent.cpp:139
> +#elif PLATFORM(GTK)
> +    if (!decoder.decode(t.m_phase))
> +        return false;
> +    if (!decoder.decode(t.m_momentumPhase))
> +        return false;

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
> +WebWheelEvent WebEventFactory::createWebWheelEvent(const GdkEvent* event, WebWheelEvent::Phase phase, WebWheelEvent::Phase momentumPhase)

Here we could probably also keep a single method just with default values for the phases.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1819
> -        if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold)
> +        if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold && canSkipWheelEvent(event))

m_wheelEventQueue.size() < wheelEventQueueSizeThreshold is also a condition to skip wheel events. So, I would replace all this with just if (!shouldProcessWheelEventNow(event)) and move the condition to the function. Note that we are not actually skipping events, just queuing them to process them later when we have a batch. So, the main question is why we are doing this, and why this doesn't work for us in this case. Because maybe this is just a matter of using 0 as threshold for the GTK+ port, and we always process wheel events immediately.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1865
> +#if !PLATFORM(GTK)
> +bool WebPageProxy::canSkipWheelEvent(const WebWheelEvent&)
> +{
> +    return true;
> +}
> +#endif

Since we are adding platform ifdefs in cross-platform code in any case here, I would keep the function common, checking the event queue size for all platforms, and then adding an #ifdef for GTK+, something like:

bool WebPageProxy::shouldProcessWheelEventNow(const WebWheelEvent& event)
{
    if (m_wheelEventQueue.size() >= wheelEventQueueSizeThreshold)
        return true;
#if PLATFORM(GTK)
return ....
#endif
}

Another way to avoid platform ifdefs here would be to add a method to WebWheelEvent, something like shouldBeProcessesImmediately or something like that, with a default implementation returning false, and a GTK+ implementation. I still don't understand the whole thing to propose any of the solution, though. In any case, we should explain this in the changelog and add a comment in the code as well.

> Source/WebKit2/UIProcess/gtk/GestureController.cpp:80
> -void GestureController::DragGesture::handleDrag(const GdkEvent* event, double x, double y)
> +static GUniquePtr<GdkEvent> newScrollEvent(const GdkEvent* event, double x, double y, double deltaX, double deltaY, gboolean isStop)

use create instead of new

[Carlos Garcia Campos]

(In reply to comment #20)
> Created attachment 281711 [details]
> Patch - new features + bugfix
> 
> I've used this patch in an internal project and made some modifications that
> you may find useful. I am posting the updated patch without ChangeLogs, feel
> free to update it or let me know if you prefer if I do it.
> 
> The changes included in this patch (on top of latest one from Adrien):
> - overshoot animations support (ScrollAnimationKinetic.cpp)
> - bugfix for a crash when starting a drag while scrolling
> (GestureController.cpp)
> - build fix for gtk < 3.20 (PlatformWheelEventGtk.cpp)
> - bugfix for gtk < 3.20 to properly copy NativeWebWheelEvent (is_stop is not
> available in gtk < 3.20 and the phase/momentumPhase were not being properly
> copied on the NativeWebWheelEvent copy constructor, making the page "jump"
> during scroll instead of starting momentum scroll -
> NativeWebWheelEventGtk.cpp)
> - respect scrollbar policy (ScrollAnimationKinetic.cpp +
> ScrollAnimatorGtk.cpp)
> 
> That should be it, please let me know what you think.

Thanks! I think Adrien should include crashes and build fixes in his patch when submitting a new version and you should open a new bug depending on this one to add the overshooting support on top of Adrien's patch.

[Andre Moreira Magalhaes]

(In reply to comment #22)
> (In reply to comment #20)
> > Created attachment 281711 [details]
> > Patch - new features + bugfix
> > 
> > I've used this patch in an internal project and made some modifications that
> > you may find useful. I am posting the updated patch without ChangeLogs, feel
> > free to update it or let me know if you prefer if I do it.
> > 
> > The changes included in this patch (on top of latest one from Adrien):
> > - overshoot animations support (ScrollAnimationKinetic.cpp)
> > - bugfix for a crash when starting a drag while scrolling
> > (GestureController.cpp)
> > - build fix for gtk < 3.20 (PlatformWheelEventGtk.cpp)
> > - bugfix for gtk < 3.20 to properly copy NativeWebWheelEvent (is_stop is not
> > available in gtk < 3.20 and the phase/momentumPhase were not being properly
> > copied on the NativeWebWheelEvent copy constructor, making the page "jump"
> > during scroll instead of starting momentum scroll -
> > NativeWebWheelEventGtk.cpp)
> > - respect scrollbar policy (ScrollAnimationKinetic.cpp +
> > ScrollAnimatorGtk.cpp)
> > 
> > That should be it, please let me know what you think.
> 
> Thanks! I think Adrien should include crashes and build fixes in his patch
> when submitting a new version and you should open a new bug depending on
> this one to add the overshooting support on top of Adrien's patch.

Yes, I talked to Adrien on IRC and I will file a new bug with the overshooting support once he has an updated/final patch here.
He will check about including the other fixes here in this patch.

[Andre Moreira Magalhaes]

Comment on attachment 281354 [details]
[GTK] Add kinetic scrolling

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

>> Source/WebKit2/Shared/NativeWebWheelEvent.h:55
>> +    NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase, WebWheelEvent::Phase momentumPhase);
> 
> Maybe instead of adding a new constructor we could add the phase parameters with default values

I am not sure this would work as we need to pass the phase/momentumPhase when creating the events from GestureController. I even added a fix in my patch to make this work fine with gtk<3.20 (is_stop issue when copying).

>> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
>> +WebWheelEvent WebEventFactory::createWebWheelEvent(const GdkEvent* event, WebWheelEvent::Phase phase, WebWheelEvent::Phase momentumPhase)
> 
> Here we could probably also keep a single method just with default values for the phases.

Ditto.

[Carlos Garcia Campos]

(In reply to comment #24)
> Comment on attachment 281354 [details]
> [GTK] Add kinetic scrolling
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281354&action=review
> 
> >> Source/WebKit2/Shared/NativeWebWheelEvent.h:55
> >> +    NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase, WebWheelEvent::Phase momentumPhase);
> > 
> > Maybe instead of adding a new constructor we could add the phase parameters with default values
> 
> I am not sure this would work as we need to pass the phase/momentumPhase
> when creating the events from GestureController. I even added a fix in my
> patch to make this work fine with gtk<3.20 (is_stop issue when copying).

And what is the problem? I'm proposing to have a single constructor with the parameters having default values, but you can still call it with other values

> >> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
> >> +WebWheelEvent WebEventFactory::createWebWheelEvent(const GdkEvent* event, WebWheelEvent::Phase phase, WebWheelEvent::Phase momentumPhase)
> > 
> > Here we could probably also keep a single method just with default values for the phases.
> 
> Ditto.

[Andre Moreira Magalhaes]

Comment on attachment 281354 [details]
[GTK] Add kinetic scrolling

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

>>>> Source/WebKit2/Shared/NativeWebWheelEvent.h:55
>>>> +    NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase, WebWheelEvent::Phase momentumPhase);
>>> 
>>> Maybe instead of adding a new constructor we could add the phase parameters with default values
>> 
>> I am not sure this would work as we need to pass the phase/momentumPhase when creating the events from GestureController. I even added a fix in my patch to make this work fine with gtk<3.20 (is_stop issue when copying).
> 
> And what is the problem? I'm proposing to have a single constructor with the parameters having default values, but you can still call it with other values

Sorry, I misread you, I thought you were proposing to remove this constructor and just use default values :D.

[Adrien Plazas]

Comment on attachment 281354 [details]
[GTK] Add kinetic scrolling

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

>> Source/WebCore/platform/ScrollAnimationKinetic.cpp:126
>> +}
> 
> We could move this to the header then, with the comment before the function, or even consider making scroll not pure virtual, I guess I assumed all animations would have to implement this when I added it.

I moved it to ScrollAnimation.h and made the method non-pure virtual.

>> Source/WebCore/platform/ScrollAnimationKinetic.cpp:136
>> +    m_position = position;
> 
> I wonder if we really need to save the current position, since we are not actually scrolling, and the initial position is always passed to start()

I completely removed m_position,  made the method a non-pure virtual one doing nothing in ScrollAnimation.h.

I jut call stop() instead now.

>> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:91
>> +        PlatformWheelEventPhaseChanged;
> 
> I don't understand this. When is it None then? Why is initially changed?

m_phase represent the phase of the regular scrolling, m_momentumPhase represent the phase of the momentum scrolling (it's the same in the Cocoa code). m_phase is None when we are not in a regular scrolling phase anymore (for example when we are in a momentum/inertial scrolling phase).

The event we are handling at this point can either represent a regular scrolling event in the scrolling phase (Changed) or it can represent the end of the regular scrolling session (Ended).

These info are then used by methods like isEndOfNonMomentumScroll() and isTransitioningToMomentumScroll() to determine the overall state of the scrolling.

>> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:98
>> +    m_phase = PlatformWheelEventPhaseChanged;
> 
> Do we really need this? m_pahse is initialized in the header and will not be used in GTK+2 for sure.

I think we decided with mcatanzaro to add this GTK_API_VERSION_2 check, I don't remember what was the reason though.

If you are suggesting to set the default to "Changed" I'm not sure it's a saner default than "None", especially as we share this initialization with Cocoa.

>> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:116
>> +    return isTransitioningToMomentumScroll() ? FloatPoint(m_wheelTicksX, m_wheelTicksY) : FloatPoint();
> 
> FloatPoint() -> { }

The compiler doesn't like it: error: initializer list cannot be used on the right hand side of operator '?'

>> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:128
>> +    return FloatPoint(accumDelta.x() * -1000 / (last - first), accumDelta.y() * -1000 / (last - first));
> 
> And here you could also use { }

The compiler doesn't like this, I have two times this error, one per parameter of the initializer list:
error: non-constant-expression cannot be narrowed from type 'double' to 'float' in initializer list [-Wc++11-narrowing]

>> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:140
>> +        m_kineticAnimation->start(m_currentPosition, computeVelocity());
> 
> Don't we need to add the event to the history in this case too?

No because these events' delta are by definition in GTK+ always (0, 0). I added a comment.

>> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:34
>> +#include "ScrollAnimationKinetic.h"
> 
> Why do you need to include this in the header?

I explicitly used ScrollAnimationKinetic in the header, I replaced it by ScrollAnimation as you prefer and hence removed this line.

>> Source/WebKit2/Shared/NativeWebWheelEvent.h:55
>> +    NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase, WebWheelEvent::Phase momentumPhase);
> 
> Maybe instead of adding a new constructor we could add the phase parameters with default values

The only way I see to implement it would be to set the defaults of phase and momentumPhase to None, to add the code of NativeWebWheelEvent(GdkEvent*) at the beggining of NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase, WebWheelEvent::Phase momentumPhase) and to execute it only if the phase and momentumPhase are both None.

It doesn't look very clean to me as an explicit call with both phases set to None would end up having phases guessed from the event, breaking the semantic of the explicit value pasing. It would also make NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase) exist (with momentumPhase being None) which we probably don't want.

We could also remove the NativeWebWheelEvent(GdkEvent*) construtor and copy its code to each of its call points but it doesn't sound really clean to me either (it looks like it's only called in gboolean webkitWebViewBaseScrollEvent(GtkWidget* widget, GdkEventScroll* event) but I may have searched wrongly).

Do you have better suggestions or is one of these good for reasons I didn't think about?

>> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
>> +WebWheelEvent WebEventFactory::createWebWheelEvent(const GdkEvent* event, WebWheelEvent::Phase phase, WebWheelEvent::Phase momentumPhase)
> 
> Here we could probably also keep a single method just with default values for the phases.

I'm not sure how to do it cleanly (see a previous comment on NativeWebWheelEvent.h), would you mind explaining me how to do so?

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1819
>> +        if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold && canSkipWheelEvent(event))
> 
> m_wheelEventQueue.size() < wheelEventQueueSizeThreshold is also a condition to skip wheel events. So, I would replace all this with just if (!shouldProcessWheelEventNow(event)) and move the condition to the function. Note that we are not actually skipping events, just queuing them to process them later when we have a batch. So, the main question is why we are doing this, and why this doesn't work for us in this case. Because maybe this is just a matter of using 0 as threshold for the GTK+ port, and we always process wheel events immediately.

I think it was a bit more sluggish when directly processing all the events but I have no data to prove it.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1865
>> +#endif
> 
> Since we are adding platform ifdefs in cross-platform code in any case here, I would keep the function common, checking the event queue size for all platforms, and then adding an #ifdef for GTK+, something like:
> 
> bool WebPageProxy::shouldProcessWheelEventNow(const WebWheelEvent& event)
> {
>     if (m_wheelEventQueue.size() >= wheelEventQueueSizeThreshold)
>         return true;
> #if PLATFORM(GTK)
> return ....
> #endif
> }
> 
> Another way to avoid platform ifdefs here would be to add a method to WebWheelEvent, something like shouldBeProcessesImmediately or something like that, with a default implementation returning false, and a GTK+ implementation. I still don't understand the whole thing to propose any of the solution, though. In any case, we should explain this in the changelog and add a comment in the code as well.

IIRC the goal was to avoid having events representing the end of a regular scrolling session, the beginning of a momentum scrolling one or the end of a momentum scrolling one to stay trapped in the queue if no further event come to fill it up, triggering its processing. These trapped events can prevent a momentum scrolling session to start or end correctly.

It would probably be easier/safer to check that no event is in a None or Changed phase.

[Adrien Plazas]

Created attachment 281962 [details]
[GTK] Add kinetic scrolling

Potentially missing bits:
- using the "Optional<PerAxisData>" from Andre's patch
- improving the NativeWebWheelEvent construction

[WebKit Commit Bot]

Attachment 281962 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ScrollAnimationKinetic.cpp:135:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ScrollAnimationKinetic.cpp:139:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrien Plazas]

Created attachment 281975 [details]
[GTK] Add kinetic scrolling

[Andre Moreira Magalhaes]

Comment on attachment 281962 [details]
[GTK] Add kinetic scrolling

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

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:161
> +    m_notifyPositionChangedFunction(FloatPoint(m_horizontalData.position(), m_verticalData.position()));

Here you are using the horizonte/vertical data position, but on ::start you only set them if mayV/HScroll are true, meaning you may be reusing the data from another scrolling event (you only initialize the PerAxisData on the constructor). Also, you want to use the original position (on notifyPositionChanged) for a given direction (x/y) if that doesnt change (mayV/HScroll=false) during animation and most probably not invoke animateScroll on that direction.

[Adrien Plazas]

Created attachment 281977 [details]
[GTK] Add kinetic scrolling

Potentially missing bits:
- improving the NativeWebWheelEvent construction

[Andre Moreira Magalhaes]

Changes look good to me. See also bug 158531, which may affect changes here in the GestureController::handleEvent if it gets in before this one.

[Carlos Garcia Campos]

Comment on attachment 281977 [details]
[GTK] Add kinetic scrolling

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

Looks good to me, thanks! We just need a wk2 owner to approve the changes in WebPageProxy

> Source/WebCore/platform/ScrollAnimationKinetic.h:74
> +    Optional<PerAxisData> m_horizontalData { Nullopt };
> +    Optional<PerAxisData> m_verticalData { Nullopt };

You don't need to initialize these, the constructor already sets is to Nullopt.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:60
> +        updatePosition(WTFMove(position));
> +#if ENABLE(SMOOTH_SCROLLING)
> +        if (m_smoothAnimation)
> +            m_smoothAnimation->setCurrentPosition(position);
> +#endif

hmm, this is a problem now that I see it, this is using position after being moved. You can probably just change the order, and call updatePosition after setCurrentPosition

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:107
> +#if ENABLE(SMOOTH_SCROLLING)
> +    if (m_smoothAnimation)
> +        m_smoothAnimation->setCurrentPosition(position);
> +#endif
> +
> +    updatePosition(WTFMove(position));

like you do here

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1893
> +bool WebPageProxy::shouldProcessWheelEventNow(const WebWheelEvent& event)
> +{
> +#if PLATFORM(GTK)
> +    // Don't queue events representing a non-trivial scrolling phase to
> +    // avoid having them trapped in the queue, potentially preventing a
> +    // scrolling session to beginning or end correctly.
> +    // This is only needed by platforms whose WebWheelEvent has this phase
> +    // information (Cocoa and GTK+) but Cocoa was fine without it.
> +    if (event.phase() == WebWheelEvent::Phase::PhaseNone
> +        || event.phase() == WebWheelEvent::Phase::PhaseChanged
> +        || event.momentumPhase() == WebWheelEvent::Phase::PhaseNone
> +        || event.momentumPhase() == WebWheelEvent::Phase::PhaseChanged)
> +        return true;
> +#else
> +    UNUSED_PARAM(event);
> +#endif
> +    if (m_wheelEventQueue.size() >= wheelEventQueueSizeThreshold)
> +        return true;
> +    return false;
> +}

This is in cross-platform code, we need approval from a WebKit2 owner

> Source/WebKit2/UIProcess/WebPageProxy.h:1469
> +    bool shouldProcessWheelEventNow(const WebWheelEvent&);

This could probably be const

[Carlos Garcia Campos]

Adding wk2 owners to the CC

[Adrien Plazas]

Created attachment 282126 [details]
[GTK] Add kinetic scrolling

Fixed all the points noted by Carlos in the last comments.

[Adrien Plazas]

Created attachment 282127 [details]
[GTK] Add kinetic scrolling

Add files missing in the last patch.

[WebKit Commit Bot]

Attachment 282127 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrien Plazas]

Created attachment 282129 [details]
[GTK] Add kinetic scrolling

Bis.

[Carlos Garcia Campos]

Comment on attachment 282129 [details]
[GTK] Add kinetic scrolling

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

> Source/WebCore/platform/ScrollAnimationKinetic.h:74
> +    Optional<PerAxisData> m_horizontalData { Nullopt };
> +    Optional<PerAxisData> m_verticalData { Nullopt };

You don't need to initialize these, the constructor already sets is to Nullopt.

[Carlos Garcia Campos]

Comment on attachment 281711 [details]
Patch - new features + bugfix

Let's handle new stuff in a different bug, please.

[Adrien Plazas]

Created attachment 282136 [details]
[GTK] Add kinetic scrolling

[Adrien Plazas]

Created attachment 282137 [details]
[GTK] Add kinetic scrolling

[Andre Moreira Magalhaes]

(In reply to comment #41)
> Comment on attachment 281711 [details]
> Patch - new features + bugfix
> 
> Let's handle new stuff in a different bug, please.

Yep, Adrien already applied the bugfixes on his patch and I will open a new bug for the overshoot support once this one is merged.

[Adrien Plazas]

(In reply to comment #44)
> (In reply to comment #41)
> > Comment on attachment 281711 [details]
> > Patch - new features + bugfix
> > 
> > Let's handle new stuff in a different bug, please.
> 
> Yep, Adrien already applied the bugfixes on his patch and I will open a new
> bug for the overshoot support once this one is merged.

I opened a bug depending on this one to properly convert the wheel event phases rather than casting them at you suggested to do so Carlos: https://bugs.webkit.org/show_bug.cgi?id=159150

[Carlos Garcia Campos]

Comment on attachment 282137 [details]
[GTK] Add kinetic scrolling

This looks good to me. Please, WebKit2 owners, if cross-platform changes (WebPageProxy) look good to you, just cq+ the patch.

[Michael Catanzaro]

Looks like this got forgotten. Pinging WK2 owners!

[Michael Catanzaro]

Six months is way more than enough time for owners to object, so I think we can safely cq+ now, but it's also way more than enough time for this to bitrot. Real shame this did not land and we seemingly forgot about it. Adrien, any chance you're interested in updating the patch...?

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #48)
> Six months is way more than enough time for owners to object, so I think we
> can safely cq+ now, but it's also way more than enough time for this to
> bitrot. Real shame this did not land and we seemingly forgot about it.
> Adrien, any chance you're interested in updating the patch...?

How is the situation of this patch?

[Michael Catanzaro]

We need to rebase it on trunk.

[Yusuke Suzuki]

Created attachment 312397 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 312397 [details]
Patch

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

I've just rebased this patch.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:68
> +static const double decelFriction = 4;
> +static const double frameRate = 60;
> +static const Seconds tickTime = 1_s / frameRate;
> +static const Seconds minimumTimerInterval { 1_ms };

Modernized. Using Seconds.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:89
> +bool ScrollAnimationKinetic::PerAxisData::animateScroll(Seconds timeDelta)

Using Seconds.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:149
> +    m_startTime = MonotonicTime::now() - tickTime / 2.;

One of my change is this. It looks that the previous patch does not set m_startTime in any place.

> Source/WebCore/platform/ScrollAnimationKinetic.h:73
> +    std::optional<PerAxisData> m_horizontalData;
> +    std::optional<PerAxisData> m_verticalData;

Use std::optional instead of Optional.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:44
> +static const Seconds scrollCaptureThreshold { 150_ms };

Modernized.

[Yusuke Suzuki]

Created attachment 312405 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 312405 [details]
Patch

Thanks, I had forgotten this.

[Yusuke Suzuki]

Committed r217971: <http://trac.webkit.org/changeset/217971>