[Android] Properly handle cancelled scroll events

[Android] Properly handle cancelled scroll events

If the gesture handler receives an ACTION_CANCEL event during a scroll,
it should tell native to end scrolling.

Adds tests that the proper events are sent to native in both the normal
scroll sequence and the cancelled sequence.

BUG=b/10892868

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

[cjhopman, 2013-10-14 17:52:26]

tedchoc: *

[Ted C, 2013-10-14 20:05:37]

Looks like the action_up test failed on the bots.  Is something missing?

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ZoomManager.java
(right):

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ZoomManager.java:127:
|| event.getActionMasked() == MotionEvent.ACTION_CANCEL)) return false;
braces needed since the conditional no longer fits on one line

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java
(right):

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:607:
public void testScrollEventSequence() throws Exception {
should specify this as action_up in the name to be consistent with cancelled
below

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:623:

remove extra blank line

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:625:
private void doScrollEventSequence(int endActionType) throws Exception {
The name of this method seems somewhat unclear to me.  I think something like:

assertScrollEndSentForEndingActionType

Just so it doesn't look like some generic scroll method.

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:655:
downTime, eventTime + 100, endActionType, scrollToX, scrollToY, 0);
Although it probably doesn't matter, but should you be incrementing the event
time by something larger than what you do for ACTION_MOVE above?

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:667:

remove extra blank line

[cjhopman, 2013-10-14 20:37:08]

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ZoomManager.java
(right):

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ZoomManager.java:127:
|| event.getActionMasked() == MotionEvent.ACTION_CANCEL)) return false;
On 2013/10/14 20:05:38, Ted C wrote:
> braces needed since the conditional no longer fits on one line

Done.

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java
(right):

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:607:
public void testScrollEventSequence() throws Exception {
On 2013/10/14 20:05:38, Ted C wrote:
> should specify this as action_up in the name to be consistent with cancelled
> below

Done.

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:623:

On 2013/10/14 20:05:38, Ted C wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:625:
private void doScrollEventSequence(int endActionType) throws Exception {
On 2013/10/14 20:05:38, Ted C wrote:
> The name of this method seems somewhat unclear to me.  I think something like:
> 
> assertScrollEndSentForEndingActionType
> 
> Just so it doesn't look like some generic scroll method.

How's this?

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:655:
downTime, eventTime + 100, endActionType, scrollToX, scrollToY, 0);
On 2013/10/14 20:05:38, Ted C wrote:
> Although it probably doesn't matter, but should you be incrementing the event
> time by something larger than what you do for ACTION_MOVE above?

It's pretty common that the framework sends a final ACTION_MOVE before the
ACTION_UP with the same time, so this should be fine.

https://codereview.chromium.org/27220002/diff/4001/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:667:

On 2013/10/14 20:05:38, Ted C wrote:
> remove extra blank line

Done.

[Ted C, 2013-10-14 21:12:32]

lgtm

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/27220002/17001

[commit-bot: I haz the power, 2013-10-14 22:13:25]

Failed to trigger a try job on win7_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-10-14 22:13:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/27220002/33001

[commit-bot: I haz the power, 2013-10-15 01:23:14]

Change committed as 228583