rdar://problem/24223767
Created attachment 271514 [details] Patch
Comment on attachment 271514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271514&action=review > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:941 > + [_fastDoubleTapGestureRecognizer setEnabled:NO]; > + [_fastDoubleTapGestureRecognizer setEnabled:YES]; Really?
Comment on attachment 271514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271514&action=review >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:941 >> + [_fastDoubleTapGestureRecognizer setEnabled:YES]; > > Really? Maybe add a comment to make it clear this isn't a typo. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1342 > +- (void)_didNotHandleTapAsClick:(const WebCore::IntPoint&)point Method name doesn't obviously read as a setter (it sounds like a getter), and doesn't imply that things like data detectors will kick in. It needs to sound more imperative. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1405 > +- (void)_fastDoubleTapRecognized:(UITapGestureRecognizer *)gestureRecognizer "fast double tap" makes it sound like there's another kind of double tap. is it more like "interruptible double tap"?
View in context: https://bugs.webkit.org/attachment.cgi?id=271514&action=review Some comments. I am not sure to understand this patch. I'll have to talk to Jon. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:158 > + BOOL _isDoubleTapPending; This must be reset in [-cleanupInteraction] or [-setupInteraction]. Otherwise, you may keep a bogus state after a crash. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1353 > + if (!_isDoubleTapPending) > + return; > + > + _smartMagnificationController->handleSmartMagnificationGesture(_lastInteractionLocation); > + _isDoubleTapPending = NO; Is it expected to show the data detector AND zoom? I *think* zooming dismisses some of the pop overs. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1403 > +- (void)_resetIsDoubleTapPending > +{ > + _isDoubleTapPending = NO; > +} Looks like adding complexity. All the lines with [self _resetIsDoubleTapPending] could be "_isDoubleTapPending = NO;". > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1407 > + _lastInteractionLocation = gestureRecognizer.location; This is a bit odd. As far as I know, "lastInteractionLocation" previously always referred to taps/clicks.
Created attachment 271515 [details] Patch
Comment on attachment 271514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271514&action=review >>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:941 >>> + [_fastDoubleTapGestureRecognizer setEnabled:YES]; >> >> Really? > > Maybe add a comment to make it clear this isn't a typo. Turns out I can remove this. >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1342 >> +- (void)_didNotHandleTapAsClick:(const WebCore::IntPoint&)point > > Method name doesn't obviously read as a setter (it sounds like a getter), and doesn't imply that things like data detectors will kick in. It needs to sound more imperative. This is an existing name that signals an incoming message from the Web Process. It's an event handler, like _singleTapRecognized, which can also do a number of things. >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1405 >> +- (void)_fastDoubleTapRecognized:(UITapGestureRecognizer *)gestureRecognizer > > "fast double tap" makes it sound like there's another kind of double tap. is it more like "interruptible double tap"? I definitely agree here. It actually means the double tap that is recognized when we're in fast click mode.
Comment on attachment 271515 [details] Patch smfr gave r+ to the first patch. This one is better.
> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1407 > > + _lastInteractionLocation = gestureRecognizer.location; > > This is a bit odd. As far as I know, "lastInteractionLocation" previously > always referred to taps/clicks. The other double tap recognizer does the same.
Created attachment 271517 [details] Patch
Committed r196679: <http://trac.webkit.org/changeset/196679>