Provided batched input delivery with a BufferedInputRouter

content/browser/renderer_host/input/buffered_input_router.cc

Index: content/browser/renderer_host/input/buffered_input_router.cc
diff --git a/content/browser/renderer_host/input/buffered_input_router.cc b/content/browser/renderer_host/input/buffered_input_router.cc
new file mode 100644
index 0000000000000000000000000000000000000000..73f15dcc7174aa8bd563294dda30c8b5b012db94
--- /dev/null
+++ b/content/browser/renderer_host/input/buffered_input_router.cc
@@ -0,0 +1,434 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/renderer_host/input/buffered_input_router.h"
+
+#include "base/auto_reset.h"
+#include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/browser/renderer_host/render_widget_host_impl.h"
+#include "content/common/input/input_event_utils.h"
+#include "content/common/input_messages.h"
+#include "content/common/view_messages.h"
+#include "content/public/browser/native_web_keyboard_event.h"
+#include "content/public/browser/user_metrics.h"
+
+using base::Time;
+using base::TimeDelta;
+using WebKit::WebGestureEvent;
+using WebKit::WebInputEvent;
+using WebKit::WebKeyboardEvent;
+using WebKit::WebMouseEvent;
+using WebKit::WebMouseWheelEvent;
+using WebKit::WebTouchEvent;
+
+namespace content {
+
+namespace {
+
+InputEventAckState FromState(InputEventState state) {
+  switch (state) {
+    case INPUT_EVENT_UNHANDLED:
+      return INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
+    case INPUT_EVENT_IMPL_THREAD_ABSORBED:
+      return INPUT_EVENT_ACK_STATE_CONSUMED;
+    case INPUT_EVENT_IMPL_THREAD_NO_CONSUMER_EXISTS:
+      return INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS;
+    case INPUT_EVENT_IMPL_THREAD_BOUNCE_TO_MAIN:
+      return INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
+    case INPUT_EVENT_IMPL_THREAD_COULD_NOT_DELIVER:
+      return INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
+    case INPUT_EVENT_MAIN_THREAD_ABSORBED:
+      return INPUT_EVENT_ACK_STATE_CONSUMED;
+    case INPUT_EVENT_MAIN_THREAD_PREVENT_DEFAULTED:
+      return INPUT_EVENT_ACK_STATE_CONSUMED;
+    case INPUT_EVENT_MAIN_THREAD_NOT_PREVENT_DEFAULTED:
+      return INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
+    case INPUT_EVENT_MAIN_THREAD_NO_HANDLER_EXISTS:
+      return INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS;
+    case INPUT_EVENT_MAIN_THREAD_COULD_NOT_DELIVER:
+      return INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
+  }
+
+  NOTREACHED();
+  return INPUT_EVENT_ACK_STATE_UNKNOWN;
+}
+
+} // namespace
+
+BufferedInputRouter::BufferedInputRouter(RenderProcessHost* process,
+                                         InputRouterClient* client,
+                                         int routing_id)
+    : client_(client),
+      process_(process),
+      routing_id_(routing_id),
+      input_queue_(new InputQueue(this)),

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

Passing partially constructed "this" pointer here seems slightly sketchy, I'd
prefer InputQueue was created within the {} section of the constructor.

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+      queued_gesture_count_(0),
+      has_touch_handler_(false),
+      queued_touch_count_(0),
+      input_queue_override_(NULL),
+      last_input_id_(0),
+      in_flight_packet_id_(0),
+      // TODO(jdduke): Determine a sensible value...
+      delayed_packet_timeout_ms_(10) {}
+
+BufferedInputRouter::~BufferedInputRouter() {}
+
+void BufferedInputRouter::Flush() {
+  TRACE_EVENT0("input", "BufferedInputRouter::Flush");
+  DCHECK(in_flight_packet_id_ == 0);

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

DCHECK_EQ(0, in_flight_packet_id_);

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+
+  in_flight_packet_id_ = 0;

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

Delete this line, you already DCHECKed it above.  It's Chromium practice to
avoid this kind of pseudo-fault-tolerance, it often just makes failure modes
more subtle and complex when they do occur.

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+  input_queue_->FlushEventsInCurrentFrame();
+  if (in_flight_packet_id_) {
+    StartDelayedPacketMonitorTimeout(
+        TimeDelta::FromMilliseconds(delayed_packet_timeout_ms_));
+  }
+}
+
+bool BufferedInputRouter::SendInput(IPC::Message* message) {
+  scoped_ptr<IPC::Message> scoped_message(message);

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

Why take ownership?  Why not let the client manage lifetime?

If you're going to take ownership, the argument should be a scoped_ptr instead
of doing it in the implementation.

[jdduke (slow), 2013/08/13 15:29:48]

This was originally an OVERRIDE of IPC::Sender::Send(), but without those
signature constraints the scoped_ptr makes sense.

+  if (IPC_MESSAGE_ID_CLASS(message->type()) != InputMsgStart) {

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

These look like they should be DCHECK_EQ/DCHECK_NE.  Get rid of the "return
false"s.

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+    NOTREACHED() << "BufferedInputRouter only handles input-related messages.";
+    return false;
+  }
+  if (message->type() == InputMsg_HandleEventPacket::ID) {
+    NOTREACHED() << "WebInputEvents should never be sent via SendInput.";
+    return false;
+  }
+
+  input_queue_->QueueEvent(InputEvent(NewInputID(),
+                                      INPUT_EVENT_ONE_WAY,
+                                      *scoped_message), NULL);
+  return true;
+}
+
+void BufferedInputRouter::SendMouseEvent(
+    const MouseEventWithLatencyInfo& mouse_event) {
+  if (!client_->OnSendMouseEvent(mouse_event))
+    return;
+  QueueWebEvent(mouse_event.event, mouse_event.latency, false, false);
+}
+
+void BufferedInputRouter::SendWheelEvent(
+    const MouseWheelEventWithLatencyInfo& wheel_event) {
+  if (!client_->OnSendWheelEvent(wheel_event))
+    return;
+  QueueWebEvent(wheel_event.event, wheel_event.latency, false, false);
+}
+
+void BufferedInputRouter::SendKeyboardEvent(
+    const NativeWebKeyboardEvent& key_event,
+    const ui::LatencyInfo& latency_info) {
+  bool is_shortcut = false;
+  if (!client_->OnSendKeyboardEvent(key_event, latency_info, &is_shortcut))
+    return;
+  int64 event_id = QueueWebEvent(key_event, latency_info, is_shortcut, false);
+  if (event_id) {
+    DCHECK(queued_key_map_.find(event_id) == queued_key_map_.end());
+    queued_key_map_[event_id] = key_event;
+  }
+}
+
+void BufferedInputRouter::SendGestureEvent(
+    const GestureEventWithLatencyInfo& gesture_event) {
+  if (!client_->OnSendGestureEvent(gesture_event))
+    return;
+  if (QueueWebEvent(gesture_event.event, gesture_event.latency, false, false))
+    ++queued_gesture_count_;
+}
+
+void BufferedInputRouter::SendTouchEvent(
+    const TouchEventWithLatencyInfo& touch_event) {
+  if (!client_->OnSendTouchEvent(touch_event))
+    return;
+  if (QueueWebEvent(touch_event.event, touch_event.latency, false, true))
+    ++queued_touch_count_;
+}
+
+void BufferedInputRouter::SendMouseEventImmediately(
+    const MouseEventWithLatencyInfo& mouse_event) {
+  if (!client_->OnSendMouseEventImmediately(mouse_event))
+    return;
+  QueueWebEvent(mouse_event.event, mouse_event.latency, false, false);
+}
+
+void BufferedInputRouter::SendTouchEventImmediately(
+    const TouchEventWithLatencyInfo& touch_event) {
+  if (!client_->OnSendTouchEventImmediately(touch_event))
+    return;
+  QueueWebEvent(touch_event.event, touch_event.latency, false, false);
+}
+
+void BufferedInputRouter::SendGestureEventImmediately(
+    const GestureEventWithLatencyInfo& gesture_event) {
+  if (!client_->OnSendGestureEventImmediately(gesture_event))
+    return;
+  QueueWebEvent(gesture_event.event, gesture_event.latency, false, false);
+}
+
+void BufferedInputRouter::Deliver(const EventPacket& packet) {
+  TRACE_EVENT2("input", "BufferedInputRouter::DeliverPacket",
+               "id", packet.id,
+               "events", packet.events.size());
+  // TODO(jdduke): How should we handle this case? Clear the existing queue?

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

I think you should mandate that the process has a longer lifetime than your
class.  Don't try to handle it in the implementation.

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+  if (!process_->HasConnection())
+    return;
+
+  DCHECK(packet.id && !in_flight_packet_id_);

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

Split this into two separate DCHECKs (the output will be less ambiguous)

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+  in_flight_packet_id_ = packet.id;

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

Can you put these two lines after the Send() and just "return;" if it fails?

[jdduke (slow), 2013/08/13 15:29:48]

Yeah, I was thinking of a the case where an ack response might be synchronous,
but I suppose that should never happen?

+  client_->IncrementInFlightEventCount();
+  if (!process_->Send(new InputMsg_HandleEventPacket(routing_id_, packet))) {
+    in_flight_packet_id_ = 0;
+    client_->DecrementInFlightEventCount();
+  }
+}
+
+void BufferedInputRouter::DidFlush() {
+  TRACE_EVENT0("input", "BufferedInputRouter::DidFlush");
+  StopDelayedPacketMonitorTimeout();
+  client_->DidFlush();
+}
+
+void BufferedInputRouter::SetNeedsFlush() {
+  TRACE_EVENT0("input", "BufferedInputRouter::SetNeedsFlush");
+  client_->SetNeedsFlush();
+}
+
+void BufferedInputRouter::OnInputEventAck(const InputEvent& acked_event) {
+  if (!IsWebInputEventMessage(acked_event.message))
+    return;
+
+  const WebKit::WebInputEvent* web_event = NULL;
+  ui::LatencyInfo latency_info;
+  if (!CrackWebInputEventMessage(acked_event.message,
+                                 &web_event,
+                                 &latency_info,
+                                 NULL)) {
+    client_->OnUnexpectedEventAck(true);

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

I'm not sure what the client is supposed to do with this information. 
Misbehaving renderers is a problem that should be handled within the
InputRouter, that's part of its job no?

I understand this call is useful for testing, but that concern shouldn't bleed
into production code.  See what you can do with by overriding methods in a test
subclass of BufferedInputRouter instead.

[jdduke (slow), 2013/08/13 15:29:48]

Why should the router know how to handle a bad message? The client can determine
what the best response is; in this case it's telling the process that it
received a bad message and recording that action. If we kept the process as a
member we could just do that directly I suppose. 

+    return;
+  }
+
+  InputEventAckState ack_state = FromState(acked_event.state);
+  OnInputEventAck(acked_event.id, *web_event, latency_info, ack_state, true);
+}
+
+void BufferedInputRouter::OnInputEventAck(const InputEvent& acked_event,
+                                          EventInjector* injector) {
+  // Temporarily override the input queue with |injector|.
+  base::AutoReset<EventAckHandler::EventInjector*>

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

I don't think this AutoReset trick is a good idea.  You're using this like a
call stack object, therefore only stack storage should be used.  You could
achieve the same thing more explicitly with parameters passed to every function
that needs it.  See also my design comments about "class EventInjector" and
"class InputRouterClient" methods later.

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+      input_queue_override(&input_queue_override_, injector);
+  OnInputEventAck(acked_event);
+}
+
+bool BufferedInputRouter::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  bool message_is_ok = true;
+  IPC_BEGIN_MESSAGE_MAP_EX(BufferedInputRouter, message, message_is_ok)
+    IPC_MESSAGE_HANDLER(InputHostMsg_HandleEventPacket_ACK, OnEventPacketAck)
+    IPC_MESSAGE_HANDLER(ViewHostMsg_HasTouchEventHandlers,
+                        OnHasTouchEventHandlers)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  if (!message_is_ok)
+    client_->OnUnexpectedEventAck(true);
+
+  return handled;
+}
+
+const NativeWebKeyboardEvent*
+    BufferedInputRouter::GetLastKeyboardEvent() const {
+  return queued_key_map_.empty() ? NULL : &queued_key_map_.begin()->second;
+}
+
+bool BufferedInputRouter::ShouldForwardTouchEvent() const {
+  return has_touch_handler_ && queued_touch_count_ > 0;
+}
+
+bool BufferedInputRouter::ShouldForwardGestureEvent(
+    const GestureEventWithLatencyInfo& touch_event) const {
+  return true;
+}
+
+bool BufferedInputRouter::HasQueuedGestureEvents() const {
+  return queued_gesture_count_ > 0;
+}
+
+void BufferedInputRouter::OnInputEventAck(
+    int64 event_id,
+    const WebKit::WebInputEvent& web_event,
+    const ui::LatencyInfo& latency_info,
+    InputEventAckState acked_result,
+    bool ack_from_input_queue) {
+  if (WebInputEvent::isKeyboardEventType(web_event.type)) {
+    if (ack_from_input_queue) {
+      KeyMap::iterator key_it = queued_key_map_.find(event_id);
+      DCHECK(key_it != queued_key_map_.end());

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

DCHECK_NE(key_it, queued_key_map_.end());

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+      if (key_it != queued_key_map_.end()) {

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

Delete this if().

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+        NativeWebKeyboardEvent key_event = key_it->second;
+        queued_key_map_.erase(key_it);
+        client_->OnKeyboardEventAck(key_event, acked_result);
+      }
+    } else {
+      DCHECK(event_id == 0);

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

DCHECK_EQ(0, event_id);

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+      client_->OnKeyboardEventAck(
+          static_cast<const NativeWebKeyboardEvent&>(web_event), acked_result);
+    }
+    // WARNING: This BufferedInputRouter can be deallocated at this point

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

Should Ctrl-W's consequence be made async so that this isn't a problem?

[jdduke (slow), 2013/08/13 15:29:48]

That would be nice; I'm not sure what that would entail, but I can look into it.

+    // (i.e.  in the case of Ctrl+W, where the call to
+    // HandleKeyboardEvent destroys this BufferedInputRouter).
+  } else if (web_event.type == WebInputEvent::MouseWheel) {
+    client_->OnWheelEventAck(
+        static_cast<const WebMouseWheelEvent&>(web_event), acked_result);
+  } else if (WebInputEvent::isTouchEventType(web_event.type)) {
+    if (ack_from_input_queue) {
+      DCHECK(queued_touch_count_ > 0);

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

DCHECK_GT(queued_touch_count_, 0);

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+      --queued_touch_count_;
+    }
+    client_->OnTouchEventAck(
+        TouchEventWithLatencyInfo(static_cast<const WebTouchEvent&>(web_event),
+                                  latency_info), acked_result);
+  } else if (WebInputEvent::isGestureEventType(web_event.type)) {
+    if (ack_from_input_queue) {
+      DCHECK(queued_gesture_count_ > 0);

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

DCHECK_GT(queued_touch_count_, 0);

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+      --queued_gesture_count_;
+    }
+    client_->OnGestureEventAck(
+        static_cast<const WebGestureEvent&>(web_event), acked_result);
+  } else {
+    NOTREACHED() << "Unexpected WebInputEvent in OnInputEventAck";
+  }
+}
+
+void BufferedInputRouter::OnEventPacketAck(const EventPacket& packet) {
+  TRACE_EVENT1("input", "BufferedInputRouter::OnEventPacketAck",
+               "id", packet.id);
+  if (!in_flight_packet_id_ || packet.id != in_flight_packet_id_) {
+    client_->OnUnexpectedEventAck(false);
+    return;
+  }
+
+  in_flight_packet_id_ = 0;
+  client_->DecrementInFlightEventCount();
+
+  InputQueue::AckResult ack_result = input_queue_->OnEventPacketAck(packet);
+  if (ack_result == InputQueue::ACK_UNEXPECTED)
+    client_->OnUnexpectedEventAck(false);
+  else if (ack_result == InputQueue::ACK_INVALID)
+    client_->OnUnexpectedEventAck(true);
+}
+
+void BufferedInputRouter::OnHasTouchEventHandlers(bool has_handlers) {
+  if (has_touch_handler_ == has_handlers)
+    return;
+  has_touch_handler_ = has_handlers;
+  client_->OnHasTouchEventHandlers(has_handlers);
+}
+
+int64 BufferedInputRouter::QueueWebEvent(const WebKit::WebInputEvent& web_event,
+                                         const ui::LatencyInfo& latency_info,
+                                         bool is_keyboard_shortcut,
+                                         bool has_followup) {
+  TRACE_EVENT0("input", "BufferedInputRouter::QueueWebEvent");
+
+  DCHECK(!process_->IgnoreInputEvents());
+  if (FilterWebEvent(web_event, latency_info)) {
+    TRACE_EVENT_INSTANT0("input",
+                         "BufferedInputRouter::QueueWebEvent::Filtered",
+                         TRACE_EVENT_SCOPE_THREAD);
+    return 0;
+  }
+
+  InputEventType type = has_followup ? INPUT_EVENT_NEEDS_ACK_AND_HAS_FOLLOWUP
+                                     : INPUT_EVENT_NEEDS_ACK;
+  InputEvent event(NewInputID(),
+                   type,
+                   InputMsg_HandleInputEvent(routing_id_,
+                                             &web_event,
+                                             latency_info,
+                                             is_keyboard_shortcut));
+
+  // The presence of |input_queue_override_| implies that we are in the
+  // scope of OnInputEventAck with a valid injector.
+  if (input_queue_override_)
+    input_queue_override_->InjectEvent(event, this);
+  else
+    input_queue_->QueueEvent(event, this);
+
+  return event.id;
+}
+
+bool BufferedInputRouter::FilterWebEvent(const WebKit::WebInputEvent& web_event,
+                                         const ui::LatencyInfo& latency_info) {
+  // Perform optional, synchronous event handling, sending ACK messages for
+  // processed events, or proceeding as usual.
+  InputEventAckState filter_ack = client_->FilterInputEvent(web_event,
+                                                            latency_info);
+  switch (filter_ack) {
+    // Send the ACK and early exit.
+    case INPUT_EVENT_ACK_STATE_CONSUMED:
+    case INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS:
+      OnInputEventAck(0, web_event, latency_info, filter_ack, false);
+      // WARNING: |this| may be deleted at this point.
+      return true;
+
+    // Drop the event.
+    case INPUT_EVENT_ACK_STATE_UNKNOWN:
+      return true;
+
+    // Proceed as normal.
+    case INPUT_EVENT_ACK_STATE_NOT_CONSUMED:
+    default:
+      break;
+  };
+
+  return false;
+}
+
+void BufferedInputRouter::StartDelayedPacketMonitorTimeout(TimeDelta delay) {
+  Time target_delayed_time = Time::Now() + delay;

[aelias_OOO_until_Jul13, 2013/08/13 06:11:51]

Use TimeTicks::Now() instead.

[jdduke (slow), 2013/08/13 15:29:48]

Done.

+  if (time_when_signal_delayed_packet_.is_null() ||
+      time_when_signal_delayed_packet_ > target_delayed_time) {
+    // This will effectively clear an existing timeout.
+    time_when_signal_delayed_packet_ = target_delayed_time;
+  }
+
+  // If we already have a timer with the same or shorter duration, then we can
+  // wait for it to finish.
+  if (delayed_packet_timer_.IsRunning() &&
+      delayed_packet_timer_.GetCurrentDelay() <= delay) {
+    return;
+  }
+
+  time_when_signal_delayed_packet_ = target_delayed_time;
+  delayed_packet_timer_.Stop();
+  delayed_packet_timer_.Start(FROM_HERE, delay, this,
+      &BufferedInputRouter::CheckShouldSignalDelayedPacket);
+}
+
+void BufferedInputRouter::StopDelayedPacketMonitorTimeout() {
+  time_when_signal_delayed_packet_ = Time();
+  delayed_packet_timer_.Stop();
+}
+
+void BufferedInputRouter::CheckShouldSignalDelayedPacket() {
+  if (time_when_signal_delayed_packet_.is_null())
+    return;
+
+  Time now = Time::Now();
+  if (now < time_when_signal_delayed_packet_) {
+    StartDelayedPacketMonitorTimeout(time_when_signal_delayed_packet_ - now);
+    return;
+  }
+
+  TRACE_EVENT1("input", "BufferedInputRouter::EventPacketDelayed",
+               "id", in_flight_packet_id_);
+  input_queue_->OnEventPacketAckDelayed();
+}
+
+int64 BufferedInputRouter::NewInputID() {
+  return ++last_input_id_;
+}
+
+} // namespace content