Make tap gesture ignore its ack disposition.

Make tap gesture ignore its ack disposition.

BUG=302852, 275611
TEST=ImmediateInputRouterTest.GestureTypesIgnoringAck

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

[commit-bot: I haz the power, 2013-11-26 15:09:28]

No comments yet.

[tdresser, 2013-11-26 16:43:21]

jdduke@, can you take a look at 
content/browser/renderer_host/input/immediate_input_router_unittest.cc and 
content/common/input/web_input_event_traits.cc?

jam@, can you take a look at 
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc?

[jdduke (slow), 2013-11-26 17:10:56]

On 2013/11/26 16:43:21, tdresser wrote:
> jdduke@, can you take a look at 
> content/browser/renderer_host/input/immediate_input_router_unittest.cc and 
> content/common/input/web_input_event_traits.cc?
> 
> jam@, can you take a look at 
> content/browser/web_contents/touch_editable_impl_aura_browsertest.cc?

I'd like to see more evidence that this is a big enough win to justify the
potentially unpredictable user experience.  A Tap often has meaningful
side-effects that are relevant to subsequent gestures like scroll.

[tdresser, 2013-11-26 17:43:02]

I believe that rbyers@ felt this was worth doing.

In terms of unpredictability, I did some testing with taps which spawned new
scrollable divs, and didn't feel like the behavior felt unreasonable.

Are there specific cases you're worried about? It feels to me like any time we
can perform scroll instead of blocking completely, it's a pretty big win.

[jdduke (slow), 2013-11-26 17:57:47]

On 2013/11/26 17:43:02, tdresser wrote:
> I believe that rbyers@ felt this was worth doing.
> 
> In terms of unpredictability, I did some testing with taps which spawned new
> scrollable divs, and didn't feel like the behavior felt unreasonable.
> 
> Are there specific cases you're worried about? It feels to me like any time we
> can perform scroll instead of blocking completely, it's a pretty big win.

New scrollable divs with a heavy main thread? I'd like to get aelias@ view on
this before we roll it out.  Unfortunately he's out of town, but I don't see any
reason to rush this.

[tdresser, 2013-11-26 18:02:00]

On 2013/11/26 17:57:47, jdduke wrote:
> On 2013/11/26 17:43:02, tdresser wrote:
> > I believe that rbyers@ felt this was worth doing.
> > 
> > In terms of unpredictability, I did some testing with taps which spawned new
> > scrollable divs, and didn't feel like the behavior felt unreasonable.
> > 
> > Are there specific cases you're worried about? It feels to me like any time
we
> > can perform scroll instead of blocking completely, it's a pretty big win.
> 
> New scrollable divs with a heavy main thread? I'd like to get aelias@ view on
> this before we roll it out.  Unfortunately he's out of town, but I don't see
any
> reason to rush this.

Sure, sounds good.

[tdresser, 2013-12-10 21:27:20]

Looping in aelias@ to get his opinion.

[aelias_OOO_until_Jul13, 2013-12-11 01:21:06]

This lgtm.  I can't think of a scenario where this is meaningfully harmful, and
side effects of taps can happen async after the disposition returns so the
current behavior isn't really buying us any hard ordering guarantees.

[tdresser, 2013-12-11 18:35:29]

On 2013/12/11 01:21:06, aelias wrote:
> This lgtm.  I can't think of a scenario where this is meaningfully harmful,
and
> side effects of taps can happen async after the disposition returns so the
> current behavior isn't really buying us any hard ordering guarantees.

jdduke@, what do you think?

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

On 2013/12/11 18:35:29, tdresser wrote:
> On 2013/12/11 01:21:06, aelias wrote:
> > This lgtm.  I can't think of a scenario where this is meaningfully harmful,
> and
> > side effects of taps can happen async after the disposition returns so the
> > current behavior isn't really buying us any hard ordering guarantees.
> 
> jdduke@, what do you think?

Ship it.

[tdresser, 2013-12-11 19:27:42]

