Bug 235507 – [GTK] REGRESSION: Cumulative velocity for scrolling doesn't work

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

Ok, so ScrollingEffectsController destroys and recreates ScrollAnimationKinetic every time you scroll. That would explain it. Not sure how to cleanly fix this though.

[Alice Mikhaylenko]

Created attachment 451354 [details]
Patch

[Chris Lord]

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.

[Alice Mikhaylenko]

> 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.

[Chris Lord]

(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'?

[Alice Mikhaylenko]

> and having the side effect of scrolling

It doesn't have it though? :)

[Chris Lord]

(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.

[Alice Mikhaylenko]

Created attachment 452004 [details]
Patch

[Alice Mikhaylenko]

That looks unexpected. Restarting.

[Chris Lord]

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.

[Alice Mikhaylenko]

Created attachment 452171 [details]
Patch

[Alice Mikhaylenko]

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. :)

[Chris Lord]

Comment on attachment 452171 [details]
Patch

LGTM. Pretty certain the failure is entirely unrelated, I've retriggered and set cq?

[EWS]

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].