Bug 163161 - Extend event stream to include interpolated events and add a force press test that uses that interpolation
Summary: Extend event stream to include interpolated events and add a force press test...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 163409 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-10-07 18:29 PDT by Megan Gardner
Modified: 2016-10-13 15:53 PDT (History)
7 users (show)

See Also:


Attachments
Patch (20.13 KB, patch)
2016-10-07 18:38 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.32 KB, patch)
2016-10-07 18:51 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.36 KB, patch)
2016-10-10 11:49 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.63 KB, patch)
2016-10-10 17:38 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.62 KB, patch)
2016-10-10 17:42 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.75 KB, patch)
2016-10-10 18:56 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.75 KB, patch)
2016-10-11 10:59 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2016-10-07 18:29:39 PDT
Extend event stream to include interpolated events and add a force press test that uses that interpolation
Comment 1 Megan Gardner 2016-10-07 18:38:35 PDT
Created attachment 290999 [details]
Patch
Comment 2 Tim Horton 2016-10-07 18:47:08 PDT
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.
Comment 3 Megan Gardner 2016-10-07 18:51:08 PDT
Created attachment 291001 [details]
Patch
Comment 4 Wenson Hsieh 2016-10-07 19:19:51 PDT
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'?
Comment 5 Megan Gardner 2016-10-10 11:49:37 PDT
Created attachment 291127 [details]
Patch
Comment 6 Simon Fraser (smfr) 2016-10-10 13:11:26 PDT
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 7 Tim Horton 2016-10-10 13:12:33 PDT
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!
Comment 8 Megan Gardner 2016-10-10 17:38:28 PDT
Created attachment 291197 [details]
Patch
Comment 9 WebKit Commit Bot 2016-10-10 17:39:25 PDT
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.
Comment 10 Megan Gardner 2016-10-10 17:42:34 PDT
Created attachment 291198 [details]
Patch
Comment 11 Simon Fraser (smfr) 2016-10-10 18:18:57 PDT
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.
Comment 12 Megan Gardner 2016-10-10 18:56:41 PDT
Created attachment 291206 [details]
Patch
Comment 13 Megan Gardner 2016-10-11 10:59:14 PDT
Created attachment 291277 [details]
Patch
Comment 14 WebKit Commit Bot 2016-10-11 11:41:24 PDT
Comment on attachment 291277 [details]
Patch

Clearing flags on attachment: 291277

Committed r207153: <http://trac.webkit.org/changeset/207153>
Comment 15 WebKit Commit Bot 2016-10-11 11:41:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Tim Horton 2016-10-13 15:53:23 PDT
*** Bug 163409 has been marked as a duplicate of this bug. ***