[webkit-reviews] review granted: [Bug 200729] Web Automation: mouse buttons are not correctly printed in SimulatedInputDispatcher log spew : [Attachment 376291] Patch - for EWS

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted  review:
Bug 200729: Web Automation: mouse buttons are not correctly printed in
SimulatedInputDispatcher log spew
https://bugs.webkit.org/show_bug.cgi?id=200729

Attachment 376291: Patch - for EWS

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 376291
  --> https://bugs.webkit.org/attachment.cgi?id=376291
Patch - for EWS

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

r=me, nice fixes

> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:147
> +    void simulateMouseInteraction(WebPageProxy&, MouseInteraction,
MouseButton, const WebCore::IntPoint& locationInView,
AutomationCompletionHandler&&) final;

Should this also be `locationInViewport` to match?

> Source/WebKit/UIProcess/Automation/mac/WebAutomationSessionMac.mm:125
> +static WebMouseEvent::Button
automationMouseButtonToPlatformMouseButton(MouseButton button)

Is this function really necessary?  The only place it's used is inside yet
another switch-case, so couldn't you just have it switch on `MouseButton` there
instead?

> Source/WebKit/UIProcess/Automation/mac/WebAutomationSessionMac.mm:132
> +    case MouseButton::Left:	 return WebMouseEvent::LeftButton;
> +    case MouseButton::Middle: return WebMouseEvent::MiddleButton;
> +    case MouseButton::Right:  return WebMouseEvent::RightButton;
> +    case MouseButton::None:	 return WebMouseEvent::NoButton;
> +    default: ASSERT_NOT_REACHED();

Style: this doesn't look like WebKit style, so please split onto separate lines
(like how `protocolMouseButtonToWebMouseEventButton` used to be).

> Source/WebKit/UIProcess/Automation/mac/WebAutomationSessionMac.mm:140
> +    IntPoint locationInView = WebCore::IntPoint(locationInViewport.x(),
locationInViewport.y() + page.topContentInset());

NIT: I usually either use `auto` as the type, or don't do an assignment
constructor so that you only have to type the type once.
```
    auto locationInView = IntPoint(locationInViewport.x(),
locationInViewport.y() + page.topContentInset());
```
or
```
    IntPoint locationInView(locationInViewport.x(), locationInViewport.y() +
page.topContentInset());
```