On 2013/12/11 19:19:10, jdduke wrote:
> On 2013/12/11 18:35:29, tdresser wrote:
> > On 2013/12/11 01:21:06, aelias wrote:
> > > This lgtm.  I can't think of a scenario where this is meaningfully
harmful,
> > and
> > > side effects of taps can happen async after the disposition returns so the
> > > current behavior isn't really buying us any hard ordering guarantees.
> > 
> > jdduke@, what do you think?
> 
> Ship it.

jam@, can you take a look at the touch_editable_impl_aura_browsertest.cc change?

[jam, 2013-12-13 17:35:19]

On 2013/12/11 19:27:42, tdresser wrote:
> On 2013/12/11 19:19:10, jdduke wrote:
> > On 2013/12/11 18:35:29, tdresser wrote:
> > > On 2013/12/11 01:21:06, aelias wrote:
> > > > This lgtm.  I can't think of a scenario where this is meaningfully
> harmful,
> > > and
> > > > side effects of taps can happen async after the disposition returns so
the
> > > > current behavior isn't really buying us any hard ordering guarantees.
> > > 
> > > jdduke@, what do you think?
> > 
> > Ship it.
> 
> jam@, can you take a look at the touch_editable_impl_aura_browsertest.cc
change?

lgtm sorry I just saw this

