Bug 209230 – [GTK][WPE] Enable kinetic scrolling with async scrolling

• Component: Compositing
• Status: RESOLVED FIXED

[Chris Lord]

Currently, there is kinetic scrolling code used in the non-async rendering path in WebKitGtk, but enabling async rendering will mean that code is bypassed. It would be good to enable kinetic scrolling in this situation and share the existing code to avoid duplication. This would also allow kinetic scrolling in WPE (which is the real goal).

[Chris Lord]

Created attachment 393862 [details]
Patch

[Chris Lord]

I think the 2 style errors are in keeping with the rest of the file and the immediate surrounds of the two lines too, but let me know if there's something I can do to fix that.

[Carlos Garcia Campos]

Comment on attachment 393862 [details]
Patch

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

> Source/WebCore/ChangeLog:38
> +

I miss an explanation of changes made to several of these files. I don't have time for detailed review, sorry, but I can add a few comments.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:189
> +void ScrollingTreeScrollingNode::stopScrollAnimations()
> +{
> +}

This empty impl can be moved to the header.

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:39
> +#if ENABLE(KINETIC_SCROLLING)
> +#include "ScrollAnimationKinetic.h"
> +#endif

For ifdefed headers we don't follow the alphabetic order, move this after the unconditional includes block,

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:127
> -        scrollBy({ wheelEvent.deltaX(), -wheelEvent.deltaY() });
> +        scrollBy({ -wheelEvent.deltaX(), -wheelEvent.deltaY() });

This is bug #208638, but Zan told me that breaks touch scrolling, I don't know why.

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.h:44
> +#if ENABLE(KINETIC_SCROLLING)
> +class ScrollAnimationKinetic;
> +#endif

I don't think we need ifdefs for forward declarations.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:109
> +    : m_scrollExtentsFunction(scrollExtentsFunction)

WTFMove(scrollExtentsFunction)

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:113
>  {
>  }

Now that animation timer is a RunLoop::Timer we should give it a priority.

> Source/WebCore/platform/ScrollAnimationKinetic.h:68
> +using ScrollExtentsCallback = std::function<ScrollExtents(void)>;
> +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;

Is this correctly indented? I think we can use Function instead of std::function to make sure they aren't copyable.

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
> -            m_axisEvent.x_axis = m_offset.x - touchPoint->x;
> +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);

This might explain why touch scrolling was broken with bug #208638

[Chris Lord]

Thanks for the mini-review Carlos :)

(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 393862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393862&action=review
> 
> > Source/WebCore/ChangeLog:38
> > +
> 
> I miss an explanation of changes made to several of these files. I don't
> have time for detailed review, sorry, but I can add a few comments.

My bad, I should have put more detail here - will do so.

> > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:189
> > +void ScrollingTreeScrollingNode::stopScrollAnimations()
> > +{
> > +}
> 
> This empty impl can be moved to the header.

If I move it to the header, I get a style warning (although I already get a style warning for the other two functions added and the file already seems to ignore this particular style requirement, so maybe I should here too)

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:39
> > +#if ENABLE(KINETIC_SCROLLING)
> > +#include "ScrollAnimationKinetic.h"
> > +#endif
> 
> For ifdefed headers we don't follow the alphabetic order, move this after
> the unconditional includes block,

Okidokes.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:127
> > -        scrollBy({ wheelEvent.deltaX(), -wheelEvent.deltaY() });
> > +        scrollBy({ -wheelEvent.deltaX(), -wheelEvent.deltaY() });
> 
> This is bug #208638, but Zan told me that breaks touch scrolling, I don't
> know why.

Indeed, the change you mention below is why.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.h:44
> > +#if ENABLE(KINETIC_SCROLLING)
> > +class ScrollAnimationKinetic;
> > +#endif
> 
> I don't think we need ifdefs for forward declarations.

Okidokes

> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:109
> > +    : m_scrollExtentsFunction(scrollExtentsFunction)
> 
> WTFMove(scrollExtentsFunction)

Good spot.

> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:113
> >  {
> >  }
> 
> Now that animation timer is a RunLoop::Timer we should give it a priority.

Any suggestions about this? Every other similar use I could find via grep in the codebase doesn't set any kind of priority.

> > Source/WebCore/platform/ScrollAnimationKinetic.h:68
> > +using ScrollExtentsCallback = std::function<ScrollExtents(void)>;
> > +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;
> 
> Is this correctly indented? I think we can use Function instead of
> std::function to make sure they aren't copyable.

I believe so. At least, I don't get any warnings and it matches formatting in several other files. I was following what was there before regarding use of std::function, but can change both to WTF::Function.

> > Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
> > -            m_axisEvent.x_axis = m_offset.x - touchPoint->x;
> > +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);
> 
> This might explain why touch scrolling was broken with bug #208638

