[webkit-reviews] review granted: [Bug 190345] Adjust keyboard scrolling animator to be a bit more physical : [Attachment 351795] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 8 11:44:33 PDT 2018
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
More information about the webkit-reviews
mailing list