[Webkit-unassigned] [Bug 226986] Refactor MacOS keyboard scrolling and use KeyboardScroll struct

[bugzilla-daemon at webkit.org]

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

Sam Weinig <sam at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at webkit.org
 Attachment #431997|review?                     |review-
              Flags|                            |

--- Comment #22 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 431997
  --> https://bugs.webkit.org/attachment.cgi?id=431997
Patch

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

Seems like there is a bunch of code duplication going on. Instead, it would be better to make the code useable by both WKKeyboardScrollingAnimator and EventHandler, and have those classes called shared functionality.

> Source/WebCore/ChangeLog:8
> +        Renavigate MacOS keyboard scrolling and use KeyboardScroll struct

I don't think you mean Renavigate here. Refactor?

> Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:696
> +   default: false

Given we do support smooth keyboard scrolling on iOS, this seems like an incorrect default. If this setting does not effect the iOS functionality, we should either update things so this does, or rename this to be more clear about what it does.

> Source/WebCore/page/EventHandler.cpp:4240
> +FloatSize EventHandler::unitVector(ScrollDirection direction)

This seems like it should be a free function rather than a static class function.

> Source/WebCore/page/EventHandler.cpp:4254
> +CGFloat EventHandler::scrollDistance(ScrollDirection direction, ScrollGranularity granularity)

We try to avoid using platform specific types like CGFloat in platform independent files if we can avoid it. This should just return a float or double.

> Source/WebCore/page/EventHandler.cpp:4261
> +    Scrollbar* scrollbar;
> +
> +    if (direction == ScrollDirection::ScrollUp || direction == ScrollDirection::ScrollDown) 
> +        scrollbar = m_frame.view()->verticalScrollbar();
> +    else 
> +        scrollbar = m_frame.view()->horizontalScrollbar();

A trick we do for things like this with an uninitialized variable is to use an anonymous lambda that is called immediately.  
auto scrollbar = [&] {
    if (direction == ScrollDirection::ScrollUp || direction == ScrollDirection::ScrollDown) 
        return m_frame.view()->verticalScrollbar();
     return m_frame.view()->horizontalScrollbar();
}();

> Source/WebCore/page/EventHandler.cpp:4278
> +    switch (granularity) {
> +    case ScrollGranularity::ScrollByLine:
> +        step = scrollbar->lineStep();
> +        break;
> +    case ScrollGranularity::ScrollByPage:
> +        step = scrollbar->pageStep();
> +        break;
> +    case ScrollGranularity::ScrollByDocument:
> +        step = scrollbar->totalSize();
> +        break;
> +    case ScrollGranularity::ScrollByPixel:
> +        step = scrollbar->pixelStep();
> +        break;
> +    }

These should just return the value directly.

> Source/WebCore/page/EventHandler.cpp:4301
> +    auto granularity = ^() {

This should use a c++ lambda, not a block. Blocks should only be used when lambdas absolutely cannot.

> Source/WebCore/page/EventHandler.cpp:4330
> +        };

; is not needed here.

> Source/WebCore/page/KeyboardScroll.h:28
> +#pragma once
> +#include "ScrollTypes.h"
> +namespace WebCore {

Please add newlines between these.

> Source/WebCore/page/KeyboardScroll.h:33
> +    WebCore::FloatSize offset; // Points per increment.
> +    WebCore::FloatSize maximumVelocity; // Points per second.
> +    WebCore::FloatSize force;

WebCore:: are not needed.

> Source/WebCore/page/KeyboardScroll.h:47
> +    CGFloat springMass { 1 };
> +    CGFloat springStiffness { 109 };
> +    CGFloat springDamping { 20 };
> +
> +    CGFloat maximumVelocityMultiplier { 25 };
> +    CGFloat timeToMaximumVelocity { 1 };
> +
> +    CGFloat rubberBandForce { 5000 };

Should not be using CGFloat here. Use float instead.

Do these ever change? Seems weird to have these constants just sitting here as default parameters. If we think they might vary, perhaps they should be on Settings? If not, perhaps just some constexprs would suffice?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:48
> +- (CGFloat)distanceForIncrement:(ScrollGranularity)increment inDirection:(ScrollDirection)direction;

I would add the explicit WebCore:: here for the types.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:127
> +    case ScrollDirection::ScrollUp:
>          return { 0, -1 };
> -    case WebKit::ScrollingDirection::Down:
> +    case ScrollDirection::ScrollDown:
>          return { 0, 1 };
> -    case WebKit::ScrollingDirection::Left:
> +    case ScrollDirection::ScrollLeft:
>          return { -1, 0 };
> -    case WebKit::ScrollingDirection::Right:
> +    case ScrollDirection::ScrollRight:

This seems like it is a duplicate of the code in WebCore. Why not just use that?

-- 
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/20210623/5efb3596/attachment.htm>