Move TouchEvent timeout code to the TouchEventQueue

Move TouchEvent timeout code to the TouchEventQueue

Android currently uses custom TouchEvent timeout logic on unresponsive desktop
sites.  The existence of this timeout has been a trouble spot, so move it to
a common, visible location where it can be examined and used by other platforms.

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

[jdduke (slow), 2013-10-30 21:45:32]

First of all, timeout-based code is always going to be painful, so I hate to
even have to make this change.  However, until we remove the last cases where
Android still uses a timeout, we might as well have the code in a common place. 

wangxianzhu@: Could you double check that the logic from
ContentViewGestureHandler is now captured in the TouchEventQueue?
sadrul@: Would you mind taking a high-level glance over the TEQ changes to see
if the general approach fits?

I originally had the timeout-related code in the router, that would trigger a
Cancel(...) call on the TEQ.  But the general case of handling a Cancel() at any
moment became a bit much.

[Xianzhu, 2013-10-30 22:07:13]

I'm not familiar with TouchEventQueue. What's the specific problem this CL to
resolve? 

Are all the changes merely for the touch timeout? If yes, it looks to me a too
big change for a temporary feature that will be removed.

[jdduke (slow), 2013-10-31 00:46:40]

On 2013/10/30 22:07:13, Xianzhu wrote:
> I'm not familiar with TouchEventQueue. What's the specific problem this CL to
> resolve? 
> 
> Are all the changes merely for the touch timeout? If yes, it looks to me a too
> big change for a temporary feature that will be removed.

Yes, this is only for the timeout, and I'm somewhat inclined to agree with you
that this change is a bit weighty for a feature that we're hoping to remove.  

There's been a general push to shift Android-specific touch handling quirks
down(up?) the pipeline, and this was one such case others thought could benefit
from being more visible.  However, I'm happy to defer this if we're confident we
can rid ourselves of the timeout in a timely (several months?) fashion.

[aelias_OOO_until_Jul13, 2013-10-31 00:48:48]

On 2013/10/30 22:07:13, Xianzhu wrote:
> I'm not familiar with TouchEventQueue. What's the specific problem this CL to
> resolve? 
> 
> Are all the changes merely for the touch timeout? If yes, it looks to me a too
> big change for a temporary feature that will be removed.

Do we have a concrete plan for removal?  What is the criteria?

[Xianzhu, 2013-10-31 16:46:44]

On 2013/10/31 00:48:48, aelias wrote:
> Do we have a concrete plan for removal?  What is the criteria?

Filed crbug.com/313741 to track the criteria and plan for removal.

[Rick Byers, 2013-10-31 16:47:22]

On 2013/10/31 00:48:48, aelias wrote:
> On 2013/10/30 22:07:13, Xianzhu wrote:
> > I'm not familiar with TouchEventQueue. What's the specific problem this CL
to
> > resolve? 
> > 
> > Are all the changes merely for the touch timeout? If yes, it looks to me a
too
> > big change for a temporary feature that will be removed.
> 
> Do we have a concrete plan for removal?  What is the criteria?

I've had some discussions with people suggesting the opposite.  The real problem
with the timeout is we don't have any simple answers for developers hitting it,
other than "understand how blink works and optimize your site to make it less
likely".  That changes once we have touch-action - by opting into touch-action
sites can avoid the need for a timeout entirely.  Then the timeout becomes a
stick to help people opt into the new fast approach.  At that point I'd be OK
enabling the timeout on desktop so that Chrome's behavior is consistent
everywhere.

It was under this potential future that I agreed with Jared when he asked me if
I thought the timeout code should be moved to a common place.

Alternately if there's some plan that we believe will allow us to eliminate the
timeout logic entirely sometime in the next year, then I agree that it's not
worth investing in cleaning it up.

[jdduke (slow), 2013-10-31 16:55:43]

On 2013/10/31 16:47:22, Rick Byers wrote:
> On 2013/10/31 00:48:48, aelias wrote:
> > On 2013/10/30 22:07:13, Xianzhu wrote:
> > > I'm not familiar with TouchEventQueue. What's the specific problem this CL
> to
> > > resolve? 
> > > 
> > > Are all the changes merely for the touch timeout? If yes, it looks to me a
> too
> > > big change for a temporary feature that will be removed.
> > 
> > Do we have a concrete plan for removal?  What is the criteria?
> 
> I've had some discussions with people suggesting the opposite.  The real
problem
> with the timeout is we don't have any simple answers for developers hitting
it,
> other than "understand how blink works and optimize your site to make it less
> likely".  That changes once we have touch-action - by opting into touch-action
> sites can avoid the need for a timeout entirely.  Then the timeout becomes a
> stick to help people opt into the new fast approach.  At that point I'd be OK
> enabling the timeout on desktop so that Chrome's behavior is consistent
> everywhere.
> 
> It was under this potential future that I agreed with Jared when he asked me
if
> I thought the timeout code should be moved to a common place.
> 
> Alternately if there's some plan that we believe will allow us to eliminate
the
> timeout logic entirely sometime in the next year, then I agree that it's not
> worth investing in cleaning it up.

