I thought this was a problem with my patch in https://bugs.webkit.org/show_bug.cgi?id=235436, but it doesn't seem to be working in canary either, with either touchpad or touchscreen. The velocity doesn't seem affected by ongoing scrolling, and quickly scrolling with multiple swipes is noticeably slower than in 2.34.x. Not sure what it could be offhand.
Ok, so ScrollingEffectsController destroys and recreates ScrollAnimationKinetic every time you scroll. That would explain it. Not sure how to cleanly fix this though.
Created attachment 451354 [details] Patch
Comment on attachment 451354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451354&action=review Overall good, but I guess I'd like to understand that animateScroll removal and I think my suggestion for the change to prepareLastVelocity is worthwhile implementing before committing. > Source/WebCore/platform/ScrollAnimationKinetic.cpp:-167 > - if (axisData && axisData.value().animateScroll(elapsedTime)) { Why has this call to animateScroll been removed? Doesn't this mean that there'll be an extra frame of lag when starting animated scrolls now? > Source/WebCore/platform/ScrollAnimationKinetic.cpp:234 > +void ScrollAnimationKinetic::prepareLastVelocity(const MonotonicTime lastStartTime, const FloatPoint& lastInitialOffset, const FloatSize& lastInitialVelocity) Personally, I'd really prefer this to return the calculated value and for it to be assigned at the call-site, this is a bit confusingly named until you see how it's called. I think it'd be better for this to be FloatSize ScrollAnimationKinetic::calculateLastVelocity(...) > Source/WebCore/platform/ScrollingEffectsController.cpp:319 > + kineticAnimation.prepareLastVelocity(m_lastStartTime, m_lastInitialOffset, m_lastInitialVelocity); Then this would read m_lastVelocity = kineticAnimation.calculateLastVelocity(...) and I think that'd be easier to follow.
> Why has this call to animateScroll been removed? Doesn't this mean that there'll be an extra frame of lag when starting animated scrolls now? Nope. Notice how the elapsed time was 0_s - it wasn't doing anything already. The actual call with non-0 elapsed time is now in prepareLastVelocity() - which I split to avoid having 3 more parameters in startAnimatedScrollWithInitialVelocity(). > Personally, I'd really prefer this to return the calculated value and for it to be assigned at the call-site, this is a bit confusingly named until you see how it's called. I think it'd be better for this to be FloatSize ScrollAnimationKinetic::calculateLastVelocity(...) So you mean having a local variable in processWheelEventForKineticScrolling() to immediately pass it again to startAnimatedScrollWithInitialVelocity()? That can work, sure - though not sure if it's cleaner. I guess less state in ScrollAnimationKinetic is nice.
(In reply to Alexander Mikhaylenko from comment #4) > > Why has this call to animateScroll been removed? Doesn't this mean that there'll be an extra frame of lag when starting animated scrolls now? > > Nope. Notice how the elapsed time was 0_s - it wasn't doing anything > already. The actual call with non-0 elapsed time is now in > prepareLastVelocity() - which I split to avoid having 3 more parameters in > startAnimatedScrollWithInitialVelocity(). I see, that's fine in that case. > > Personally, I'd really prefer this to return the calculated value and for it to be assigned at the call-site, this is a bit confusingly named until you see how it's called. I think it'd be better for this to be FloatSize ScrollAnimationKinetic::calculateLastVelocity(...) > > So you mean having a local variable in > processWheelEventForKineticScrolling() to immediately pass it again to > startAnimatedScrollWithInitialVelocity()? That can work, sure - though not > sure if it's cleaner. I guess less state in ScrollAnimationKinetic is nice. Ah, I think I hadn't really understood what was happening here properly and now it makes more sense to me. I still think it's a little confusing the mix of both calculating a value based on the set inputs, and having the side effect of scrolling, when the name of it is 'prepareLastVelocity' - it seems to do a lot more than that. Would it make sense to just call it 'animateScroll'?
> and having the side effect of scrolling It doesn't have it though? :)
(In reply to Alexander Mikhaylenko from comment #6) > > and having the side effect of scrolling > > It doesn't have it though? :) It calls animateScroll on each axis with an elapsed time, which will change the offset of each axis - unless I'm really misreading/misunderstanding something? I think this confusion is enough evidence to me that at the very least some naming needs to be reconsidered.
Created attachment 452004 [details] Patch
That looks unexpected. Restarting.
Comment on attachment 452004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452004&action=review Yes, this reads much better to me now - thanks for your patience :) I'll leave cq? in case you want to make any of the changes I suggest, but I'm happy enough as it is, so don't feel obligated - ping me if you want me to set the cq+ > Source/WebCore/platform/ScrollAnimationKinetic.cpp:168 > + auto accumulateVelocity = [&](double velocity, double lastVelocity) -> double { Might be nice to rename lastVelocity -> previousVelocity here to match the name of the parent function parameter - that does mean a name collision, which should be fine, but if you wanted to avoid that, the parent's parameter could be renamed to previousVelocities. > Source/WebCore/platform/ScrollingEffectsController.h:242 > + MonotonicTime m_previousStartTime; > + FloatPoint m_previousInitialOffset; > + FloatSize m_previousInitialVelocity; Given that these are specific to kinetic animation/gestures, it'd be good to somehow reflect that in the naming, or even group them together. Maybe it'd be overkill, but perhaps these could be in a struct or a class - PreviousKineticAnimationInfo, or VelocityAccumulationInfo, or some better third option? I can't think of good naming that wouldn't massively balloon code-size/line-length, so maybe it's just fine as it is.
Created attachment 452171 [details] Patch
Added an inline struct just to group them together. Doing more would likely be an overkill here indeed. :) > ping me if you want me to set the cq+ I have access to set it myself FWIW. :)
Comment on attachment 452171 [details] Patch LGTM. Pretty certain the failure is entirely unrelated, I've retriggered and set cq?
Committed r290045 (247417@main): <https://commits.webkit.org/247417@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452171 [details].