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).
Created attachment 393862 [details] Patch
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 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
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.
(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 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?
(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 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).
Created attachment 394478 [details] Patch
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.
(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.
Created attachment 394712 [details] Patch
Comment on attachment 394712 [details] Patch Already reviewed.
Committed r259112: <https://trac.webkit.org/changeset/259112> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394712 [details].
<rdar://problem/60964765>