[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

--- Comment #18 from Aditya Keerthi <akeerthi at apple.com> ---
Comment on attachment 431979
  --> https://bugs.webkit.org/attachment.cgi?id=431979
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 `//`.

-- 
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/20210622/137c6711/attachment.htm>