Yes, this is the cause after applying the above fix - and judging on other similar event handling code, I believe this is the right fix for it.

[Carlos Garcia Campos]

(In reply to Chris Lord from comment #4)
> Thanks for the mini-review Carlos :)
> 
> (In reply to Carlos Garcia Campos from comment #3)
> > Comment on attachment 393862 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=393862&action=review
> > 
> > > Source/WebCore/ChangeLog:38
> > > +
> > 
> > I miss an explanation of changes made to several of these files. I don't
> > have time for detailed review, sorry, but I can add a few comments.
> 
> My bad, I should have put more detail here - will do so.

Thanks. I see now that ScrollAnimator is not available in the case of async scrolling. That needs to be explained in the changelog, please.

> > > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:189
> > > +void ScrollingTreeScrollingNode::stopScrollAnimations()
> > > +{
> > > +}
> > 
> > This empty impl can be moved to the header.
> 
> If I move it to the header, I get a style warning (although I already get a
> style warning for the other two functions added and the file already seems
> to ignore this particular style requirement, so maybe I should here too)

What warning exactly?

> > > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:39
> > > +#if ENABLE(KINETIC_SCROLLING)
> > > +#include "ScrollAnimationKinetic.h"
> > > +#endif
> > 
> > For ifdefed headers we don't follow the alphabetic order, move this after
> > the unconditional includes block,
> 
> Okidokes.
> 
> > > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:127
> > > -        scrollBy({ wheelEvent.deltaX(), -wheelEvent.deltaY() });
> > > +        scrollBy({ -wheelEvent.deltaX(), -wheelEvent.deltaY() });
> > 
> > This is bug #208638, but Zan told me that breaks touch scrolling, I don't
> > know why.
> 
> Indeed, the change you mention below is why.
> 
> > > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.h:44
> > > +#if ENABLE(KINETIC_SCROLLING)
> > > +class ScrollAnimationKinetic;
> > > +#endif
> > 
> > I don't think we need ifdefs for forward declarations.
> 
> Okidokes
> 
> > > Source/WebCore/platform/ScrollAnimationKinetic.cpp:109
> > > +    : m_scrollExtentsFunction(scrollExtentsFunction)
> > 
> > WTFMove(scrollExtentsFunction)
> 
> Good spot.
> 
> > > Source/WebCore/platform/ScrollAnimationKinetic.cpp:113
> > >  {
> > >  }
> > 
> > Now that animation timer is a RunLoop::Timer we should give it a priority.
> 
> Any suggestions about this? Every other similar use I could find via grep in
> the codebase doesn't set any kind of priority.

Include <wtf/glib/RunLoopSourcePriority.h> and then simply m_animationTimer.setPriority(). I think we can use LayerFlushTimer or add a new priority for this (even if it has the same value).

> > > Source/WebCore/platform/ScrollAnimationKinetic.h:68
> > > +using ScrollExtentsCallback = std::function<ScrollExtents(void)>;
> > > +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;
> > 
> > Is this correctly indented? I think we can use Function instead of
> > std::function to make sure they aren't copyable.
> 
> I believe so. At least, I don't get any warnings and it matches formatting
> in several other files. I was following what was there before regarding use
> of std::function, but can change both to WTF::Function.

This code was added before WTF::Function existed.

> > > Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
> > > -            m_axisEvent.x_axis = m_offset.x - touchPoint->x;
> > > +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);
> > 
> > This might explain why touch scrolling was broken with bug #208638
> 
> Yes, this is the cause after applying the above fix - and judging on other
> similar event handling code, I believe this is the right fix for it.

Let's move this to the other patch then and land it before this one.

[Zan Dobersek]

Comment on attachment 393862 [details]
Patch

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

