Bug 226986

Summary: Refactor MacOS keyboard scrolling and use KeyboardScroll struct
Product: WebKit Reporter: Dana Estra <dana.estra>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, annulen, cmarcelo, darin, ews-watchlist, fred.wang, gyuyoung.kim, jamesr, luiz, ryuan.choi, sam, sergio, simon.fraser, thorton, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dana Estra 2021-06-14 13:26:06 PDT
Renavigate MacOS keyboard scrolling and use KeyboardScroll struct
Comment 1 Dana Estra 2021-06-14 13:30:51 PDT
Created attachment 431359 [details]
Patch
Comment 2 Tim Horton 2021-06-14 13:43:50 PDT
Comment on attachment 431359 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:4302
> +    else if (event.keyIdentifier() == "PageDown")
> +        key = Key::PageDown;
> +    else if (event.keyIdentifier() == "PageUp")
> +        key = Key::PageUp;

PageDown is not going to get to defaultArrowEventHandler, likely this all needs to be factored out and called from multiple places (some kind of "maybe handle keyboard scroll" function)

> Source/WebCore/page/EventHandler.cpp:4344
> +    WebCore::KeyboardScroll scroll;
> +
> +    scroll.offset = unitVector(direction).scaled(scrollDistance);
> +    scroll.increment = increment;
> +    scroll.direction = direction;
> +    scroll.maximumVelocity = scroll.offset.scaled(this->parameters().maximumVelocityMultiplier);
> +
> +    scroll.force = scroll.maximumVelocity.scaled(this->parameters().springMass / this->parameters().timeToMaximumVelocity);

