Move OverscrollController to RenderWidgetHostViewAura

Move OverscrollController to RenderWidgetHostViewAura

OverscrollController is currently only used by Aura, and there are no plans to
adopt it on other platforms.  As such, move it to RenderWidgetHostViewAura,
simplifying both RenderWidgetHostImpl and InputRouterImpl.  This move should not
affect overscroll behavior on Aura (particularly, ChromeOS) in any way.

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

[jdduke (slow), 2014-05-20 18:51:17]

sadrul@: I'd like to get your thoughts on this change before I proceed any
further.  On the whole, I think it's a win and fits nicely with the
|InputRouterClient::FilterInputEvent()| infrastructure.   Don't be cowed by the
line count; most of it was just moving unit tests from RWHI to RWHVAura (I
really wish we had some standalone OverscrollController unit tests, but for now
hopefully this is good enough).  

There's still one test failing that I think has to do with the overscroll client
bounds, still looking into it.  There's also a question I have about
navigation_entry_screenshot_manager.cc (included in the comment below).

https://codereview.chromium.org/291003002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/291003002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:102: /*if
(!static_cast<RenderViewHostImpl*>
Is there a better way to early out here?

[jdduke (slow), 2014-05-20 18:58:46]

I should also note one routing change here.  Previously, as soon as an
overscroll started we would feed gestures directly to the OverscrollController,
rather than passing them through the GestureEventQueue.  In this patch, events
will always pass first through the GestureEventQueue.  The only meaningful
difference I can think of here is that you'll get debouncing before the event
hits the OverscrollController, but I have a hard time time seeing how that could
break things.

[jdduke (slow), 2014-05-22 15:46:24]

sadrul@: Should I find a different reviewer?

[sadrul, 2014-05-22 16:18:27]

This looks reasonable to me.

We have at various times considered using the OC on other platforms (e.g. mac),
so I was hesitant to take this into RWHVAura. But looking at this change, the
cleanup in the input-router is nice, and the RWHVAura already does some things
directly with the overscroll-controller that other ports would also have to do.
So moving more things in there isn't making this a lot worse.

https://codereview.chromium.org/291003002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/291003002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:105: }*/
The screenshots are used only when overscroll is turned on at the moment. This
is to avoid having to take the screenshots if they are not going to be used.

[jdduke (slow), 2014-05-22 16:21:31]

https://codereview.chromium.org/291003002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/291003002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:105: }*/
On 2014/05/22 16:18:27, sadrul wrote:
> The screenshots are used only when overscroll is turned on at the moment. This
> is to avoid having to take the screenshots if they are not going to be used.

Any ideas on another way to plumb through this data? I'd hate to do some ugly
casting if this can be used on platforms besides Aura (I believe we compile it
for all platforms?).

[sadrul, 2014-05-22 16:53:19]

https://codereview.chromium.org/291003002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/291003002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:105: }*/
On 2014/05/22 16:21:31, jdduke wrote:
> On 2014/05/22 16:18:27, sadrul wrote:
> > The screenshots are used only when overscroll is turned on at the moment.
This
> > is to avoid having to take the screenshots if they are not going to be used.
> 
> Any ideas on another way to plumb through this data? I'd hate to do some ugly
> casting if this can be used on platforms besides Aura (I believe we compile it
> for all platforms?).

We compile it on all platforms, yeah.

Maybe we could have a static bool OverscrollController::IsEnabled() that looks
at the cmd-line flag?

[jdduke (slow), 2014-05-22 19:29:37]

Alright, I fixed the bad test (turns out the touches were getting fed to the
gesture detector as I had inadvertently added a parent host, or something like
that =/), and added what I hope will work for the screenshot navigation
early-out.  Let me know if there are other reviewers in particular I should add,
otherwise I'll wait for your review before adding additional owners. Thanks.

https://codereview.chromium.org/291003002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/291003002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:105: }*/
On 2014/05/22 16:53:20, sadrul wrote:
> On 2014/05/22 16:21:31, jdduke wrote:
> > On 2014/05/22 16:18:27, sadrul wrote:
> > > The screenshots are used only when overscroll is turned on at the moment.
> This
> > > is to avoid having to take the screenshots if they are not going to be
used.
> > 
> > Any ideas on another way to plumb through this data? I'd hate to do some
ugly
> > casting if this can be used on platforms besides Aura (I believe we compile
it
> > for all platforms?).
> 
> We compile it on all platforms, yeah.
> 
> Maybe we could have a static bool OverscrollController::IsEnabled() that looks
> at the cmd-line flag?

Hmm, I started down that road and found some runtime cases where it starts to
get messy.  I added a |CanOverscrollContent()| to the nav delegate that I think
gets us most of the way there (and the method is actually already implemented
generically in the WebContentsImpl).

[sadrul, 2014-05-27 15:37:04]

The changes look good. The test failures in the tries look related though. Do
you mind taking a look and see what needs to change to unbreak?

https://codereview.chromium.org/291003002/diff/80001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_configuration.cc (right):

https://codereview.chromium.org/291003002/diff/80001/ui/events/gestures/gestu...
ui/events/gestures/gesture_configuration.cc:49: // scroll end detection on the
Pixel are resolved, crbug.com/353702.
The debounce code was not added because of noisy touches. It was added because
of a scrolling behaviour where the finger actually leaves the touchscreen
briefly (start scrolling with your knuckle, and roll the finger as you scroll up
... weird behaviour, I know!)

[jdduke (slow), 2014-05-28 17:29:45]

OK, tests fixed... looks like the remaining failures are flakes or residual
trybot problems?

[sadrul, 2014-06-02 00:04:16]

On 2014/05/28 17:29:45, jdduke wrote:
> OK, tests fixed... looks like the remaining failures are flakes or residual
> trybot problems?

Some of the telemetry crashes on Windows seem related :( (I see
content::RenderWidgetHostImpl::DecrementInFlightEventCount in the stack-trace).

[jdduke (slow), 2014-06-02 17:03:42]

On 2014/06/02 00:04:16, sadrul wrote:
> On 2014/05/28 17:29:45, jdduke wrote:
> > OK, tests fixed... looks like the remaining failures are flakes or residual
> > trybot problems?
> 
> Some of the telemetry crashes on Windows seem related :( (I see
> content::RenderWidgetHostImpl::DecrementInFlightEventCount in the
stack-trace).

Of course you're right, sorry, should have looked at all of the failures to see
the cause.  Should be resolved now.

[sadrul, 2014-06-02 20:56:00]

LGTM

https://codereview.chromium.org/291003002/diff/110001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/291003002/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1792:
DCHECK(in_flight_event_count_ >= 0);
DCHECK_GE

[jdduke (slow), 2014-06-02 21:26:11]

sky@: Owner review for content/browser/frame_host and
content/browser/web_contents? Thanks.

[sky, 2014-06-02 23:24:46]

LGTM

[jdduke (slow), 2014-06-02 23:25:08]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-06-02 23:25:54]

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

[commit-bot: I haz the power, 2014-06-03 10:38:05]

Change committed as 274462