I'm fine with sitting on this while we flesh out what is blocking timeout
removal, as per crbug.com/313741.  Thanks for the feedback.

[jdduke (slow), 2013-11-26 18:55:33]

sadrul@, could you take a look at the changes to TouchEventQueue,
RenderWidgetHostImpl and ImmediateInputRouter?

I don't really like the addition of |OnSwapCompositorFrame()| to the router, so
I could replace that with |OnHasFixedPageScale()| instead.

[jdduke (slow), 2013-12-05 17:10:26]

wangxianzhu@: I've modified the patch to more closely parallel the original code
in ContentViewGestureHandler.  It should (hopefully) be fairly easy to follow in
TouchEventQueue, would you mind taking a look at those changes? Thanks.

[Xianzhu, 2013-12-05 17:48:40]

https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:61: case
PENDING_ACK_ORIGINAL_EVENT:
In original Java method, because we can only have one in-flight event, we can
only send cancel after we receive the ack of the original event. Doesn't the
restriction still apply?

https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:71:
TRACE_EVENT_ASYNC_BEGIN0(
TRACE_EVENT_ASYNC_END0?

[jdduke (slow), 2013-12-05 18:26:04]

https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:61: case
PENDING_ACK_ORIGINAL_EVENT:
On 2013/12/05 17:48:40, Xianzhu wrote:
> In original Java method, because we can only have one in-flight event, we can
> only send cancel after we receive the ack of the original event. Doesn't the
> restriction still apply?

The restriction is important only when we care about the ack order. As it
doesn't make a difference in this case (we're simply waiting on 2 acks and we
don't care what the acks are), it shouldn't make a difference.  I suppose we
could send the cancel here, but I see no harm in sending it immediately upon
timeout.

https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:71:
TRACE_EVENT_ASYNC_BEGIN0(
On 2013/12/05 17:48:40, Xianzhu wrote:
> TRACE_EVENT_ASYNC_END0?

Good catch, fixed.

[Xianzhu, 2013-12-05 18:30:18]

The timeout logic lgtm.

https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:61: case
PENDING_ACK_ORIGINAL_EVENT:
On 2013/12/05 18:26:04, jdduke wrote:
> On 2013/12/05 17:48:40, Xianzhu wrote:
> > In original Java method, because we can only have one in-flight event, we
can
> > only send cancel after we receive the ack of the original event. Doesn't the
> > restriction still apply?
> 
> The restriction is important only when we care about the ack order. As it
> doesn't make a difference in this case (we're simply waiting on 2 acks and we
> don't care what the acks are), it shouldn't make a difference.  I suppose we
> could send the cancel here, but I see no harm in sending it immediately upon
> timeout.

Then pending_ack_state_ could be pending_ack_count_, as the states no longer
indicate which event we are waiting for.

[Xianzhu, 2013-12-05 18:36:16]

On 2013/12/05 18:30:18, Xianzhu wrote:
> The timeout logic lgtm.
> 
>
https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
> File content/browser/renderer_host/input/touch_event_queue.cc (right):
> 
>
https://codereview.chromium.org/48973005/diff/750001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue.cc:61: case
> PENDING_ACK_ORIGINAL_EVENT:
> On 2013/12/05 18:26:04, jdduke wrote:
> > On 2013/12/05 17:48:40, Xianzhu wrote:
> > > In original Java method, because we can only have one in-flight event, we
> can
> > > only send cancel after we receive the ack of the original event. Doesn't
the
> > > restriction still apply?
> > 
> > The restriction is important only when we care about the ack order. As it
> > doesn't make a difference in this case (we're simply waiting on 2 acks and
we
> > don't care what the acks are), it shouldn't make a difference.  I suppose we
> > could send the cancel here, but I see no harm in sending it immediately upon
> > timeout.
> 
> Then pending_ack_state_ could be pending_ack_count_, as the states no longer
> indicate which event we are waiting for.

Wait... I'm afraid the two in-flight events may confuse RenderWidget. Please
double-check that.

[sadrul, 2013-12-06 07:35:17]

LGTM, with some nits

Do you think there could be a browser-test for this?

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:26: typedef
std::map<int, InputEventAckState> AckStates;
This isn't used anywhere?

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:165: explicit
CoalescedWebTouchEvent(const TouchEventWithLatencyInfo& event,
remove explicit

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:291:
renderer_is_consuming_gesture_ = false;
Do you also want to check if touch.event.touchesLength == 1?

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:65: // Toggle whether a
delayed touch ack will cancel and flush the current
s/Toggle/Sets/

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:130: bool
renderer_is_consuming_gesture_;
renderer_is_consuming_touch_?

[jdduke (slow), 2013-12-06 17:17:20]

On 2013/12/06 07:35:17, sadrul wrote:
> Do you think there could be a browser-test for this?

Yes, I was just thinking about this yesterday when I was looking at the
browser-tests you added.  Now that we have a seed browser-test for touch/input,
there are probably a number of end-to-end input sequences we should be
hardening, and this is probably one of them (though I think unit test coverage
here gets us most of the way there).  I'll file a separate bug, as I think there
are some outstanding issues with browser-tests on Android that may need to be
resolved.

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:26: typedef
std::map<int, InputEventAckState> AckStates;
On 2013/12/06 07:35:18, sadrul wrote:
> This isn't used anywhere?

Done.

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:165: explicit
CoalescedWebTouchEvent(const TouchEventWithLatencyInfo& event,
On 2013/12/06 07:35:18, sadrul wrote:
> remove explicit

Done.

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:291:
renderer_is_consuming_gesture_ = false;
On 2013/12/06 07:35:18, sadrul wrote:
> Do you also want to check if touch.event.touchesLength == 1?

Of course!  I guess in theory we could get multiple pressed touches
simultaneously, so I'll add a check that all touches are StatePressed (and we
get at least 1).  Good catch.

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:65: // Toggle whether a
delayed touch ack will cancel and flush the current
On 2013/12/06 07:35:18, sadrul wrote:
> s/Toggle/Sets/

Done.

https://codereview.chromium.org/48973005/diff/790001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:130: bool
renderer_is_consuming_gesture_;
On 2013/12/06 07:35:18, sadrul wrote:
> renderer_is_consuming_touch_?

Hmm, I'd like it to be specific about this particular touch sequence, what about
|touch_gesture|?

[jdduke (slow), 2013-12-06 17:31:06]

jam@: Review for content/public/common? Thanks.

[jam, 2013-12-06 20:33:33]

On 2013/12/06 17:31:06, jdduke wrote:
> jam@: Review for content/public/common? Thanks.

lgtm

[commit-bot: I haz the power, 2013-12-06 21:30:52]

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

[commit-bot: I haz the power, 2013-12-07 03:19:13]

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

[commit-bot: I haz the power, 2013-12-09 15:44:31]

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

[commit-bot: I haz the power, 2013-12-09 22:36:33]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-09 23:05:18]

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

[commit-bot: I haz the power, 2013-12-10 02:18:35]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-10 02:36:37]

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

[commit-bot: I haz the power, 2013-12-10 02:36:46]

Failed to apply patch for
content/browser/renderer_host/input/input_router_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/input/input_router_impl.cc
  Hunk #2 succeeded at 41 (offset 1 line).
  Hunk #3 succeeded at 101 (offset 1 line).
  Hunk #4 FAILED at 197.
  Hunk #5 succeeded at 248 (offset 4 lines).
  Hunk #6 succeeded at 265 (offset 4 lines).
  Hunk #7 succeeded at 638 (offset 14 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
content/browser/renderer_host/input/input_router_impl.cc.rej

Patch:       content/browser/renderer_host/input/input_router_impl.cc
Index: content/browser/renderer_host/input/input_router_impl.cc
diff --git a/content/browser/renderer_host/input/input_router_impl.cc
b/content/browser/renderer_host/input/input_router_impl.cc
index
3f3c16a2b79bc6c950ddd6bf9a7155e1cc17e7a7..b5e78a6ecbd5d8730111855e18e635cd029d4a06
100644
--- a/content/browser/renderer_host/input/input_router_impl.cc
+++ b/content/browser/renderer_host/input/input_router_impl.cc
@@ -7,6 +7,7 @@
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
 #include "content/browser/renderer_host/input/gesture_event_filter.h"
 #include "content/browser/renderer_host/input/input_ack_handler.h"
 #include "content/browser/renderer_host/input/input_router_client.h"
@@ -39,6 +40,21 @@ using blink::WebMouseWheelEvent;
 namespace content {
 namespace {
 
+bool GetTouchAckTimeoutDelayMs(size_t* touch_ack_timeout_delay_ms) {
+  CommandLine* parsed_command_line = CommandLine::ForCurrentProcess();
+  if (!parsed_command_line->HasSwitch(switches::kTouchAckTimeoutDelayMs))
+    return false;
+
+  std::string timeout_string = parsed_command_line->GetSwitchValueASCII(
+      switches::kTouchAckTimeoutDelayMs);
+  size_t timeout_value;
+  if (!base::StringToSizeT(timeout_string, &timeout_value))
+    return false;
+
+  *touch_ack_timeout_delay_ms = timeout_value;
+  return true;
+}
+
 GestureEventWithLatencyInfo MakeGestureEvent(WebInputEvent::Type type,
                                              double timestamp_seconds,
                                              int x,
@@ -84,12 +100,18 @@ InputRouterImpl::InputRouterImpl(IPC::Sender* sender,
       mouse_move_pending_(false),
       mouse_wheel_pending_(false),
       has_touch_handler_(false),
+      touch_ack_timeout_enabled_(false),
+      touch_ack_timeout_delay_ms_(std::numeric_limits<size_t>::max()),
       current_ack_source_(ACK_SOURCE_NONE),
-      touch_event_queue_(new TouchEventQueue(this)),
       gesture_event_filter_(new GestureEventFilter(this, this)) {
   DCHECK(sender);
   DCHECK(client);
   DCHECK(ack_handler);
+  touch_event_queue_.reset(new TouchEventQueue(this));
+  touch_ack_timeout_enabled_ =
+      GetTouchAckTimeoutDelayMs(&touch_ack_timeout_delay_ms_);
+  touch_event_queue_->SetAckTimeoutEnabled(touch_ack_timeout_enabled_,
+                                           touch_ack_timeout_delay_ms_);
 }
 
 InputRouterImpl::~InputRouterImpl() {}
@@ -175,7 +197,7 @@ void InputRouterImpl::SendKeyboardEvent(const
NativeWebKeyboardEvent& key_event,
 
 void InputRouterImpl::SendGestureEvent(
     const GestureEventWithLatencyInfo& gesture_event) {
-  HandleGestureScroll(gesture_event);
+  touch_event_queue_->OnGestureScrollEvent(gesture_event);
 
   if (!IsInOverscrollGesture() &&
       !gesture_event_filter_->ShouldForward(gesture_event)) {
@@ -222,7 +244,6 @@ void InputRouterImpl::SendTouchEventImmediately(
 
 void InputRouterImpl::SendGestureEventImmediately(
     const GestureEventWithLatencyInfo& gesture_event) {
-  HandleGestureScroll(gesture_event);
   FilterAndSendWebInputEvent(gesture_event.event, gesture_event.latency,
false);
 }
 
@@ -240,6 +261,14 @@ bool InputRouterImpl::ShouldForwardTouchEvent() const {
   return has_touch_handler_ || !touch_event_queue_->empty();
 }
 
+void InputRouterImpl::OnViewUpdated(int view_flags) {
+  bool fixed_page_scale = (view_flags & FIXED_PAGE_SCALE) != 0;
+  bool mobile_viewport = (view_flags & MOBILE_VIEWPORT) != 0;
+  touch_event_queue_->SetAckTimeoutEnabled(
+      touch_ack_timeout_enabled_ && !(fixed_page_scale || mobile_viewport),
+      touch_ack_timeout_delay_ms_);
+}
+
 bool InputRouterImpl::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   bool message_is_ok = true;
@@ -595,11 +624,6 @@ void InputRouterImpl::ProcessAckForOverscroll(const
WebInputEvent& event,
       event, (INPUT_EVENT_ACK_STATE_CONSUMED == ack_result));
 }
 
-void InputRouterImpl::HandleGestureScroll(
-    const GestureEventWithLatencyInfo& gesture_event) {
-  touch_event_queue_->OnGestureScrollEvent(gesture_event);
-}
-
 void InputRouterImpl::SimulateTouchGestureWithMouse(
     const MouseEventWithLatencyInfo& event) {
   const WebMouseEvent& mouse_event = event.event;

[commit-bot: I haz the power, 2013-12-10 03:10:26]

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

[commit-bot: I haz the power, 2013-12-10 05:29:09]

Change committed as 239670