Provided batched input delivery with a BufferedInputRouter

content/browser/renderer_host/input/input_queue.h

+// 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.
+
+#ifndef CONTENT_BROWSER_RENDERER_HOST_INPUT_INPUT_QUEUE_H_
+#define CONTENT_BROWSER_RENDERER_HOST_INPUT_INPUT_QUEUE_H_
+
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "content/common/content_export.h"
+
+namespace content {
+
+class InputQueueClient;
+
+class EventAckHandler;
+class EventPacket;
+class InputEvent;
+
+class CONTENT_EXPORT InputQueue {
+ public:
+
+  // The |client| must outlive this object.
+  InputQueue(InputQueueClient* client);
+  virtual ~InputQueue();
+
+  // If |ack_handler| is non-null, it must live at least until |event| has been
+  // ack'ed. If null, no ack response is made for |event|.
+  // If |event| is flagged for follow-up, its |ack_handler| will receive
+  // an EventInjector capable of inserting events into the current flush stream.
+  void QueueEvent(const InputEvent& event, EventAckHandler* ack_handler);

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

InputQueue is basically a split-off portion of BufferedInputRouter
implementation.   So there's not any value in having multiplexed ack handlers
only at this level, but then notify exactly one InputRouterClient about it.  The
point of the whole injection handler concept was so that multiple
platform-specific or test-specific clients could queue events into
BufferedInputRouter and decide what to do when they're acked.

As far as InputQueue -> BufferedInputRouter communication goes, it should just
be via InputQueueClient.  Move the EventAckHandler up one level so that
RenderWidgetHostImpl derives from it, not BufferedInputRouter.

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

Right, this is what I told you I wanted to do...

+
+  // Initiates the flush of the pending event packet to |client_|, if necessary.
+  // This should only be called in response to |client_->SetNeedsFlush()|
+  // Note: Events queued after this call, but before the flush completes, will
+  // not be sent until the following flush. If a flush is still in progress, the
+  // call will be ignored.
+  void FlushEventsInCurrentFrame();
+
+
+  // Called by the owner upon EventPacket responses from the sender target. This
+  // will dispatch event acks for events with a corresponding |ack_handler|.
+  enum AckResult {
+    ACK_OK,          // |acked_packet| was processed successfully.
+    ACK_UNEXPECTED,  // |acked_packet| was unexpected; no flush was in-progress.
+    ACK_INVALID,     // |acked_packet| contains invalid data.
+    ACK_SHUTDOWN     // |acked_packet| processing triggered queue shutdown.
+  };
+  AckResult OnEventPacketAck(const EventPacket& acked_packet);
+
+  // Called by the owner if the in-flight EventPacket has been delayed.

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

OK, I've read this comment but I still don't really understand what this is
trying to achieve, could you explain?

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

In short, "DidFlush" allows the client to proceed with anything held up by the
"Flush" call.  If the ack is delayed, we can send a "DidFlush" early to unlock
the BeginFrame.

I agree that the method is a bit awkward on this class; it was convenient to
have a central entity manage when the "DidFlush" was sent, but I suppose that
needn't come from the InputQueue.  We could send it from the BIR, and when the
ack does come we can either send a redundant DidFlush or drop the DidFlush.  In
any case, I'm not sure that this timeout logic will be effective in practice; I
think it adds a bit of complexity that we don't need and it's prematurely
optimizing a case we haven't measured. 

+  // |DidFlush()| will be signalled to the client, but the queue will continue
+  // waiting for the packet ack.  If there are pending events, a flush
+  // will be requested; if the requested |Flush()| is made before the
+  // outstanding packet has been ack'ed, the packet will be dropped and all
+  // outstanding events will be marked |UNHANDLED| and ack'ed appropriately.
+  void OnEventPacketAckDelayed();
+
+  // Total number of evenst in the queue, both being flushed and pending flush.

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

typo: "evenst"

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

Done.

+  size_t QueuedEventCount() const;
+
+ protected:
+  friend class InputQueueTest;

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

Shouldn't use friending here either.

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

Where is this policy established? I see "friend FooTest" just about everywhere;
not that I'm for friending, but I'd like to get more than just mixed signals
here.

+
+  // If we have a current, non-empty EventPacket, deliver it to |client_|.
+  // Otherwise, finish the flush by signalling flush completion, and requesting
+  // an additional flush, if necessary.
+  void TryFinishFlush();
+
+  // Requests a flush via the |client_|, but only if a flush is required and has
+  // not yet been requested.
+  void RequestFlushIfNecessary();
+
+  // Signal flush completion to the |client_| if not yet sent for this flush.
+  void SignalFlushedIfNecessary();
+
+  // True when |current_frame_| is non-empty.
+  bool IsFlushing() const;
+
+private:
+  InputQueueClient* client_;
+
+  // Used to assign unique ID's to each EventPacket that is generated.
+  int64 last_packet_id_;

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

next_packet_id_

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

Done.

+
+  // Avoid spamming the client with redundant flush requests.
+  bool flush_requested_;
+
+  // Allows a DidFlush() signal before the packet has been ack'ed.
+  bool flush_signalled_;
+
+  class EventFrame;
+
+  // Events currently being flushed.
+  scoped_ptr<EventFrame> current_frame_;
+
+  // Events pending flush.
+  scoped_ptr<EventFrame> pending_frame_;
+
+  DISALLOW_COPY_AND_ASSIGN(InputQueue);
+};
+
+} // namespace content
+
+#endif // CONTENT_BROWSER_RENDERER_HOST_INPUT_INPUT_QUEUE_H_