[webkit-reviews] review granted: [Bug 193852] Web Automation: add support for simulating single touches to Automation.performInteractionSequence : [Attachment 360194] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 193852: Web Automation: add support for simulating single touches to
Automation.performInteractionSequence
https://bugs.webkit.org/show_bug.cgi?id=193852

Attachment 360194: Patch

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 360194
  --> https://bugs.webkit.org/attachment.cgi?id=360194
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);