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

Thu Jun 23 01:10:43 PDT 2016

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

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

-- 
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/20160623/116ca195/attachment-0001.html>