[webkit-reviews] review granted: [Bug 228009] Add key-driven smooth scrolling to macOS : [Attachment 434249] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Dana Estra <destra at apple.com>'s
request for review:
Bug 228009: Add key-driven smooth scrolling to macOS
https://bugs.webkit.org/show_bug.cgi?id=228009

Attachment 434249: Patch

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




--- Comment #15 from Tim Horton <thorton at apple.com> ---
Comment on attachment 434249
  --> https://bugs.webkit.org/attachment.cgi?id=434249
Patch

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

> Source/WebCore/page/EventHandler.cpp:3807
> +    if (event.type() == eventNames().keyupEvent)
> +	   stopKeyboardScrolling();

It is odd that this one doesn't follow the pattern of the others (passing it to
editor first, bailing if it ate the event). On the other hand, we know that we
never got here with keyup before, so I assume editor's handleKeyboardEvent
doesn't do anything with keyup? (please check) But in that case, maybe it's
best to follow the pattern anyway to minimize surprise later. Would be
interested to hear other people's opinions.

> Source/WebCore/platform/KeyboardScrollingAnimator.h:42
> +    void handleKeyUpEventWhileScrolling();

The caller doesn't actually know that this is "while scrolling", right, they're
just plumbing us every keyup? So maybe drop to just "handleKeyUpEvent". The
class name indicates who's handling it and why sufficiently.