Bug 193852 – Web Automation: add support for simulating single touches to Automation.performInteractionSequence

• Component: WebDriver
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Blaze Burg]

<rdar://problem/28360781>

[Blaze Burg]

Created attachment 360194 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 360194 [details]
Patch

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

Looks good to me! May want to get a nod from a WK2 owner.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:54
> +

Style: Unnecessary newline

> Source/WebKit/UIProcess/Automation/mac/WebAutomationSessionMac.mm:217
> +#if ENABLE(WEBDRIVER_KEYBOARD_INTERACTIONS)
>  

Style: Most of these guards do not have empty line buffers. Should be consistent.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.h:43
> +- (void)receivedHIDEvent:(IOHIDEventRef)event;

This could use a comment, since it is part of the contract I guess.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:35
> +#import <wtf/Optional.h>

Is optional used?

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:70
> +    return (CFAbsoluteTimeGetCurrent() - startTime);

Style: Unnecessary parenthesis.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:75
> +    return (a + (b - a) * sin(sin(t * M_PI / 2) * t * M_PI / 2));

Style: Unnecessary parenthesis.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:83
> +typedef double(*pressureInterpolationFunction)(double, double, CFTimeInterval);

Nit: Unused

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:112
> +    static _WKTouchEventGenerator *eventGenerator = nil;
> +    if (!eventGenerator)
> +        eventGenerator = [[_WKTouchEventGenerator alloc] init];

This is a .mm you should be able to make this one line:

    static _WKTouchEventGenerator *eventGenerator = [[_WKTouchEventGenerator alloc] init];;

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:141
> +- (void)dealloc
> +{
> +    [_eventCallbacks release];
> +    [super dealloc];
> +}

This is never reached, but it should release the _ioSystemClient if non-null:

    if (_ioSystemClient)
        CFRelease(_ioSystemClient);

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:399
> +    CFIndex callbackID = IOHIDEventGetIntegerValue(event, kIOHIDEventFieldVendorDefinedData);
> +    void (^completionBlock)() = [_eventCallbacks objectForKey:@(callbackID)];
> +    if (completionBlock) {
> +        [_eventCallbacks removeObjectForKey:@(callbackID)];

I'd suggest doing @(callbackID) once, to avoid ObjC duplicating this multiple times.

    NSNumber *key = @(callbackID);

[Simon Fraser (smfr)]

Comment on attachment 360194 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        use a stripped down version of HIDEventGenerator to send HID events to the UIWindow.

Now we have 3 copies of this code :(

> Source/WebKit/UIProcess/_WKTouchEventGenerator.h:41
> +- (void)touchDown:(CGPoint)location completionBlock:(void (^)(void))completionBlock;
> +- (void)liftUp:(CGPoint)location completionBlock:(void (^)(void))completionBlock;
> +- (void)moveToPoint:(CGPoint)location duration:(NSTimeInterval)seconds completionBlock:(void (^)(void))completionBlock;

The coordinate system of 'location' should be documented.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:93
> +    double delay = (eventIndex * fingerMoveInterval) - elapsed;
> +    if (delay > 0) {
> +        struct timespec moveDelay = { 0, static_cast<long>(delay * nanosecondsPerSecond) };
> +        nanosleep(&moveDelay, NULL);
> +    }   
> +}

We may not want to keep doing it this way, sleeping the main thread.

[Blaze Burg]

Committed r240554: <https://trac.webkit.org/changeset/240554>

[Wenson Hsieh]

Comment on attachment 360194 [details]
Patch

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

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:153
>      for (auto& identifier : copyToVector(m_pendingKeyboardEventsFlushedCallbacksPerPage.keys())) {

Nit - Not new to this patch, but I think using std::exchange here might be a bit cleaner.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:160
>      for (auto& identifier : copyToVector(m_pendingMouseEventsFlushedCallbacksPerPage.keys())) {

Ditto.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:665
>              for (auto key : copyToVector(m_pendingMouseEventsFlushedCallbacksPerPage.keys())) {

Ditto.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:674
>              for (auto key : copyToVector(m_pendingKeyboardEventsFlushedCallbacksPerPage.keys())) {

Ditto.

> Source/WebKit/UIProcess/Automation/ios/WebAutomationSessionIOS.mm:180
> +    WebCore::IntPoint locationOnScreen = page.syncRootViewToScreen(IntRect(locationInViewport, IntSize())).location();

Contrary to its name, syncRootViewToScreen seems to convert root view coordinates to window coordinates rather than screen coordinates (since it passes nil as the view to convertRect:toView: — https://developer.apple.com/documentation/uikit/uiview/1622504-convertrect).

We should double check that this is what IOKit expects.

> Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:132
> +    _eventCallbacks = [[NSMutableDictionary alloc] init];

Nit - I think we generally prefer RetainPtr over explicit -release in -dealloc.