>>> Source/WebCore/platform/ScrollAnimationKinetic.h:68
>>> +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;
>> 
>> Is this correctly indented? I think we can use Function instead of std::function to make sure they aren't copyable.
> 
> I believe so. At least, I don't get any warnings and it matches formatting in several other files. I was following what was there before regarding use of std::function, but can change both to WTF::Function.

These type aliases have to be indented like the rest of the declarations inside this class.

> Source/WebKit/Shared/libwpe/WebEventFactory.cpp:182
> +    if (event->type == wpe_input_touch_event_type_null) {
> +        return WebWheelEvent(WebEvent::Wheel, position, position,
> +            delta, wheelTicks, phase, momentumPhase,
> +            WebWheelEvent::ScrollByPixelWheelEvent,
> +            OptionSet<WebEvent::Modifier> { }, wallTimeForEventTime(event->time));
> +    }

From where are touch events coming into `createWebWheelEvent()`? Why are null events expected to be handled here?

>>> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
>>> +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);
>> 
>> This might explain why touch scrolling was broken with bug #208638
> 
> Yes, this is the cause after applying the above fix - and judging on other similar event handling code, I believe this is the right fix for it.

This breaks horizontal touch scrolling.

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:178
> -                    struct wpe_input_axis_event* axisEvent = scrollGestureController.axisEvent();
> -                    if (axisEvent->type != wpe_input_axis_event_type_null)
> -                        page.handleWheelEvent(WebKit::NativeWebWheelEvent(axisEvent, page.deviceScaleFactor()));
> +                    page.handleWheelEvent(WebKit::NativeWebWheelEvent(scrollGestureController.axisEvent(), page.deviceScaleFactor(), scrollGestureController.phase(), WebWheelEvent::Phase::PhaseNone));

Why are null events being handled?

[Chris Lord]

(In reply to Zan Dobersek from comment #6)
> Comment on attachment 393862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393862&action=review
> 
> >>> Source/WebCore/platform/ScrollAnimationKinetic.h:68
> >>> +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;
> >> 
> >> Is this correctly indented? I think we can use Function instead of std::function to make sure they aren't copyable.
> > 
> > I believe so. At least, I don't get any warnings and it matches formatting in several other files. I was following what was there before regarding use of std::function, but can change both to WTF::Function.
> 
> These type aliases have to be indented like the rest of the declarations
> inside this class.

Will change.

> > Source/WebKit/Shared/libwpe/WebEventFactory.cpp:182
> > +    if (event->type == wpe_input_touch_event_type_null) {
> > +        return WebWheelEvent(WebEvent::Wheel, position, position,
> > +            delta, wheelTicks, phase, momentumPhase,
> > +            WebWheelEvent::ScrollByPixelWheelEvent,
> > +            OptionSet<WebEvent::Modifier> { }, wallTimeForEventTime(event->time));
> > +    }
> 
> From where are touch events coming into `createWebWheelEvent()`? Why are
> null events expected to be handled here?

The scroll gesture controller synthesises a null axis event on release - I guess we could give it any type we liked, but null made sense to me.

> >>> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
> >>> +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);
> >> 
> >> This might explain why touch scrolling was broken with bug #208638
> > 
> > Yes, this is the cause after applying the above fix - and judging on other similar event handling code, I believe this is the right fix for it.
> 
> This breaks horizontal touch scrolling.

Seems fine with the corresponding changes in handleWheelEvent to the scrollBy calls. I certainly don't notice anything wrong when testing it.

> > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:178
> > -                    struct wpe_input_axis_event* axisEvent = scrollGestureController.axisEvent();
> > -                    if (axisEvent->type != wpe_input_axis_event_type_null)
> > -                        page.handleWheelEvent(WebKit::NativeWebWheelEvent(axisEvent, page.deviceScaleFactor()));
> > +                    page.handleWheelEvent(WebKit::NativeWebWheelEvent(scrollGestureController.axisEvent(), page.deviceScaleFactor(), scrollGestureController.phase(), WebWheelEvent::Phase::PhaseNone));
> 
> Why are null events being handled?

(see above) - in short, we need to handle release events. I can change this to be axis events instead of null.

[Zan Dobersek]

Comment on attachment 393862 [details]
Patch

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

