.
<rdar://problem/28360781>
Created attachment 360194 [details] Patch
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);
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.
Committed r240554: <https://trac.webkit.org/changeset/240554>
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.