[Webkit-unassigned] [Bug 228009] Add key-driven smooth scrolling to macOS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=228009

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

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

> Source/WebCore/page/EventHandler.cpp:93
> +#include "ScrollAnimatorMac.h"

Can't do this, because this is cross-platform code.

> Source/WebCore/page/EventHandler.cpp:4314
> +    KeyboardScroll* scroll = new KeyboardScroll;

A manually-managed heap-allocated object is an odd choice for this; I would go with something more like the iOS implementation, which maintains a `std::optional<KeyboardScroll>` instead (unique_ptr<KeyboardScroll> would also be fine, but a raw pointer is bad news and easy to get wrong/cause leaks/etc.)

> Source/WebCore/platform/ScrollController.cpp:71
> +    setIsAnimatingSmoothScrolling(true);

Probably want to say "keyboard" most of the places you say "smooth"

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:70
> +    bool beginKeyboardScrollAnimation(KeyboardScroll*, KeyboardEvent&) override;
> +    void handleKeyUpEventWhileScrolling() override;
> +    void stopAnimatedScroll();

Possible we can just put our implementation on ScrollAnimator proper, instead of the mac specific subclass? I'm not sure, should see what smfr thinks. Generally cross-platform-first-unless-there's-a-reason-not-to is the WebKit approach, though.

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:173
> +    KeyboardScroll *m_currentScroll;

star's on the wrong side.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:96
> +    if (m_currentScroll) {

It would be fantastic to think about how to implement some of the physics in terms of free functions that can be shared between the two implementations (maybe a structure to represent the velocity/acceleration/current+ideal positions/etc., and some functions that are like "please move this state forward one tick (of this duration) given this KeyboardScroll", that only operate on inputs and return the new state, etc. (We can talk about the details, but we should get to the place where there's very little interesting code in WKKeyboardScrollingAnimator for example).

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:186
> +void ScrollAnimatorMac::stopAnimatedScroll()

Probably needs to have a name that is somehow about the keyboard

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:902
> +    delete m_currentScroll;

See above

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210716/f6a3e280/attachment.htm>