Bug 177583 – Add debug flag to WebKitTestRunner to show where touches are being generated

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Add debug flag to WebKitTestRunner to show where touches are being generated

[Megan Gardner]

Created attachment 322046 [details]
Patch

[Tim Horton]

Comment on attachment 322046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322046&action=review

> Tools/WebKitTestRunner/Options.cpp:139
>      optionList.append(Option("--show-webview", "Show the WebView during test runs (for Debugging)", handleOptionShowWebView));
> +    optionList.append(Option("--show-touches", "Show the touches during test runs (for Debugging)", handleOptionShowTouches));

both of these "debugging" should be lower case

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:84
> +static const NSUInteger debugTouchDotFrame = debugTouchDotRadius*2;

Spaces around operators!

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:157
> +-(BOOL)pointInside:(CGPoint)point withEvent:(UIEvent *)event { // Never react to touches

Can't you just view.userInteractionEnabled=NO? Then you don't even need this subclass.

Also, brace should be on the next line, and remove the comment.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:217
> +        self.debugTouchViews = @[[[DebugTouchView alloc] initWithFrame:CGRectMake(10, 10, debugTouchDotFrame, debugTouchDotFrame)],

Can this be a loop that uses maxTouchCount, instead?

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:322
> +- (void)updateDebugUI:(UInt)index withPoint:(CGPoint)point isTouching:(BOOL)isTouching

should probably be updateDebugIndicatorForTouch or something, not "DebugUI"

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:569
> +        if (self.shouldShowTouches) {

Why do some of these not use the helper? Then you could put the initDebugViews call inside the helper?

[Megan Gardner]

Created attachment 322049 [details]
Patch

[Tim Horton]

Comment on attachment 322049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322049&action=review

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:162
> +    BOOL _shouldInitDebugViews;

Do we need this, or can we just check debugTouchViews?

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:200
> +        if ([[UIApplication sharedApplication] keyWindow])
> +            [self initDebugViews];
> +        else

Is there any reason to eagerly do it at this point? We don't support dynamically changing it (e.g. nothing ever removes views), which is fine, so shouldn't we just depend on the lazy loading mechanism always?

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:205
> +- (void)initDebugViews

This is now an "-IfNeeded"

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:207
> +    if (self.shouldShowTouches && _shouldInitDebugViews) {

Should be an early return :)

[Megan Gardner]

Created attachment 322055 [details]
Patch

[Tim Horton]

Comment on attachment 322055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322055&action=review

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:84
> +static const NSUInteger debugTouchDotFrame = debugTouchDotRadius * 2;

“Frame”? Isn’t this a size? Frame is usually a rect.

[Wenson Hsieh]

Comment on attachment 322055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322055&action=review

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:201
> +        UIView *newView = [[UIView alloc] initWithFrame:CGRectMake(10, 10, debugTouchDotFrame, debugTouchDotFrame)];

It looks like newView is leaked here. Perhaps we should use something like:

  auto newView = adoptNS([[UIView alloc] initWithFrame:CGRectMake(10, 10, debugTouchDotSize, debugTouchDotSize)]);

(or alternately, we can just manually call -release)

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:209
> +    self.debugTouchViews = [NSArray arrayWithArray:debugViews];

I think we can just do `self.debugTouchViews = debugViews;` here? (Since an NSMutableArray is a kind of NSArray).

[Megan Gardner]

Created attachment 322101 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 322055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322055&action=review

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:204
> +        newView.backgroundColor = [UIColor colorWithRed:1.0-i*.05 green:0.0 blue:1.0-i*.05 alpha:0.5];

Spaces around operators.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:207
> +        [[[UIApplication sharedApplication] keyWindow] addSubview:newView];

Uh, this seems a bit random. I think we should explicitly pass a view to add them to so we know the coordinate system is correct.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:299
> +- (void)updateDebugIndicatorForTouch:(UInt)index withPoint:(CGPoint)point isTouching:(BOOL)isTouching

Point In what coordinate system? Method name should make that clear.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:338
> +        if (self.shouldShowTouches)
> +            [self updateDebugIndicatorForTouch:[touchInfo[HIDEventTouchIDKey] intValue] withPoint:CGPointMake([touchInfo[HIDEventXKey] floatValue], [touchInfo[HIDEventYKey] floatValue]) isTouching:(BOOL)touch];

I don't think HIDEventGenerator should be in the business of doing UIView stuff. I think this code should move somewhere else.

[Simon Fraser (smfr)]

Comment on attachment 322101 [details]
Patch

r- for reasons in previous patch.

[Megan Gardner]

Comment on attachment 322055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322055&action=review

>> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:207
>> +        [[[UIApplication sharedApplication] keyWindow] addSubview:newView];
> 
> Uh, this seems a bit random. I think we should explicitly pass a view to add them to so we know the coordinate system is correct.

Maybe I don't understand HID stuff, but I thought by this time they were in global coordinate space? If that's not the case (and it sounds like it's not), then how can we determine what view these touches were meant for?

>> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:338
>> +            [self updateDebugIndicatorForTouch:[touchInfo[HIDEventTouchIDKey] intValue] withPoint:CGPointMake([touchInfo[HIDEventXKey] floatValue], [touchInfo[HIDEventYKey] floatValue]) isTouching:(BOOL)touch];
> 
> I don't think HIDEventGenerator should be in the business of doing UIView stuff. I think this code should move somewhere else.

Where do you suggest? We do a lot of work here to determine where the touches are going to be, and this is just for debugging. It's not even on all the time, only the you actually turn it on.

[Simon Fraser (smfr)]

(In reply to Megan Gardner from comment #11)
> Comment on attachment 322055 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322055&action=review
> 
> >> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:207
> >> +        [[[UIApplication sharedApplication] keyWindow] addSubview:newView];
> > 
> > Uh, this seems a bit random. I think we should explicitly pass a view to add them to so we know the coordinate system is correct.
> 
> Maybe I don't understand HID stuff, but I thought by this time they were in
> global coordinate space? If that's not the case (and it sounds like it's
> not), then how can we determine what view these touches were meant for?

The issue here isn't necessarily that you're not adding the dots in the view that the events are targeted at, but that you have no idea about the geometry of [[UIApplication sharedApplication] keyWindow]. Maybe it's some small view owned by UIKit at some point, in which case your dots are going to show in the wrong place.

I'd suggesting adding a hosting view whose geometry you can rely on, and maybe do this in some UIApplication override?

> >> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:338
> >> +            [self updateDebugIndicatorForTouch:[touchInfo[HIDEventTouchIDKey] intValue] withPoint:CGPointMake([touchInfo[HIDEventXKey] floatValue], [touchInfo[HIDEventYKey] floatValue]) isTouching:(BOOL)touch];
> > 
> > I don't think HIDEventGenerator should be in the business of doing UIView stuff. I think this code should move somewhere else.
> 
> Where do you suggest? We do a lot of work here to determine where the
> touches are going to be, and this is just for debugging. It's not even on
> all the time, only the you actually turn it on.

It's not the runtime cost that I object to, but the code cleanliness.

[Megan Gardner]

Offensive part cleaned up and broken out into it's own file in:
https://bugs.webkit.org/show_bug.cgi?id=177796

[Megan Gardner]

https://trac.webkit.org/changeset/222631/webkit

This was committed before Simon's R-. The concerns he raised were fixed in 
https://bugs.webkit.org/show_bug.cgi?id=177796

[Radar WebKit Bug Importer]

<rdar://problem/40105531>