Provided batched input delivery with a BufferedInputRouter

Provided batched input delivery with a BufferedInputRouter

The BufferedInputRouter batches input events into EventPackets. The delivery of 
these packets is dependent on periodic flush signals, as requested by the
InputRouterClient.  A given flush involves the forwarding of EventPackets to the
renderer until all InputEvents from the original packet have been dispatched.

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

[jdduke (slow), 2013-07-25 17:37:42]

OK, I've tried to limit the scope of the previous browser-side patch.  This
includes only changes to renderer_host/input, and a couple changes to RWHI for
the InputRouterClient method additions of "RequestFlush()" and "DidFlush()".  

Please ignore what is in content/common; that is pulled directly from
https://codereview.chromium.org/19624005, and should be reviewed there.  Thanks.

[jdduke (slow), 2013-07-31 15:42:56]

So, a couple thoughts as I've fleshed this out some more.

1) The timeout logic in the BIR needs some work.  I don't believe we should stop
waiting for input ACK's and flush with "UNHANDLED"; that should really only
happen if the renderer is hung.  If an ACK is delayed, we should proceed with
the normal BeginFrame messages, and just sit tight waiting for the ack.

2) There's a lot of nice optimizations we can make here: send the packet
immediately if we get a "NEEDS_FOLLOWUP" event, use the packet as the BeginFrame
signal, call "DidFlush" immediately after sending packets that don't require
follow-up, etc...  I'm hesitant to add all of that special logic to this patch,
for the sake of initial simplicity.  In fact, I think I may rip out the current
timeout logic.

3) I'm starting to feel that having the state as part of the InputEvent, and
ack'ing with the event itself, is unnecessary.  I'd rather we have the event ACK
include the state, the unique event id, and any additional ACK payload we want
(overscroll delta, etc...), but not the event itself.  We have to store the
event in the browser, regardless (for security), so it's really just extra
baggage and overhead.  Thoughts?

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