Maybe we can add KeyboardScroll in a second patch? First patch computes the bits but calls scrollRecursively directly, second patch introduces KeyboardScroll for the arguments.
Comment 3 Tim Horton 2021-06-14 13:51:10 PDT
(In reply to Tim Horton from comment #2)
> Comment on attachment 431359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431359&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:4302
> > +    else if (event.keyIdentifier() == "PageDown")
> > +        key = Key::PageDown;
> > +    else if (event.keyIdentifier() == "PageUp")
> > +        key = Key::PageUp;
> 
> PageDown is not going to get to defaultArrowEventHandler, likely this all
> needs to be factored out and called from multiple places (some kind of
> "maybe handle keyboard scroll" function)

Also don't forget about the spacebar.
Comment 4 Dana Estra 2021-06-14 13:52:16 PDT
Created attachment 431361 [details]
Patch
Comment 5 Aditya Keerthi 2021-06-14 14:17:21 PDT
You'll need to add 'KeyboardScroll.h' to the Xcode project.
Comment 6 Darin Adler 2021-06-14 14:56:57 PDT
Comment on attachment 431361 [details]
Patch

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

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

What is "Renavigate"?
Comment 7 Dana Estra 2021-06-15 09:48:51 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 431361 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431361&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Renavigate MacOS keyboard scrolling and use KeyboardScroll struct
> 
> What is "Renavigate"?

I couldn't think of a better word. I meant that I am changing the route through functions that is taken to make keyboard scrolling happen.
Comment 8 Dana Estra 2021-06-17 14:07:04 PDT
Created attachment 431719 [details]
Patch
Comment 9 Tim Horton 2021-06-17 15:50:28 PDT
Comment on attachment 431719 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:461
> +    // // First give accessibility a chance to handle the event.
> +    // Frame* frame = frameForEvent(event);
> +    // frame->eventHandler().handleKeyboardSelectionMovementForAccessibility(*event);
> +    // if (event->defaultHandled())
> +    //     return true;

Probably need to leave THIS code alone.

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:468
> +    // if (selector == "moveUp:")

Instead of leaving these commented out, I think you should add an off-by-default (until you're further along) setting (see WebPreferences*.yaml) or maybe a compile-time flag (see PlatformEnable.h); here you'll use it to guard the ones you don't want to be evaluated as "non-editing editing command behaviors" anymore, and will bail in EventHandler::handleKeyboardScrolling.

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:487
> +    // else if (selector == "moveToLeftEndOfLine:")
> +    //     didPerformAction = m_userInterfaceLayoutDirection == WebCore::UserInterfaceLayoutDirection::LTR ? m_page->backForward().goBack() : m_page->backForward().goForward();

For now, you've not moved goForward/goBack, so need to put this one back

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:493
> +    //     didPerformAction = m_userInterfaceLayoutDirection == WebCore::UserInterfaceLayoutDirection::LTR ? m_page->backForward().goForward() : m_page->backForward().goBack();

For now, you've not moved goForward/goBack, so need to put this one back
Comment 10 Tim Horton 2021-06-17 16:43:46 PDT
Comment on attachment 431719 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:4244
> +const WebCore::KeyboardScrollParameters& EventHandler::parameters()
> +{
> +    static const KeyboardScrollParameters parameters;
> +    return parameters;
> +}

I think KeyboardScrollParameters could maybe hang on a static method off KeyboardScroll? EventHandler is not the right place, anyway

> Source/WebCore/page/EventHandler.h:379
> +

Too many newlines around this paragraph

> Source/WebCore/page/EventHandler.h:380
> +    const WebCore::KeyboardScrollParameters& parameters();

Not sure why this is here. Don't think it needs to be.

> Source/WebCore/page/KeyboardScroll.h:34
> +enum class ScrollingIncrement : uint8_t {
> +    Line,
> +    Page,
> +    Document
> +};

Instead of this, you should just make all this code use ScrollTypes.h's ScrollGranularity

> Source/WebCore/page/KeyboardScroll.h:41
> +enum class ScrollingDirection : uint8_t {
> +    Up,
> +    Down,
> +    Left,
> +    Right
> +};

Instead of this, you should just make all this code use ScrollTypes.h's ScrollDirection

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:783
> +    

Should be able to revert this and not touch this file at all :D
Comment 11 Aditya Keerthi 2021-06-17 17:14:36 PDT
Comment on attachment 431719 [details]
Patch

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

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

Nit: Update to match bug title.

> Source/WebCore/page/EventHandler.cpp:4193
> +    

You should configure your editor to remove trailing spaces :)

> Source/WebCore/page/EventHandler.cpp:4209
> +    

Nit: Unnecessary newline.

> Source/WebCore/page/EventHandler.cpp:4250
> +        return {0, -1};

Nit: Add spaces around the values, like `{ 0, -1 };`.

> Source/WebCore/page/EventHandler.cpp:4260
> +CGFloat EventHandler::ScrollDistance(WebCore::ScrollingDirection direction, WebCore::ScrollingIncrement increment)

Nit: `scrollDistance`.

> Source/WebCore/page/EventHandler.cpp:4263
> +    FrameView* view = frame->view();

`m_frame.view()`. No need for the local variable.

> Source/WebCore/page/EventHandler.cpp:4351
> +    scroll.maximumVelocity = scroll.offset.scaled(this->parameters().maximumVelocityMultiplier);
> +
> +    scroll.force = scroll.maximumVelocity.scaled(this->parameters().springMass / this->parameters().timeToMaximumVelocity);

Remove `this`.
Comment 12 Radar WebKit Bug Importer 2021-06-21 13:27:20 PDT
<rdar://problem/79577430>
Comment 13 Dana Estra 2021-06-21 15:22:28 PDT
Created attachment 431916 [details]
Patch
Comment 14 Dana Estra 2021-06-21 15:26:59 PDT
Created attachment 431918 [details]
Patch
Comment 15 Dana Estra 2021-06-21 15:41:26 PDT
Created attachment 431919 [details]
Patch
Comment 16 Dana Estra 2021-06-22 11:18:14 PDT
Created attachment 431979 [details]
Patch
Comment 17 Tim Horton 2021-06-22 11:37:30 PDT
Comment on attachment 431979 [details]
Patch

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

Need to get the bots not red and clean up some random files that you don't need to touch, but this seems pretty much ready to go.

> Source/WebCore/page/EventHandler.cpp:4192
> -
> +    

What editor are you using that is making all these spurious changes :) If it's Xcode, check "including white-space only lines" in the "editing" tab of "text editing"

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:315
> +        eventWasHandled = executeKeypressCommandsInternal(commands, &event); 

?

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:456
> +    // // First give accessibility a chance to handle the event.

