Extend event stream to include interpolated events and add a force press test that uses that interpolation

[Megan Gardner]

Extend event stream to include interpolated events and add a force press test that uses that interpolation

[Megan Gardner]

Created attachment 290999 [details]
Patch

[Tim Horton]

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.

[Megan Gardner]

Created attachment 291001 [details]
Patch

[Wenson Hsieh]

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'?

[Megan Gardner]

Created attachment 291127 [details]
Patch

[Simon Fraser (smfr)]

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.

[Tim Horton]

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!

[Megan Gardner]

Created attachment 291197 [details]
Patch

[WebKit Commit Bot]

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.

[Megan Gardner]

Created attachment 291198 [details]
Patch

[Simon Fraser (smfr)]

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.

[Megan Gardner]

Created attachment 291206 [details]
Patch

[Megan Gardner]

Created attachment 291277 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 291277 [details]
Patch

Clearing flags on attachment: 291277

Committed r207153: <http://trac.webkit.org/changeset/207153>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

*** Bug 163409 has been marked as a duplicate of this bug. ***