[commit-bot: I haz the power, 2013-12-13 17:37:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/84653006/40001

[commit-bot: I haz the power, 2013-12-13 17:37:18]

Failed to apply patch for
content/browser/renderer_host/input/immediate_input_router_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: content/browser/renderer_host/input/immediate_input_router_unittest.cc
  |diff --git
a/content/browser/renderer_host/input/immediate_input_router_unittest.cc
b/content/browser/renderer_host/input/immediate_input_router_unittest.cc
  |index
dfd1ee026b80cbab166ac975b9cd21eb00bd3d89..afd2d2eb910d1ebbc5b2d5b28babae06bf81caf0
100644
  |--- a/content/browser/renderer_host/input/immediate_input_router_unittest.cc
  |+++ b/content/browser/renderer_host/input/immediate_input_router_unittest.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:      
content/browser/renderer_host/input/immediate_input_router_unittest.cc
Index: content/browser/renderer_host/input/immediate_input_router_unittest.cc
diff --git
a/content/browser/renderer_host/input/immediate_input_router_unittest.cc
b/content/browser/renderer_host/input/immediate_input_router_unittest.cc
index
dfd1ee026b80cbab166ac975b9cd21eb00bd3d89..afd2d2eb910d1ebbc5b2d5b28babae06bf81caf0
100644
--- a/content/browser/renderer_host/input/immediate_input_router_unittest.cc
+++ b/content/browser/renderer_host/input/immediate_input_router_unittest.cc
@@ -681,32 +681,41 @@ TEST_F(ImmediateInputRouterTest,
GestureTypesIgnoringAckInterleaved) {
 // Test that GestureShowPress events don't get out of order due to
 // ignoring their acks.
 TEST_F(ImmediateInputRouterTest, GestureShowPressIsInOrder) {
-  SimulateGestureEvent(WebInputEvent::GestureTap,
+  // GesturePinchBegin ignores its ack.
+  SimulateGestureEvent(WebInputEvent::GesturePinchBegin,
                        WebGestureEvent::Touchscreen);
+  EXPECT_EQ(1U, GetSentMessageCountAndResetSink());
+  EXPECT_EQ(1U, ack_handler_->GetAndResetAckCount());
 
+  // GesturePinchBegin waits for an ack.
+  SimulateGestureEvent(WebInputEvent::GesturePinchUpdate,
+                       WebGestureEvent::Touchscreen);
   EXPECT_EQ(1U, GetSentMessageCountAndResetSink());
   EXPECT_EQ(0U, ack_handler_->GetAndResetAckCount());
 
   SimulateGestureEvent(WebInputEvent::GestureShowPress,
                        WebGestureEvent::Touchscreen);
-
   EXPECT_EQ(0U, GetSentMessageCountAndResetSink());
   // The ShowPress, though it ignores ack, is still stuck in the queue
-  // behind the Tap which requires an ack.
+  // behind the PinchUpdate which requires an ack.
   EXPECT_EQ(0U, ack_handler_->GetAndResetAckCount());
 
   SimulateGestureEvent(WebInputEvent::GestureShowPress,
                        WebGestureEvent::Touchscreen);
-
   EXPECT_EQ(0U, GetSentMessageCountAndResetSink());
   // ShowPress has entered the queue.
   EXPECT_EQ(0U, ack_handler_->GetAndResetAckCount());
 
-  SendInputEventACK(WebInputEvent::GestureTap,
+  // This ack is ignored.
+  SendInputEventACK(WebInputEvent::GesturePinchBegin,
                     INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
+  EXPECT_EQ(0U, GetSentMessageCountAndResetSink());
+  EXPECT_EQ(0U, ack_handler_->GetAndResetAckCount());
 
+  SendInputEventACK(WebInputEvent::GesturePinchUpdate,
+                    INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
   // Now that the Tap has been ACKed, the ShowPress events should receive
-  // synthetics acks, and fire immediately.
+  // synthetic acks, and fire immediately.
   EXPECT_EQ(2U, GetSentMessageCountAndResetSink());
   EXPECT_EQ(3U, ack_handler_->GetAndResetAckCount());
 }

[commit-bot: I haz the power, 2013-12-13 18:46:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/84653006/60001

[commit-bot: I haz the power, 2013-12-13 19:22:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/84653006/60001

[commit-bot: I haz the power, 2013-12-14 04:14:06]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-06 15:34:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/84653006/60001

[commit-bot: I haz the power, 2014-01-07 01:03:51]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[tdresser, 2014-01-08 18:52:39]

jdduke, does this seems like a reasonable solution?

[aurimas (slooooooooow), 2014-01-08 19:00:33]

https://codereview.chromium.org/84653006/diff/330001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/84653006/diff/330001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:331:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
This does not look like fixing tests, it looks like you are changing threads for
production code. I think you should change the tests to call ImeAdapter on
UiThread instead.

[jdduke (slow), 2014-01-08 19:10:59]

Hmm, I'm not sure we want the ImeAdapter to be doing the posting.  Really, the
burden should be on the testing framework.

[jdduke (slow), 2014-01-08 19:11:43]

On 2014/01/08 19:10:59, jdduke wrote:
> Hmm, I'm not sure we want the ImeAdapter to be doing the posting.  Really, the
> burden should be on the testing framework.

Oops, missed aurimas@'s post.  Yeah, I'll send over a version that makes the
changes to the tests.

[tdresser, 2014-01-08 20:50:30]

On 2014/01/08 19:11:43, jdduke wrote:
> On 2014/01/08 19:10:59, jdduke wrote:
> > Hmm, I'm not sure we want the ImeAdapter to be doing the posting.  Really,
the
> > burden should be on the testing framework.
> 
> Oops, missed aurimas@'s post.  Yeah, I'll send over a version that makes the
> changes to the tests.

aurimas, does this look good?

[aurimas (slooooooooow), 2014-01-08 21:02:51]

LGTM after fixing the nit

https://codereview.chromium.org/84653006/diff/430001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/84653006/diff/430001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:356:

This call is unused.

[tdresser, 2014-01-08 21:25:14]

On 2014/01/08 21:02:51, aurimas wrote:
> LGTM after fixing the nit
> 
>
https://codereview.chromium.org/84653006/diff/430001/content/public/android/j...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
> (right):
> 
>
https://codereview.chromium.org/84653006/diff/430001/content/public/android/j...
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:356:
> 
> This call is unused.

Nit fixed, though rietveld is giving me an old chunk mismatch error, so I can't
actually mark it as such...

[commit-bot: I haz the power, 2014-01-08 21:31:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/84653006/720001

[commit-bot: I haz the power, 2014-01-09 00:48:04]

Change committed as 243713