Bug 154318 – Allow double tap to zoom in fast-click pages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jon Lee]

rdar://problem/24223767

[Dean Jackson]

Created attachment 271514 [details]
Patch

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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

[Benjamin Poulain]

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.

[Dean Jackson]

Created attachment 271515 [details]
Patch

[Dean Jackson]

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.

[Dean Jackson]

Comment on attachment 271515 [details]
Patch

smfr gave r+ to the first patch. This one is better.

[Jon Lee]

> > 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.

[Dean Jackson]

Created attachment 271517 [details]
Patch

[Dean Jackson]

Committed r196679: <http://trac.webkit.org/changeset/196679>