Bug 209230 - [GTK][WPE] Enable kinetic scrolling with async scrolling
Summary: [GTK][WPE] Enable kinetic scrolling with async scrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on: 208638
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-18 07:50 PDT by Chris Lord
Modified: 2020-03-27 05:55 PDT (History)
12 users (show)

See Also:


Attachments
Patch (40.65 KB, patch)
2020-03-18 08:59 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (39.99 KB, patch)
2020-03-25 03:04 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (39.98 KB, patch)
2020-03-27 02:01 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2020-03-18 07:50:44 PDT
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).
Comment 1 Chris Lord 2020-03-18 08:59:06 PDT
Created attachment 393862 [details]
Patch
Comment 2 Chris Lord 2020-03-18 09:00:49 PDT
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.
Comment 3 Carlos Garcia Campos 2020-03-19 05:44:12 PDT
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
Comment 4 Chris Lord 2020-03-19 05:54:56 PDT
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.
Comment 5 Carlos Garcia Campos 2020-03-19 06:52:08 PDT
(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.
Comment 6 Zan Dobersek 2020-03-19 07:02:34 PDT
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?
Comment 7 Chris Lord 2020-03-19 07:11:04 PDT
(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.
Comment 8 Zan Dobersek 2020-03-19 09:09:08 PDT
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).
Comment 9 Chris Lord 2020-03-25 03:04:47 PDT
Created attachment 394478 [details]
Patch
Comment 10 Zan Dobersek 2020-03-26 08:49:54 PDT
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.
Comment 11 Chris Lord 2020-03-27 01:48:26 PDT
(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.
Comment 12 Chris Lord 2020-03-27 02:01:39 PDT
Created attachment 394712 [details]
Patch
Comment 13 Chris Lord 2020-03-27 02:08:09 PDT
Comment on attachment 394712 [details]
Patch

Already reviewed.
Comment 14 EWS 2020-03-27 05:54:43 PDT
Committed r259112: <https://trac.webkit.org/changeset/259112>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394712 [details].
Comment 15 Radar WebKit Bug Importer 2020-03-27 05:55:14 PDT
<rdar://problem/60964765>