[webkit-reviews] review granted: [Bug 190345] Adjust keyboard scrolling animator to be a bit more physical : [Attachment 351795] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 190345: Adjust keyboard scrolling animator to be a bit more physical
https://bugs.webkit.org/show_bug.cgi?id=190345

Attachment 351795: Patch

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




--- Comment #5 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 351795
  --> https://bugs.webkit.org/attachment.cgi?id=351795
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351795&action=review

> Source/WebKit/ChangeLog:3
> +	   Adjust keyboard scrolling animator to be a bit more physical

"a bit more"

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3854
> +	   return self.bounds.size.height * selfScale;

Wouldn't that be the content view height?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3858
> +	   return WebCore::Scrollbar::pixelsPerLineStep() * selfScale;

Maybe get this from the content view too?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:46
> +    WebCore::FloatSize offset; // Points per increment.
> +    WebCore::FloatSize maximumVelocity; // Points per second.

The comments indicate that the members could be named better (or maybe some
typedef types)

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:142
> +	   return WebCore::FloatSize(0, -1);

return { 0., -1. } ?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:292
> +	   _velocity = WebCore::FloatSize();

= { }

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:380
> +    _velocity = WebCore::FloatSize();

= { }

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:391
> +	   WebCore::RectEdges<bool> scrollableDirections = [_scrollable
scrollableDirectionsFromOffset:_currentPosition];

auto

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:421
> +    // Compute the spring's force, and apply it in allowed directions.
> +    // F_spring = -k * x - c * v
> +    auto springForce = -
displacement.scaled(self.parameters.springStiffness) -
_velocity.scaled(self.parameters.springDamping);
> +    force += springForce * axesToApplySpring;

I kinda feel like all the spring logic should be packaged up in a separate
helper class.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:435
> +	   _velocity = WebCore::FloatSize();

= { }

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:580
> +    return CGSizeMake(scrollView._horizontalVelocity * 1000,
scrollView._verticalVelocity * 1000);

Please document the * 1000