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

[bugzilla-daemon at webkit.org]

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

--- Comment #12 from Adrien Plazas <aplazas at igalia.com> ---
Comment on attachment 277468
  --> https://bugs.webkit.org/attachment.cgi?id=277468
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!

-- 
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/20160427/6bb04390/attachment.html>