[webkit-reviews] review denied: [Bug 226986] Refactor MacOS keyboard scrolling and use KeyboardScroll struct : [Attachment 431997] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Dana Estra <destra at apple.com>'s request
for review:
Bug 226986: Refactor MacOS keyboard scrolling and use KeyboardScroll struct
https://bugs.webkit.org/show_bug.cgi?id=226986

Attachment 431997: Patch

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




--- 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?