input: Make the OverscrollController less intrusive, and some code cleanup.

input: Make the OverscrollController less intrusive, and some code cleanup.

* Make OverscrollController less intrusive to input-event dispatching. With this
  change, the OverscrollController consumes an event on its way to dispatch only
  if the event contributes to the overscroll.
* When an overscroll is in progress, and a gesture event comes in, this event is
  not dispatched to the GestureEventFilter from RenderWidgetHostImpl. So, if the
  event should be dispatched to the renderer, make sure it always goes through
  the host from the OverscrollController.
* Remove HasQueuedGestureEvents() from InputRouter interface, and forward all
  gesture events through the RenderWidgetHost from the overscroll-controller.

BUG=none
R=aelias@chromium.org, jdduke@chromium.org

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

[aelias_OOO_until_Jul13, 2013-09-03 02:24:10]

https://codereview.chromium.org/23801003/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (left):

https://codereview.chromium.org/23801003/diff/31001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:286: return
gesture_event_filter_->HasQueuedGestureEvents();
Is this intended to be a no-op change?  You're no longer checking
GestureEventFilter::coalesced_event_queue_ so it seems this introduces a
behavior change.  Why was this needed before but safe now?

https://codereview.chromium.org/23801003/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/23801003/diff/31001/content/browser/renderer_...
content/browser/renderer_host/overscroll_controller.cc:89: // Let the event be
dispatched to the renderer.
I don't think the OverscrollController should be making any decisions about
event dispatch.  Can't overscroll be implemented entirely using passive
listening?  I would really like to see this code moved out of here.

[jdduke (slow), 2013-09-03 15:09:33]

I'm all for dropping any of the "HasQueuedGesture" or "ShouldForwardGesture"
router methods.  These were bolted on primarily to facilitate the existence of
the OverscrollController outside of the router, but they're fairly leaky and
coupled to the design of the GestureEventFilter.  A part of me wonders if we
might as well move it(the OverscrollController) inside the ImmediateInputRouter.

As aelias@ mentioned, ideally the OverscrollController would be a purely passive
participant in the input stream, but perhaps that's not possible?

[sadrul, 2013-09-03 15:14:39]

On 2013/09/03 15:09:33, jdduke wrote:
> I'm all for dropping any of the "HasQueuedGesture" or "ShouldForwardGesture"
> router methods.  These were bolted on primarily to facilitate the existence of
> the OverscrollController outside of the router, but they're fairly leaky and
> coupled to the design of the GestureEventFilter.  A part of me wonders if we
> might as well move it(the OverscrollController) inside the
ImmediateInputRouter.
> 
> As aelias@ mentioned, ideally the OverscrollController would be a purely
passive
> participant in the input stream, but perhaps that's not possible?

I plan on making the overscroll controller do fewer things to make it less
brittle, and easier to understand. Let me look into that first, and put review
for this on hold for now.

[sadrul, 2013-09-11 22:03:16]

On 2013/09/03 15:14:39, sadrul wrote:
> On 2013/09/03 15:09:33, jdduke wrote:
> > I'm all for dropping any of the "HasQueuedGesture" or "ShouldForwardGesture"
> > router methods.  These were bolted on primarily to facilitate the existence
of
> > the OverscrollController outside of the router, but they're fairly leaky and
> > coupled to the design of the GestureEventFilter.  A part of me wonders if we
> > might as well move it(the OverscrollController) inside the
> ImmediateInputRouter.
> > 
> > As aelias@ mentioned, ideally the OverscrollController would be a purely
> passive
> > participant in the input stream, but perhaps that's not possible?
> 
> I plan on making the overscroll controller do fewer things to make it less
> brittle, and easier to understand. Let me look into that first, and put review
> for this on hold for now.

I made some changes to have the OverscrollController consume fewer events
(because it's currently not possible to have it not consume any event). With
this change, the OC will consume only scroll-update gesture and mouse-wheel
events, when overscroll is already in progress (since they are processed by the
browser, and dispatching these to the renderer can lead to undesired UX). I have
also added comments to the code and updated the CL description to explain the
change.

Please review. Thanks!

[jdduke (slow), 2013-09-11 23:09:30]

This looks pretty reasonable to me, given the constraints; thanks for the
cleanup. You'll likely need to rebase once crrev.com/20356003 lands; I think the
only change necessary will be the removal of the "HasQueuedGestureEvents()"
implementation from the BufferedInputRouter.

[sadrul, 2013-09-12 16:16:54]

On 2013/09/11 23:09:30, jdduke wrote:
> This looks pretty reasonable to me, given the constraints; thanks for the
> cleanup. You'll likely need to rebase once crrev.com/20356003 lands; I think
the
> only change necessary will be the removal of the "HasQueuedGestureEvents()"
> implementation from the BufferedInputRouter.

Done. PTAL

[jdduke (slow), 2013-09-12 16:32:35]

LGTM with nits, and potentially any additional tests you think are necessary or
appropriate?

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

https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:127: int
queued_gesture_count_;
Could you nuke this variable and the code where it's
initialized/incremented/decremented?

https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:2300: 
Are there any additional states or event sequences we'd want to test that differ
from before?

[jdduke (slow), 2013-09-12 16:33:18]

On 2013/09/12 16:32:35, jdduke wrote:
> LGTM with nits, and potentially any additional tests you think are necessary
or
> appropriate?
> 
>
https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
> File content/browser/renderer_host/input/buffered_input_router.h (right):
> 
>
https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
> content/browser/renderer_host/input/buffered_input_router.h:127: int
> queued_gesture_count_;
> Could you nuke this variable and the code where it's
> initialized/incremented/decremented?
> 
>
https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_unittest.cc:2300: 
> Are there any additional states or event sequences we'd want to test that
differ
> from before?

And perhaps make the patch title say something about OverscrollController?

[sadrul, 2013-09-12 17:31:19]

Updated the patch title too

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

https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
content/browser/renderer_host/input/buffered_input_router.h:127: int
queued_gesture_count_;
On 2013/09/12 16:32:35, jdduke wrote:
> Could you nuke this variable and the code where it's
> initialized/incremented/decremented?

Done.

https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/23801003/diff/68001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:2300: 
On 2013/09/12 16:32:35, jdduke wrote:
> Are there any additional states or event sequences we'd want to test that
differ
> from before?

I added some expectations in a few more tests.

[sadrul, 2013-09-13 15:12:42]

aelias@: ping

[aelias_OOO_until_Jul13, 2013-09-13 18:20:34]

lgtm

[sadrul, 2013-09-13 19:05:07]

Committed patchset #10 manually as r223096 (presubmit successful).