What's this!
Comment 18 Aditya Keerthi 2021-06-22 11:39:00 PDT
Comment on attachment 431979 [details]
Patch

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

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

Nit: You can remove this line.

It would be nice to summarize the refactoring here (and in the WebKit changelog).

> Source/WebCore/page/EventHandler.cpp:-3802
> -

Nit: Undo this change.

> Source/WebCore/page/EventHandler.cpp:4192
> +    

Nit: Remove trailing whitespace (also a couple more instances below).

> Source/WebCore/page/EventHandler.cpp:4206
> +    if (m_frame.settings().smoothKeyboardScrollingEnabled()) {

This could be a ternary: `m_frame.settings().smoothKeyboardScrollingEnabled() ? handleKeyboardScrolling(event) : view->logicalScroll(direction, ScrollByPage)`

> Source/WebCore/page/EventHandler.cpp:4338
> +    WebCore::KeyboardScroll scroll;

No need for `WebCore::`, since you're in the WebCore namespace.

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

Nit: Add some newlines here.

```
#pragma once

#include "ScrollTypes.h"

namespace WebCore {
```

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:456
> +    // // First give accessibility a chance to handle the event.

Nit: Remove added `//`.
Comment 19 Aditya Keerthi 2021-06-22 12:00:06 PDT
Comment on attachment 431979 [details]
Patch

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

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

Use lambda syntax (`[] {`) rather than block syntax since this is a C++ file.

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

Ditto.
Comment 20 Dana Estra 2021-06-22 14:12:18 PDT
Created attachment 431992 [details]
Patch
Comment 21 Dana Estra 2021-06-22 14:49:59 PDT
Created attachment 431997 [details]
Patch
Comment 22 Sam Weinig 2021-06-22 18:00:11 PDT
Comment on attachment 431997 [details]
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?
Comment 23 Dana Estra 2021-06-22 18:57:30 PDT
Created attachment 432012 [details]
Patch
Comment 24 Tim Horton 2021-06-23 16:26:44 PDT
(In reply to Sam Weinig from comment #22)
> Comment on attachment 431997 [details]
> 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.

Ideally, yes. In practice this is a bit annoying, because the input to this function is an event, and I don't think there is any event type that is *commonly* used both in WebCore in the Web Content process and WebKit in the UI process. Ideally we could make the code deal in terms of WebCore events (WebCore::KeyboardEvent), but it's a bit weird to use the DOMmy KeyboardEvent in the UI process, no?

I think it might be better if Dana doesn't try to solve this all the way in the first patch, since it will involve lots of thoughtful refactoring of the iOS code. But there is a good bit of sharing that she /can/ do; e.g. `unitVector` like you said, also the code to fill out a KeyboardScroll instance once you have a ScrollGranularity+ScrollDirection.

> > Source/WebCore/page/KeyboardScroll.h:47
> > +    CGFloat springMass { 1 };
> > +    CGFloat springStiffness { 109 };
> > +    CGFloat springDamping { 20 };
> > +
> > +    CGFloat maximumVelocityMultiplier { 25 };
> > +    CGFloat timeToMaximumVelocity { 1 };
> > +
> > +    CGFloat rubberBandForce { 5000 };
> 
> 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?

This was my doing; back in the day they used to be adjustable with some UI, and I never constexpr'd them. Dana, you're welcome to do so, though I don't think it's vital for this patch.

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

Agreed.
Comment 25 Sam Weinig 2021-06-24 10:01:54 PDT
(In reply to Tim Horton from comment #24)
> (In reply to Sam Weinig from comment #22)
> > Comment on attachment 431997 [details]
> > 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.
> 
> Ideally, yes. In practice this is a bit annoying, because the input to this
> function is an event, and I don't think there is any event type that is
> *commonly* used both in WebCore in the Web Content process and WebKit in the
> UI process. Ideally we could make the code deal in terms of WebCore events
> (WebCore::KeyboardEvent), but it's a bit weird to use the DOMmy
> KeyboardEvent in the UI process, no?

I would solve this with a new struct that contains the input to the algorithm that both event types can be be constructed from or adding a shared interface for both event types.

> 
> I think it might be better if Dana doesn't try to solve this all the way in
> the first patch, since it will involve lots of thoughtful refactoring of the
> iOS code. But there is a good bit of sharing that she /can/ do; e.g.
> `unitVector` like you said, also the code to fill out a KeyboardScroll
> instance once you have a ScrollGranularity+ScrollDirection.

Yeah, let's do what sharing we can. Free functions on the platform types will go a long way.
Comment 26 Dana Estra 2021-06-24 11:24:26 PDT
Created attachment 432194 [details]
Patch
Comment 27 Aditya Keerthi 2021-06-24 12:09:29 PDT
Comment on attachment 432194 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No tests yet

Nit: Add a period at the end of the sentence.

Also might be good to summarize the changes.

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

The IOS_FAMILY build is failing because this method is not in a header (and is not WEBCORE_EXPORT).

I think you should move this to a new KeyboardScroll.cpp, and WEBCORE_EXPORT the method in KeyboardScroll.h. And probably rename the method to unitVectorForScrollDirection.

You'll need to add the new file to Source/WebCore/Sources.txt (and we should probably add the header to Source/WebCore/Headers.cmake).

> Source/WebCore/page/EventHandler.cpp:4332
> +    CGFloat distance = scrollDistance(direction, granularity);
> +    
> +    KeyboardScroll scroll;
> +
> +    scroll.offset = unitVector(direction).scaled(distance);
> +    scroll.granularity = granularity;
> +    scroll.direction = direction;
> +    scroll.maximumVelocity = scroll.offset.scaled(KeyboardScrollParameters::parameters().maximumVelocityMultiplier);
> +
> +    scroll.force = scroll.maximumVelocity.scaled(KeyboardScrollParameters::parameters().springMass / KeyboardScrollParameters::parameters().timeToMaximumVelocity);

It looks like none of these variables are actually used?
Comment 28 Dana Estra 2021-06-25 13:14:06 PDT
Created attachment 432287 [details]
Patch
Comment 29 Dana Estra 2021-06-25 14:46:35 PDT
Created attachment 432293 [details]
Patch
Comment 30 Tim Horton 2021-06-28 12:23:29 PDT
Comment on attachment 432293 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:4259
> +    // FIXME: This logic does not account for writing-mode.

This should have a bugzilla number in parens (FIXME (123456): ...)

> Source/WebCore/page/EventHandler.cpp:4318
> +    KeyboardScroll scroll;
> +
> +    scroll.offset = unitVectorForScrollDirection(direction).scaled(distance);
> +    scroll.granularity = granularity;
> +    scroll.direction = direction;
> +    scroll.maximumVelocity = scroll.offset.scaled(KeyboardScrollParameters::parameters().maximumVelocityMultiplier);
> +
> +    scroll.force = scroll.maximumVelocity.scaled(KeyboardScrollParameters::parameters().springMass / KeyboardScrollParameters::parameters().timeToMaximumVelocity);

Not the end of the world, but ideally you would stash this unused code aside for patch 2 instead of including it here.

> Source/WebCore/page/KeyboardScroll.cpp:1
> +// Copyright (C) 2021 Apple Inc. All rights reserved.

This copyright header should look like the one in your .h (with a multi-line comment); not sure where this version came from.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:-299
> -    WebKit::KeyboardScroll scroll;

Should be a FIXME in this general vicinity about adopting the WebCore version of this.
Comment 31 Dana Estra 2021-06-28 14:42:31 PDT
Created attachment 432423 [details]
Patch
Comment 32 Darin Adler 2021-06-28 14:48:17 PDT
Comment on attachment 432423 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:489
> +        didPerformAction = scroll(m_page.get(), ScrollRight, ScrollByPage);

Wrong indentation here.
Comment 33 Dana Estra 2021-06-28 14:55:43 PDT
Created attachment 432426 [details]
Patch
Comment 34 Tim Horton 2021-06-28 17:20:04 PDT
Comment on attachment 432426 [details]
Patch

There's more we can do to clean up and deduplicate, but let's start here.
Comment 35 EWS 2021-06-28 17:39:24 PDT
Committed r279355 (239223@main): <https://commits.webkit.org/239223@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432426 [details].