[webkit-reviews] review granted: [Bug 188239] Using the keyboard arrow keys to scroll a webpage is very slow, not smooth, takes too long : [Attachment 346304] 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 188239: Using the keyboard arrow keys to scroll a webpage is very slow, not
smooth, takes too long
https://bugs.webkit.org/show_bug.cgi?id=188239

Attachment 346304: Patch

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




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

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3647
> +    if ([_keyboardScrollingAnimator handleKeyEvent:theEvent])
> +	   return;

If a page does preventDefault on arrow key events, should it be able to
override keyboard scrolling?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3753
> +	   return false;

NO

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3756
> +	   return false;

NO

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3758
> +    return true;

YES

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:114
> +    if ([charactersIgnoringModifiers isEqualToString:UIKeyInputUpArrow])

Blank line above please.

This code is pretty hard to read. Would be nice to be able to just have one
call to computeOffset().

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:136
> +

Remove blank line.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:141
> +    _scrollOffsetPerIncrement = *offset;

or offset.value(); that doesn't make me go looking for pointers.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:174
> +    _displayLink = [CADisplayLink displayLinkWithTarget:self
selector:@selector(displayLinkFired:)];

OK to not retain this?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:187
> +    const CFTimeInterval secondsPerScrollIncrement = 0.05; // Seconds it
should take to cover one increment when at full speed.

Odd that this isn't some multiple of 16.667ms?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:188
> +    const float maximumVelocity = 2000; // Maximum velocity in pixels per
second. Empirically determined.

How well does this work on differently sized devices?