First pass, I didn't get into the guts of the queuing/timeout logic yet. At a
high level, my most important design comment is that RenderWidgetHostImpl should
derive from InputAckHandler, not BufferedInputRouter.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.cc (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:65:
input_queue_(new InputQueue(this)),
Passing partially constructed "this" pointer here seems slightly sketchy, I'd
prefer InputQueue was created within the {} section of the constructor.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:79:
DCHECK(in_flight_packet_id_ == 0);
DCHECK_EQ(0, in_flight_packet_id_);

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:81:
in_flight_packet_id_ = 0;
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.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:90:
scoped_ptr<IPC::Message> scoped_message(message);
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.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:91: if
(IPC_MESSAGE_ID_CLASS(message->type()) != InputMsgStart) {
These look like they should be DCHECK_EQ/DCHECK_NE.  Get rid of the "return
false"s.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:174: //
TODO(jdduke): How should we handle this case? Clear the existing queue?
I think you should mandate that the process has a longer lifetime than your
class.  Don't try to handle it in the implementation.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:178:
DCHECK(packet.id && !in_flight_packet_id_);
Split this into two separate DCHECKs (the output will be less ambiguous)

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:179:
in_flight_packet_id_ = packet.id;
Can you put these two lines after the Send() and just "return;" if it fails?

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:208:
client_->OnUnexpectedEventAck(true);
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.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:219:
base::AutoReset<EventAckHandler::EventInjector*>
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.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:267: DCHECK(key_it
!= queued_key_map_.end());
DCHECK_NE(key_it, queued_key_map_.end());

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:268: if (key_it !=
queued_key_map_.end()) {
Delete this if().

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:274:
DCHECK(event_id == 0);
DCHECK_EQ(0, event_id);

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:278: // WARNING:
This BufferedInputRouter can be deallocated at this point
Should Ctrl-W's consequence be made async so that this isn't a problem?

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:286:
DCHECK(queued_touch_count_ > 0);
DCHECK_GT(queued_touch_count_, 0);

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:294:
DCHECK(queued_gesture_count_ > 0);
DCHECK_GT(queued_touch_count_, 0);

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:390: Time
target_delayed_time = Time::Now() + delay;
Use TimeTicks::Now() instead.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:67: friend class
BufferedInputRouterTest;
friending can normally be avoided by marking things "protected" and having a
test class derive from it, please give that a try.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:70: virtual void
Deliver(const EventPacket& packet) OVERRIDE;
These are public in the base class so they should be public here too.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:106: int64
NewInputID();
Call this NextInputID()

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:109:
RenderProcessHost* process_;
All you ever do with this is send IPCs, so store it as an IPC::Sender instead
(likewise take IPC::Sender in the constructor)

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:130: int64
last_input_id_;
Common idiom is to call this next_input_id_

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:136: int
delayed_packet_timeout_ms_;
Store this as base::TimeDelta as well.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:139: base::Time
time_when_signal_delayed_packet_;
Use base::TimeTicks instead.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/event_ack_handler.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/event_ack_handler.h:16: class
EventAckHandler {
Please delete this class as it stands, I don't think it makes sense in the
design.  More details below.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/event_ack_handler.h:20: virtual void
OnInputEventAck(const InputEvent& acked_event) = 0;
This method can be moved to InputQueueClient.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/event_ack_handler.h:25: class EventInjector
{
This class should be deleted (and it's not what I had in mind when I talked
about "EventInjector" in the design discussions).  If you need to hold your
place in the queue there's no need for a class like this for it.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:33: void QueueEvent(const
InputEvent& event, EventAckHandler* ack_handler);
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.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:53: // Called by the owner if
the in-flight EventPacket has been delayed.
OK, I've read this comment but I still don't really understand what this is
trying to achieve, could you explain?

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:61: // Total number of evenst
in the queue, both being flushed and pending flush.
typo: "evenst"

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:65: friend class
InputQueueTest;
Shouldn't use friending here either.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:86: int64 last_packet_id_;
next_packet_id_

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/input_router_client.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_router_client.h:65: virtual void
OnKeyboardEventAck(const NativeWebKeyboardEvent& event,
I think these should be moved into "InputAckHandler" base class.  Make
RenderWidgetHostImpl derive from InputAckHandler (not BufferedInputRouter).  And
make the methods have a return value like:

  scoped_ptr_vector<InputEvent> OnGestureEventAck(const WebGestureEvent&,
InputEventAckState);

Then the BufferedInputRouter/InputQueue would inject those returned input events
(if any) at the right part of the queue.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/mock_input_router_client.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/mock_input_router_client.h:24: virtual void
IncrementInFlightEventCount() OVERRIDE;
These two Increment/Decrement methods seem to only exist for testing purposes. 
It's an implementation detail of InputRouter that shouldn't be exposed to the
client in production.  Instead for your testing, you can send a special
IPC::Sender subclass that tracks when events are sent.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/mock_input_router_client.h:53: virtual void
OnUnexpectedEventAck(bool bad_message) OVERRIDE;
Looks like this is also only for testing, please delete from InputRouterClient.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/mock_input_router_client.h:54: virtual void
DidFlush() OVERRIDE;
Looks like this is also only for testing, please delete from InputRouterClient.

[jdduke (slow), 2013-08-13 15:29:46]

I think we should drop the timeout logic for now, and add it back in if
necessary.  It's complexity that we don't know we'll need quite yet.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.cc (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:65:
input_queue_(new InputQueue(this)),
On 2013/08/13 06:11:51, aelias wrote:
> Passing partially constructed "this" pointer here seems slightly sketchy, I'd
> prefer InputQueue was created within the {} section of the constructor.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:79:
DCHECK(in_flight_packet_id_ == 0);
On 2013/08/13 06:11:51, aelias wrote:
> DCHECK_EQ(0, in_flight_packet_id_);

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:81:
in_flight_packet_id_ = 0;
On 2013/08/13 06:11:51, aelias wrote:
> 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.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:90:
scoped_ptr<IPC::Message> scoped_message(message);
On 2013/08/13 06:11:51, aelias wrote:
> 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.

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

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:91: if
(IPC_MESSAGE_ID_CLASS(message->type()) != InputMsgStart) {
On 2013/08/13 06:11:51, aelias wrote:
> These look like they should be DCHECK_EQ/DCHECK_NE.  Get rid of the "return
> false"s.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:174: //
TODO(jdduke): How should we handle this case? Clear the existing queue?
On 2013/08/13 06:11:51, aelias wrote:
> I think you should mandate that the process has a longer lifetime than your
> class.  Don't try to handle it in the implementation.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:178:
DCHECK(packet.id && !in_flight_packet_id_);
On 2013/08/13 06:11:51, aelias wrote:
> Split this into two separate DCHECKs (the output will be less ambiguous)

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:179:
in_flight_packet_id_ = packet.id;
On 2013/08/13 06:11:51, aelias wrote:
> Can you put these two lines after the Send() and just "return;" if it fails?

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

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:208:
client_->OnUnexpectedEventAck(true);
On 2013/08/13 06:11:51, aelias wrote:
> 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.

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.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:219:
base::AutoReset<EventAckHandler::EventInjector*>
On 2013/08/13 06:11:51, aelias wrote:
> 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.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:267: DCHECK(key_it
!= queued_key_map_.end());
On 2013/08/13 06:11:51, aelias wrote:
> DCHECK_NE(key_it, queued_key_map_.end());

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:268: if (key_it !=
queued_key_map_.end()) {
On 2013/08/13 06:11:51, aelias wrote:
> Delete this if().

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:274:
DCHECK(event_id == 0);
On 2013/08/13 06:11:51, aelias wrote:
> DCHECK_EQ(0, event_id);

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:278: // WARNING:
This BufferedInputRouter can be deallocated at this point
On 2013/08/13 06:11:51, aelias wrote:
> Should Ctrl-W's consequence be made async so that this isn't a problem?

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

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:286:
DCHECK(queued_touch_count_ > 0);
On 2013/08/13 06:11:51, aelias wrote:
> DCHECK_GT(queued_touch_count_, 0);

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:294:
DCHECK(queued_gesture_count_ > 0);
On 2013/08/13 06:11:51, aelias wrote:
> DCHECK_GT(queued_touch_count_, 0);

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:390: Time
target_delayed_time = Time::Now() + delay;
On 2013/08/13 06:11:51, aelias wrote:
> Use TimeTicks::Now() instead.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:70: virtual void
Deliver(const EventPacket& packet) OVERRIDE;
On 2013/08/13 06:11:51, aelias wrote:
> These are public in the base class so they should be public here too.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:106: int64
NewInputID();
On 2013/08/13 06:11:51, aelias wrote:
> Call this NextInputID()

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:109:
RenderProcessHost* process_;
On 2013/08/13 06:11:51, aelias wrote:
> All you ever do with this is send IPCs, so store it as an IPC::Sender instead
> (likewise take IPC::Sender in the constructor)

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:130: int64
last_input_id_;
On 2013/08/13 06:11:51, aelias wrote:
> Common idiom is to call this next_input_id_

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:136: int
delayed_packet_timeout_ms_;
On 2013/08/13 06:11:51, aelias wrote:
> Store this as base::TimeDelta as well.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:139: base::Time
time_when_signal_delayed_packet_;
On 2013/08/13 06:11:51, aelias wrote:
> Use base::TimeTicks instead.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/event_ack_handler.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/event_ack_handler.h:16: class
EventAckHandler {
On 2013/08/13 06:11:51, aelias wrote:
> Please delete this class as it stands, I don't think it makes sense in the
> design.  More details below.

I agree, that's why I told you I wanted to delete it :)

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/event_ack_handler.h:20: virtual void
OnInputEventAck(const InputEvent& acked_event) = 0;
On 2013/08/13 06:11:51, aelias wrote:
> This method can be moved to InputQueueClient.

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/event_ack_handler.h:25: class EventInjector
{
On 2013/08/13 06:11:51, aelias wrote:
> This class should be deleted (and it's not what I had in mind when I talked
> about "EventInjector" in the design discussions).  If you need to hold your
> place in the queue there's no need for a class like this for it.

Perhaps not in the end, but without modifying a number of external classes it
becomes a bit tricker (though by no means impossible).  As I said, I was always
in favor of deleting this...

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:33: void QueueEvent(const
InputEvent& event, EventAckHandler* ack_handler);
On 2013/08/13 06:11:51, aelias wrote:
> 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.

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

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:53: // Called by the owner if
the in-flight EventPacket has been delayed.
On 2013/08/13 06:11:51, aelias wrote:
> OK, I've read this comment but I still don't really understand what this is
> trying to achieve, could you explain?

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.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:61: // Total number of evenst
in the queue, both being flushed and pending flush.
On 2013/08/13 06:11:51, aelias wrote:
> typo: "evenst"

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:65: friend class
InputQueueTest;
On 2013/08/13 06:11:51, aelias wrote:
> Shouldn't use friending here either.

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.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:86: int64 last_packet_id_;
On 2013/08/13 06:11:51, aelias wrote:
> next_packet_id_

Done.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/input_router_client.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/input_router_client.h:65: virtual void
OnKeyboardEventAck(const NativeWebKeyboardEvent& event,
On 2013/08/13 06:11:51, aelias wrote:
> I think these should be moved into "InputAckHandler" base class.  Make
> RenderWidgetHostImpl derive from InputAckHandler (not BufferedInputRouter). 
And
> make the methods have a return value like:
> 
>   scoped_ptr_vector<InputEvent> OnGestureEventAck(const WebGestureEvent&,
> InputEventAckState);
> 
> Then the BufferedInputRouter/InputQueue would inject those returned input
events
> (if any) at the right part of the queue.

I would love for this to be the case.  As it stands that may take a good amount
of wiring (getting things out of the GR), but having an explicit followup adds a
lot of clarity.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/input/mock_input_router_client.h (right):

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/mock_input_router_client.h:24: virtual void
IncrementInFlightEventCount() OVERRIDE;
On 2013/08/13 06:11:51, aelias wrote:
> These two Increment/Decrement methods seem to only exist for testing purposes.

> It's an implementation detail of InputRouter that shouldn't be exposed to the
> client in production.  Instead for your testing, you can send a special
> IPC::Sender subclass that tracks when events are sent.

? This is how the client can determine if the renderer is hung.  Maybe there's a
better way, but it does serve a purpose beyond simply testing.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/mock_input_router_client.h:53: virtual void
OnUnexpectedEventAck(bool bad_message) OVERRIDE;
On 2013/08/13 06:11:51, aelias wrote:
> Looks like this is also only for testing, please delete from
InputRouterClient.

I tried to restrict this patch as much as possible to renderer_host/input, but I
see that has left a few gaps in how this all fits together.  crrev.com/19220002
shows how these are used to wire the BIR to BeginFrame signals on Android.

https://codereview.chromium.org/20356003/diff/28001/content/browser/renderer_...
content/browser/renderer_host/input/mock_input_router_client.h:54: virtual void
DidFlush() OVERRIDE;
On 2013/08/13 06:11:51, aelias wrote:
> Looks like this is also only for testing, please delete from
InputRouterClient.

See comment above.

[aelias_OOO_until_Jul13, 2013-08-15 00:52:15]

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.cc (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:17: using
base::Time;
Shouldn't need this.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:242: void
BufferedInputRouter::OnInputEventAck(
Please rename to OnWebInputEventAck.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:281: } else {
nit: remove this else {}

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:335: //
TODO(jdduke): Remove when InputAckHandler returns followup events directly.
Delete this comment since it's not clear we're going to do this ever.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.h (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:43: virtual void
SendMouseEvent(
There should be a detailed comment here explaining the injection behavior.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.cc (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:36: return
RestrictingEventType(packet) >= INPUT_EVENT_NEEDS_ACK;
Please avoid <=/>= on enums.  Please delete RestrictingEventType() and just use
std::find() for each enum type you're interested in.  There are only 4 anyway.

Or delete them since they seem unused now?

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:45: // Utilty class for
storing input events and their ack observers, and
typo: "Utilty"

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:100: // TODO(jdduke): Look
into making ack-triggered shutdown async.
Please file a bug for this.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:111:
observers[event_idx]->OnInputEventAck(event);
Please DCHECK the size of "observers".

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:177: NOTREACHED() <<
"InputQueue::Flush() - A flush is already in-progress.";
Another place that should just have a DCHECK, not a return.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:185: TryFinishFlush();
Do you ever want the request-flush behavior of TryFlush here?  Isn't this
supposed to be called in reaction to a request?

I don't think TryFinishFlush has clear semantics and I'd like it to be just
FinishFlush and just have the "deliver" line.  Move the signal and request
operations to OnEventPacketAck.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:221: if
(pending_frame_->IsEmpty())
Is it correct to check pending_frame_ here?  TryFinishFlush() is called after
the current_frame_.swap() above.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:232: void
InputQueue::SignalFlushedIfNecessary() {
I don't think this method (nor the field flush_signalled_) is needed.  Just do
if (!IsFlushing()) client_->DidFlush() in OnEventPacketAck.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.h (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:8: #include <vector>
Delete this include.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:11: #include "base/callback.h"
Delete this include.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:28: // If |ack_handler| is
non-null, it must live at least until |event| has been
This comment is out of date.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:68: void
SignalFlushedIfNecessary();
Rename to SignalFlushCompleteIfNecessary

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:77: int64 last_packet_id_;
Nit: next_packet_id_

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:83: bool flush_signalled_;
Rename to flush_complete_signalled_.

https://codereview.chromium.org/20356003/diff/39001/content/common/input/inpu...
File content/common/input/input_event.h (right):

https://codereview.chromium.org/20356003/diff/39001/content/common/input/inpu...
content/common/input/input_event.h:28: IPC::Message message;
I'm not a huge fan of putting an opaque blob here.  What is this really and why
is it hard to use the proper base class?

[jdduke (slow), 2013-08-15 23:22:26]

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.cc (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:17: using
base::Time;
On 2013/08/15 00:52:15, aelias wrote:
> Shouldn't need this.

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:242: void
BufferedInputRouter::OnInputEventAck(
On 2013/08/15 00:52:15, aelias wrote:
> Please rename to OnWebInputEventAck.

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:281: } else {
On 2013/08/15 00:52:15, aelias wrote:
> nit: remove this else {}

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.cc:335: //
TODO(jdduke): Remove when InputAckHandler returns followup events directly.
On 2013/08/15 00:52:15, aelias wrote:
> Delete this comment since it's not clear we're going to do this ever.

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.h (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:43: virtual void
SendMouseEvent(
On 2013/08/15 00:52:15, aelias wrote:
> There should be a detailed comment here explaining the injection behavior.

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.cc (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:36: return
RestrictingEventType(packet) >= INPUT_EVENT_NEEDS_ACK;
On 2013/08/15 00:52:15, aelias wrote:
> Please avoid <=/>= on enums.  Please delete RestrictingEventType() and just
use
> std::find() for each enum type you're interested in.  There are only 4 anyway.
> 
> Or delete them since they seem unused now?

Yeah this was a speculative addition.  I'll remove now, and we can add something
similar in the future should the need arise.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:45: // Utilty class for
storing input events and their ack observers, and
On 2013/08/15 00:52:15, aelias wrote:
> typo: "Utilty"

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:111:
observers[event_idx]->OnInputEventAck(event);
On 2013/08/15 00:52:15, aelias wrote:
> Please DCHECK the size of "observers".

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:177: NOTREACHED() <<
"InputQueue::Flush() - A flush is already in-progress.";
On 2013/08/15 00:52:15, aelias wrote:
> Another place that should just have a DCHECK, not a return.

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:185: TryFinishFlush();
On 2013/08/15 00:52:15, aelias wrote:
> Do you ever want the request-flush behavior of TryFlush here?  Isn't this
> supposed to be called in reaction to a request?
> 
> I don't think TryFinishFlush has clear semantics and I'd like it to be just
> FinishFlush and just have the "deliver" line.  Move the signal and request
> operations to OnEventPacketAck.

Yeah, I added this to allow a bit of flexibility if we ever sent early
"DidFlush()" signals, but it has since been decided that we will never do that. 
I'll remove.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:221: if
(pending_frame_->IsEmpty())
On 2013/08/15 00:52:15, aelias wrote:
> Is it correct to check pending_frame_ here?  TryFinishFlush() is called after
> the current_frame_.swap() above.

Well, I think I should clear up the expectations: A flush request should ONLY
occur for a non-empty pending_frame_.  So the TryFinishFlush() should never hit
this path.

TryFinishFlush probably makes it more confusing than it ought to be.  I'll
remove entirely.  Thanks.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:232: void
InputQueue::SignalFlushedIfNecessary() {
On 2013/08/15 00:52:15, aelias wrote:
> I don't think this method (nor the field flush_signalled_) is needed.  Just do
> if (!IsFlushing()) client_->DidFlush() in OnEventPacketAck.

Right, deleting.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.h (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:8: #include <vector>
On 2013/08/15 00:52:15, aelias wrote:
> Delete this include.

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:11: #include "base/callback.h"
On 2013/08/15 00:52:15, aelias wrote:
> Delete this include.

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:28: // If |ack_handler| is
non-null, it must live at least until |event| has been
On 2013/08/15 00:52:15, aelias wrote:
> This comment is out of date.

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:68: void
SignalFlushedIfNecessary();
On 2013/08/15 00:52:15, aelias wrote:
> Rename to SignalFlushCompleteIfNecessary

See comment below about flush_complete_signalled_: we can remove this
completely.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:77: int64 last_packet_id_;
On 2013/08/15 00:52:15, aelias wrote:
> Nit: next_packet_id_

Done.

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.h:83: bool flush_signalled_;
On 2013/08/15 00:52:15, aelias wrote:
> Rename to flush_complete_signalled_.

Hmm, I think this is a relic from when I had the awkward timeout logic.  This
should no longer be necessary, as we will only ever send a DidFlush when the
flush actually completes.

https://codereview.chromium.org/20356003/diff/39001/content/common/input/inpu...
File content/common/input/input_event.h (right):

https://codereview.chromium.org/20356003/diff/39001/content/common/input/inpu...
content/common/input/input_event.h:28: IPC::Message message;
On 2013/08/15 00:52:15, aelias wrote:
> I'm not a huge fan of putting an opaque blob here.  What is this really and
why
> is it hard to use the proper base class?

Nope.  I've been working on that yesterday and today; see crrev.com/19624005.
I'll have another update to that patch early tomorrow.

[jdduke (slow), 2013-08-15 23:53:30]

crbug.com/274029

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.cc (right):

https://codereview.chromium.org/20356003/diff/39001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:100: // TODO(jdduke): Look
into making ack-triggered shutdown async.
On 2013/08/15 00:52:15, aelias wrote:
> Please file a bug for this.

Done.

[nduca, 2013-08-27 19:38:12]

https://codereview.chromium.org/20356003/diff/62001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.cc (right):

https://codereview.chromium.org/20356003/diff/62001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:28: void
QueueEvent(scoped_ptr<InputEvent> event, InputAckObserver* observer) {
AckObserver ---> DispositionObserver

https://codereview.chromium.org/20356003/diff/62001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:162: void
InputQueue::FlushPendingEvents() {
BeginFlushPending

https://codereview.chromium.org/20356003/diff/62001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:172: FinishFlush();
SendAnotherInputPacketIfPossible

https://codereview.chromium.org/20356003/diff/62001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:187: FinishFlush();
SendAnotherInputPacketIfPossible

https://codereview.chromium.org/20356003/diff/62001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:189: client_->DidFlush();
DidFinishFlush

[jdduke (slow), 2013-08-28 19:47:15]

I've factored out the content/common code for clarity of review.  This patch may
change slightly as the common types are refined.

[jdduke (slow), 2013-09-09 17:14:10]

OK, I think this is ready for further review.

@aelias, our last review was from Patch Set 12. Since then, the primary change
has been rebasing to the content/common changes (InputEvent -> (InputEvent +
Payload)).

I've also introduced the "BrowserInputEvent", and "BrowserInputEventClient", the
latter taking the place both of the "InputQueueClient" and "InputAckObserver". 
I think this simplifies the ack logic, and provides more flexibility to the
queuer of InputEvents.  In the future, we might make the client a closure, but
the current client interface works well and makes such a potential change
trivial.

[aelias_OOO_until_Jul13, 2013-09-09 22:31:14]

lgtm with minor comments below.

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
File content/browser/renderer_host/input/browser_input_event.h (right):

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
content/browser/renderer_host/input/browser_input_event.h:38: // |client| may be
NULL.
I don't think we should allow NULL clients.  Please DCHECK it's non-NULL.

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.h (right):

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:46: //
WebInputEvent's sent to the router synchronously after an ack dispatch to
Please update this comment to be clearer about exactly what you mean by "after".

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:92:
switches::kNoTouchToRendererWhileScrolling) == "1";
I don't understand why == "1", isn't this a boolean?  Please check it like every
other switch unless there's more than two states.

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.cc (right):

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:56: if (!weak_ref_this.get())
Please add a comment // TODO(jdduke): http://crbug.com/bug-you-filed

[jdduke (slow), 2013-09-10 19:41:16]

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
File content/browser/renderer_host/input/browser_input_event.h (right):

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
content/browser/renderer_host/input/browser_input_event.h:38: // |client| may be
NULL.
On 2013/09/09 22:31:14, aelias wrote:
> I don't think we should allow NULL clients.  Please DCHECK it's non-NULL.

Done.

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
File content/browser/renderer_host/input/buffered_input_router.h (right):

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:46: //
WebInputEvent's sent to the router synchronously after an ack dispatch to
On 2013/09/09 22:31:14, aelias wrote:
> Please update this comment to be clearer about exactly what you mean by
"after".

Done.

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:92:
switches::kNoTouchToRendererWhileScrolling) == "1";
On 2013/09/09 22:31:14, aelias wrote:
> I don't understand why == "1", isn't this a boolean?  Please check it like
every
> other switch unless there's more than two states.

I'd be happy to change it, but that wasn't my addition.

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
File content/browser/renderer_host/input/input_queue.cc (right):

https://codereview.chromium.org/20356003/diff/98001/content/browser/renderer_...
content/browser/renderer_host/input/input_queue.cc:56: if (!weak_ref_this.get())
On 2013/09/09 22:31:14, aelias wrote:
> Please add a comment // TODO(jdduke): http://crbug.com/bug-you-filed

Done.

[jdduke (slow), 2013-09-11 15:36:10]

jam@: PTAL at content/content_{browser,tests}.gypi for OWNERS.   Thanks.

aelias@: nduca@ suggested I add myself as an owner for renderer_host/input/.  If
you have any objections I'll remove.

[jam, 2013-09-11 18:21:06]

On 2013/09/11 15:36:10, jdduke wrote:
> jam@: PTAL at content/content_{browser,tests}.gypi for OWNERS.   Thanks.
> 
> aelias@: nduca@ suggested I add myself as an owner for renderer_host/input/. 
If
> you have any objections I'll remove.

lgtm

btw please feel free to TBR simple changes to the gypis to me

[commit-bot: I haz the power, 2013-09-11 19:17:57]

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

[commit-bot: I haz the power, 2013-09-11 20:27:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-11 21:37:03]

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

[commit-bot: I haz the power, 2013-09-12 00:32:50]

Change committed as 222674