Extend event stream to include interpolated events and add a force press test that uses that interpolation
Created attachment 290999 [details] Patch
Comment on attachment 290999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290999&action=review > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:999 > + NSMutableDictionary *newTouch = [endTouch mutableCopy]; This leaks.
Created attachment 291001 [details] Patch
Comment on attachment 291001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291001&action=review > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:131 > +static pressureInterpolationFunction interplations[] = { interplations[] => interpolations[]? > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:233 > + if ([string isEqualToString:HIDEventInterpolationTypeLinear]) Can we remove this [string isEqualToString:HIDEventInterpolationTypeLinear] check here? > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:975 > + InterpolationType interpType = interpolationFromString(interpolationsDictionary[HIDEventInterpolateKey]); Nit - I think we generally don't abbreviate variable names per style guidelines (i.e. it should be interpolationType here). > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:982 > + NSTimeInterval time = startTime; Should this be starting at startTime + timeStep, or just startTime? From the while loop, it seems like we'll stop before computing the touch dictionary corresponding to endTime, so it seems a bit strange we're including startTime's touch dictionary in here. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:989 > + double t = (time - startTime) / (endTime - startTime); Nit - we can pull (endTime - startTime) out of the while loop. Could we also name this variable something like 'progress'?
Created attachment 291127 [details] Patch
Comment on attachment 291127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291127&action=review > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:307 > + if (phase != UITouchPhaseCancelled && phase != UITouchPhaseBegan && phase != UITouchPhaseEnded > + && phase != UITouchPhaseStationary) No need to wrap this. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:977 > + > + Remove one of the blank lines. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:988 > + NSMutableDictionary *newEvent = [endEvent mutableCopy]; We're not using ARC, so this is leaked. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:997 > + while ((startTouch = [startEnumerator nextObject]) && (endTouch = [endEnumerator nextObject])) { This assumes that the touches are listed in "id" order. Maybe you should just bail if the start and end events have touches with non-matching ids? At the very least, you need to find matching touches by id. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:999 > + NSMutableDictionary *newTouch = [[endTouch mutableCopy] autorelease]; Perferable to just explicitly -release after the [newTouches addObject:newTouch] below. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:1002 > + newTouch[HIDEventXKey] = [NSNumber numberWithDouble:interpolations[interpolationType]([startTouch[HIDEventXKey] doubleValue], [endTouch[HIDEventXKey] doubleValue], t)]; I think you can use @() here. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:1005 > + newTouch[HIDEventYKey] = [NSNumber numberWithDouble:interpolations[interpolationType]([startTouch[HIDEventYKey] doubleValue], [endTouch[HIDEventYKey] doubleValue], t)]; Ditto. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:1008 > + newTouch[HIDEventPressureKey] = [NSNumber numberWithDouble:interpolations[interpolationType]([startTouch[HIDEventPressureKey] doubleValue], [endTouch[HIDEventPressureKey] doubleValue], t)]; Ditto. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:1029 > + if (interpolate) { > + NSArray *newEvents = [self interpolatedEvents:eventInfo]; > + [self processEventsArray:newEvents withStartTime:startTime]; > + } else { I don't like here how you generate the interpolated events in the middle of dispatching. I think you should do a first pass over -events and make a new events array with the interpolated events, and then dispatch.
Comment on attachment 291127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291127&action=review >> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:999 >> + NSMutableDictionary *newTouch = [[endTouch mutableCopy] autorelease]; > > Perferable to just explicitly -release after the [newTouches addObject:newTouch] below. Or use RetainPtr and adoptNS()! Then there's no question!
Created attachment 291197 [details] Patch
Attachment 291197 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:1039: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 291198 [details] Patch
Comment on attachment 291198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291198&action=review > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:987 > + double t = (time - startTime) / (endTime - startTime); I would call 't' something like 'progress'. 't' makes me think it's a time. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:997 > + endTouch = [endEnumerator nextObject]; Declare endTouch here. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:998 > + while (endTouch && ([endTouch[HIDEventTouchIDKey] integerValue] != [startTouch[HIDEventTouchIDKey] integerValue])) Pull [startTouch[HIDEventTouchIDKey] integerValue] into a local variable. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:999 > + endTouch = [endEnumerator nextObject]; Indent this line. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:1011 > + if (newTouch[HIDEventXKey] != startTouch[HIDEventXKey]) > + newTouch[HIDEventXKey] = @(interpolations[interpolationType]([startTouch[HIDEventXKey] doubleValue], [endTouch[HIDEventXKey] doubleValue], t)); > + > + if (newTouch[HIDEventYKey] != startTouch[HIDEventYKey]) > + newTouch[HIDEventYKey] = @(interpolations[interpolationType]([startTouch[HIDEventYKey] doubleValue], [endTouch[HIDEventYKey] doubleValue], t)); > + > + if (newTouch[HIDEventPressureKey] != startTouch[HIDEventPressureKey]) > + newTouch[HIDEventPressureKey] = @(interpolations[interpolationType]([startTouch[HIDEventPressureKey] doubleValue], [endTouch[HIDEventPressureKey] doubleValue], t)); Indentation is off in each if block. > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:1028 > +- (NSArray *)expandEvents:(NSArray *)events withStartTime:(CFAbsoluteTime)startTime Blank line above please. > LayoutTests/fast/events/touch/ios/iphone7/force-press-event.html:76 > + output += 'PASS: Generated increasing force<br>'; remove the <br> > LayoutTests/fast/events/touch/ios/iphone7/force-press-event.html:78 > + output += result; result is just "Done": I don't think that adds any useful information.
Created attachment 291206 [details] Patch
Created attachment 291277 [details] Patch
Comment on attachment 291277 [details] Patch Clearing flags on attachment: 291277 Committed r207153: <http://trac.webkit.org/changeset/207153>
All reviewed patches have been landed. Closing bug.
*** Bug 163409 has been marked as a duplicate of this bug. ***