Add debug flag to WebKitTestRunner to show where touches are being generated
Created attachment 322046 [details] Patch
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?
Created attachment 322049 [details] Patch
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 :)
Created attachment 322055 [details] Patch
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.
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).
Created attachment 322101 [details] Patch
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.
Comment on attachment 322101 [details] Patch r- for reasons in previous patch.
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.
(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.
Offensive part cleaned up and broken out into it's own file in: https://bugs.webkit.org/show_bug.cgi?id=177796
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
<rdar://problem/40105531>