[Android] Disable double-tap gesture detection with fixed page scale

[Android] Disable double-tap gesture detection with fixed page scale

Double-tap gestures, either for zoom or drag zoom, have no effect when the page
scale is fixed.  Disable double-tap gesture detection completely in this case,
allowing fast sequential tapping, eliminating associated gesture detection
delays and suppressing spurious pinch event sequences.

BUG=295906
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228603

[jdduke (slow), 2013-10-11 18:42:40]

PTAL.

@kkimlabs: Why do we always abort "onDoubleTapEvent()" if double-tap drag zoom
is disabled? Don't we still want to support double-tap zoom in that case? Has
that not been an issue because only WebView disables double-tap drag zoom, and
they just happen to not want double-tap zoom either?

[Kibeom Kim (inactive), 2013-10-11 19:04:05]

On 2013/10/11 18:42:40, jdduke wrote:
> PTAL.
> 
> @kkimlabs: Why do we always abort "onDoubleTapEvent()" if double-tap drag zoom
> is disabled? Don't we still want to support double-tap zoom in that case? Has
> that not been an issue because only WebView disables double-tap drag zoom, and
> they just happen to not want double-tap zoom either?

+benm

isdoubleTapDragDisabled() is for Webview disabling all the double-tap related
zoom. (https://chromiumcodereview.appspot.com/23346003) but I believe
enabling/disabling double-tap zoom for webview is already controlled somewhere
else. Could you confirm this Ben? Anyways, I agree that the variable
isdoubleTapDragDisabled() naming at that place is not precise, and confusing.

[jdduke (slow), 2013-10-11 19:07:50]

Hmmm... OK, so it sounds like we just want a general disable/enable doubletap
gestures knob... WebView could override this, and the default behavior will be
to disable when the page scale is fixed?

[jdduke (slow), 2013-10-11 19:09:46]

On 2013/10/11 19:07:50, jdduke wrote:
> Hmmm... OK, so it sounds like we just want a general disable/enable doubletap
> gestures knob... WebView could override this, and the default behavior will be
> to disable when the page scale is fixed?

Nevermind, I think having two knobs is useful, with the end result of
enabling/disabling double-tap support being the union of the two conditions.

[benm (inactive), 2013-10-11 19:14:02]

On 2013/10/11 19:07:50, jdduke wrote:
> Hmmm... OK, so it sounds like we just want a general disable/enable doubletap
> gestures knob... WebView could override this, and the default behavior will be
> to disable when the page scale is fixed?

WebView needs a way to disable all zoom gestures - pinch, double tap drag and
double tap. At the time doing it in onDoubleTap was an appropriate hammer at the
time, but maybe that's not suitable any more...

WebView currently uses AwContents.GestureZoomChangeListener to toggle the
gesture zooming off - see
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

[jdduke (slow), 2013-10-11 19:38:00]

Oh I think the WebView changes are fine, with perhaps a slight rename tweak. 
This change should benefit both WebView and Chrome on Android (rapid tapping
support is now possible).

https://codereview.chromium.org/27043004/diff/11001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/27043004/diff/11001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:500:
} else if (mHasFixedPageScale) {
Hmm... maybe this should be isDoubleTapDisabled()

https://codereview.chromium.org/27043004/diff/11001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:537:
if (isDoubleTapDisabled()) return false;
Maybe assert !isDoubleTapDisabled()?

[jdduke (slow), 2013-10-11 19:39:56]

https://codereview.chromium.org/27043004/diff/11001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/27043004/diff/11001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:500:
} else if (mHasFixedPageScale) {
On 2013/10/11 19:38:01, jdduke wrote:
> Hmm... maybe this should be isDoubleTapDisabled()

benm@: Have you seen any tapping bugs in WebView, perhaps persistent link
highlights or something like that? I think we can fix that by changing as per
the above comment.

[jdduke (slow), 2013-10-14 17:11:05]

techoc@: PTAL for owners.
wangxianzhu@: Would you mind double-checking this logic? Thanks.

[Ted C, 2013-10-14 17:34:08]

OWNERS lgtm w/ nits

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1207:
public void updateDoubleTapSupport(boolean supportDoubleTap) {
javadoc for the new public methods

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java
(right):

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:1025:
final long downTime1 = SystemClock.uptimeMillis();
accidental whitespace addition

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:1137:
Log.i(TAG,"Gesture event received with type id " + type);
intentional to leave this in?

[jdduke (slow), 2013-10-14 17:48:31]

Thanks for review!

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1207:
public void updateDoubleTapSupport(boolean supportDoubleTap) {
On 2013/10/14 17:34:09, Ted C wrote:
> javadoc for the new public methods

Done.

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java
(right):

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:1025:
final long downTime1 = SystemClock.uptimeMillis();
On 2013/10/14 17:34:09, Ted C wrote:
> accidental whitespace addition

Done.

https://codereview.chromium.org/27043004/diff/39001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:1137:
Log.i(TAG,"Gesture event received with type id " + type);
On 2013/10/14 17:34:09, Ted C wrote:
> intentional to leave this in?

Yeah, for parity with MockMotionEventDelegate.

[Xianzhu, 2013-10-14 17:50:07]

lgtm.

[commit-bot: I haz the power, 2013-10-14 18:03:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/27043004/46001

[commit-bot: I haz the power, 2013-10-14 18:21:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jdduke (slow), 2013-10-14 18:27:53]

benm@: OWNER review for android_webview/? Thanks.

[boliu, 2013-10-14 20:22:36]

android_webview lgtm

[commit-bot: I haz the power, 2013-10-14 20:26:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/27043004/46001

[commit-bot: I haz the power, 2013-10-15 03:04:48]

Change committed as 228603