Bug 200729 - Web Automation: mouse buttons are not correctly printed in SimulatedInputDispatcher log spew
Summary: Web Automation: mouse buttons are not correctly printed in SimulatedInputDisp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-14 12:18 PDT by Brian Burg
Modified: 2019-08-15 09:03 PDT (History)
5 users (show)

See Also:


Attachments
Patch - for review (13.59 KB, patch)
2019-08-14 12:27 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch - for EWS (15.62 KB, patch)
2019-08-14 12:28 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
For EWS (with GTK fix) (17.42 KB, patch)
2019-08-14 17:01 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>