Bug 200729

Summary: Web Automation: mouse buttons are not correctly printed in SimulatedInputDispatcher log spew
Product: WebKit Reporter: Brian Burg <bburg>
Component: WebDriverAssignee: Brian Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, drousso, joepeck, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch - for review
none
Patch - for EWS
none
For EWS (with GTK fix) none

Description Brian Burg 2019-08-14 12:18:38 PDT
Oops.
Comment 1 Brian Burg 2019-08-14 12:27:22 PDT
Created attachment 376290 [details]
Patch - for review
Comment 2 Brian Burg 2019-08-14 12:28:41 PDT
Created attachment 376291 [details]
Patch - for EWS
Comment 3 Brian Burg 2019-08-14 12:30:09 PDT
Comment on attachment 376291 [details]
Patch - for EWS

This patch changes the same signature as the patch in https://bugs.webkit.org/show_bug.cgi?id=200728, but I don't want to land these changes together because Reasons. I combined both of them for EWS purposes.
Comment 4 Devin Rousso 2019-08-14 12:45:44 PDT
Comment on attachment 376291 [details]
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());
```
Comment 5 Brian Burg 2019-08-14 17:01:25 PDT
Created attachment 376335 [details]
For EWS (with GTK fix)
Comment 6 Brian Burg 2019-08-15 09:02:33 PDT
Committed r248715: <https://trac.webkit.org/changeset/248715>
Comment 7 Radar WebKit Bug Importer 2019-08-15 09:03:17 PDT
<rdar://problem/54348165>