[webkit-reviews] review denied: [Bug 235507] [GTK] REGRESSION: Cumulative velocity for scrolling doesn't work : [Attachment 451354] Patch

[bugzilla-daemon at webkit.org]

Chris Lord <clord at igalia.com> has denied Alexander Mikhaylenko
<alexm at gnome.org>'s request for review:
Bug 235507: [GTK] REGRESSION: Cumulative velocity for scrolling doesn't work
https://bugs.webkit.org/show_bug.cgi?id=235507

Attachment 451354: Patch

https://bugs.webkit.org/attachment.cgi?id=451354&action=review




--- Comment #3 from Chris Lord <clord at igalia.com> ---
Comment on attachment 451354
  --> https://bugs.webkit.org/attachment.cgi?id=451354
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.