>>> Source/WebKit/Shared/libwpe/WebEventFactory.cpp:182
>>> +    }
>> 
>> From where are touch events coming into `createWebWheelEvent()`? Why are null events expected to be handled here?
> 
> The scroll gesture controller synthesises a null axis event on release - I guess we could give it any type we liked, but null made sense to me.

If needed, null is fine, but the test should be using wpe_input_axis_event_type_null, not the touch equivalent (even if it has the same 0 value).

[Chris Lord]

Created attachment 394478 [details]
Patch

[Zan Dobersek]

Comment on attachment 394478 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:146
> +    auto first = m_scrollHistory[0].timestamp();
> +    auto last = m_scrollHistory.rbegin()->timestamp();

These Vector lookups should use first() and last().

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:149
> +    if (last == first)
> +        return { };

Furthermore, the `last - first` difference can be computed early, tested here to be above-zero, and then used in the return statement.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:157
> +    return FloatPoint(accumDelta.x() * -1 / (last - first).value(), accumDelta.y() * -1 / (last - first).value());

The -1 can be incorporated as a sign into the accumDelta value lookup.

Given this is all existing code being moved around, you could take this polishing up in a different patch.

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:92
>  #if WPE_CHECK_VERSION(1, 5, 0)
>              m_axisEvent.base = {
> -                wpe_input_axis_event_type_null,
> -                0, 0, 0, 0, 0, 0
> +                m_axisEvent.base.type,
> +                touchPoint->time, m_start.x, m_start.y,
> +                0, 0, 0
>              };
>              m_axisEvent.x_axis = m_axisEvent.y_axis = 0;
>  #else
>              m_axisEvent = {
> -                wpe_input_axis_event_type_null,
> -                0, 0, 0, 0, 0, 0
> +                wpe_input_axis_event_type_motion,
> +                touchPoint->time, m_start.x, m_start.y,
> +                0, 0, 0
>              };
>  #endif

These two changes don't seem aligned. The first one is maintaining the event type, the second one is overriding it.

[Chris Lord]

(In reply to Zan Dobersek from comment #10)
> Comment on attachment 394478 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394478&action=review
> 
> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:146
> > +    auto first = m_scrollHistory[0].timestamp();
> > +    auto last = m_scrollHistory.rbegin()->timestamp();
> 
> These Vector lookups should use first() and last().
> 
> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:149
> > +    if (last == first)
> > +        return { };
> 
> Furthermore, the `last - first` difference can be computed early, tested
> here to be above-zero, and then used in the return statement.
> 
> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:157
> > +    return FloatPoint(accumDelta.x() * -1 / (last - first).value(), accumDelta.y() * -1 / (last - first).value());
> 
> The -1 can be incorporated as a sign into the accumDelta value lookup.
> 
> Given this is all existing code being moved around, you could take this
> polishing up in a different patch.

Yes, a separate patch I think - I need to look at Gtk, if this is essentially copy-pasted from there, I'd like to avoid changing it in any way that makes it read differently (I expect the first/last bits are fine though).

> > Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:92
> >  #if WPE_CHECK_VERSION(1, 5, 0)
> >              m_axisEvent.base = {
> > -                wpe_input_axis_event_type_null,
> > -                0, 0, 0, 0, 0, 0
> > +                m_axisEvent.base.type,
> > +                touchPoint->time, m_start.x, m_start.y,
> > +                0, 0, 0
> >              };
> >              m_axisEvent.x_axis = m_axisEvent.y_axis = 0;
> >  #else
> >              m_axisEvent = {
> > -                wpe_input_axis_event_type_null,
> > -                0, 0, 0, 0, 0, 0
> > +                wpe_input_axis_event_type_motion,
> > +                touchPoint->time, m_start.x, m_start.y,
> > +                0, 0, 0
> >              };
> >  #endif
> 
> These two changes don't seem aligned. The first one is maintaining the event
> type, the second one is overriding it.

In the second case, the type can only ever be type_motion, where as there are two types in >=1.5 - you're right that this reads confusingly though, so I'll just do the same in both blocks.

[Chris Lord]

Created attachment 394712 [details]
Patch

[Chris Lord]

Comment on attachment 394712 [details]
Patch

Already reviewed.

[EWS]

Committed r259112: <https://trac.webkit.org/changeset/259112>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394712 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60964765>