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

[bugzilla-daemon at webkit.org]

Chris Lord <clord at igalia.com> has granted 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 452004: Patch

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




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