WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
209230
[GTK][WPE] Enable kinetic scrolling with async scrolling
https://bugs.webkit.org/show_bug.cgi?id=209230
Summary
[GTK][WPE] Enable kinetic scrolling with async scrolling
Chris Lord
Reported
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).
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Chris Lord
Comment 1
2020-03-18 08:59:06 PDT
Created
attachment 393862
[details]
Patch
Chris Lord
Comment 2
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.
Carlos Garcia Campos
Comment 3
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
Chris Lord
Comment 4
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.
Carlos Garcia Campos
Comment 5
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.
Zan Dobersek
Comment 6
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?
Chris Lord
Comment 7
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.
Zan Dobersek
Comment 8
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).
Chris Lord
Comment 9
2020-03-25 03:04:47 PDT
Created
attachment 394478
[details]
Patch
Zan Dobersek
Comment 10
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.
Chris Lord
Comment 11
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.
Chris Lord
Comment 12
2020-03-27 02:01:39 PDT
Created
attachment 394712
[details]
Patch
Chris Lord
Comment 13
2020-03-27 02:08:09 PDT
Comment on
attachment 394712
[details]
Patch Already reviewed.
EWS
Comment 14
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]
.
Radar WebKit Bug Importer
Comment 15
2020-03-27 05:55:14 PDT
<
rdar://problem/60964765
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug