Seperate horizontal and vertical overscrolling

Seperate horizontal and vertical overscrolling

This CL splits the concept of the minimum overscroll threshold into a
vertical and horizontal value. This allows configuring these overscrolls
independently since they have different behaviour. This CL also changes
the behaviour of the OverscrollController when a render has consumed the
start of a scroll. The existing behaviour of not performing horizontal
overscrolls is preserved. The vertical case is changed to allow the
scroll end effect to start on a scroll that moved content.

BUG=257121
TEST=Confirmed on device that scroll end effect occurs immediately when
     the end of the page is reached via adding local patch for overscroll effect.
     Confirmed that gesture nav behaviour does not change.
     Updated a bunch of tests and ran them.

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

[rharrison, 2013-07-04 20:01:49]

PTAL

[sadrul, 2013-07-04 21:16:35]

https://codereview.chromium.org/18603008/diff/1/chrome/browser/ui/gesture_pre...
File chrome/browser/ui/gesture_prefs_observer_factory_aura.cc (right):

https://codereview.chromium.org/18603008/diff/1/chrome/browser/ui/gesture_pre...
chrome/browser/ui/gesture_prefs_observer_factory_aura.cc:58:
OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START },
You need to hook up the vertical threshold into the chrome://gesture UI

https://codereview.chromium.org/18603008/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/18603008/diff/1/content/browser/renderer_host...
content/browser/renderer_host/overscroll_controller.cc:306: if (overscroll_mode_
== OVERSCROLL_NONE) {
I believe in this changed code, if a scroll event comes with 3px horizontal
scroll and 1px vertical scroll, then it will still trigger horizontal
overscroll. We wouldn't want that.

[rharrison, 2013-07-17 18:59:48]

https://codereview.chromium.org/18603008/diff/1/chrome/browser/ui/gesture_pre...
File chrome/browser/ui/gesture_prefs_observer_factory_aura.cc (right):

https://codereview.chromium.org/18603008/diff/1/chrome/browser/ui/gesture_pre...
chrome/browser/ui/gesture_prefs_observer_factory_aura.cc:58:
OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START },
On 2013/07/04 21:16:35, sadrul wrote:
> You need to hook up the vertical threshold into the chrome://gesture UI

Done.

https://codereview.chromium.org/18603008/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/18603008/diff/1/content/browser/renderer_host...
content/browser/renderer_host/overscroll_controller.cc:306: if (overscroll_mode_
== OVERSCROLL_NONE) {
On 2013/07/04 21:16:35, sadrul wrote:
> I believe in this changed code, if a scroll event comes with 3px horizontal
> scroll and 1px vertical scroll, then it will still trigger horizontal
> overscroll. We wouldn't want that.

Good catch, I have added checks to make sure that the overscroll is over the
threshold in the direction that is trying to be set.

[sadrul, 2013-07-18 22:51:41]

I think this is pretty close. In the next iteration, please add owners.

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/overscroll_controller.cc:309:
SetOverscrollMode(new_mode);
Hm. There's a bug here, even in the current code. If new_mode is also NONE here,
then we end up triggering OnOverscrollUpdate() below. Lines 308-313 should be:

  if (overscroll_mode_ == OVERSCROLL_NONE)
    SetOverscrollMode(new_mode);
  else if (new_mode != overscroll_mode_)
    SetOverscrollMode(OVERSCROLL_NONE);

  if (overscroll_mode_ == OVERSCROLL_NONE)
    return;

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_unittest.cc (left):

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:3542: }
Can you update this test to test that there is no horizontal overscroll in this
case, but there is vertical overscroll?

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:3221:
EXPECT_EQ(OVERSCROLL_WEST, host_->overscroll_mode());
Why does the expectation change here?

[rharrison, 2013-07-23 14:36:02]

Responded to comments from sadrul

Adding sky for OWNERS

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/overscroll_controller.cc:309:
SetOverscrollMode(new_mode);
On 2013/07/18 22:51:41, sadrul wrote:
> Hm. There's a bug here, even in the current code. If new_mode is also NONE
here,
> then we end up triggering OnOverscrollUpdate() below. Lines 308-313 should be:
> 
>   if (overscroll_mode_ == OVERSCROLL_NONE)
>     SetOverscrollMode(new_mode);
>   else if (new_mode != overscroll_mode_)
>     SetOverscrollMode(OVERSCROLL_NONE);
> 
>   if (overscroll_mode_ == OVERSCROLL_NONE)
>     return;

Done.

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_unittest.cc (left):

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:3542: }
On 2013/07/18 22:51:41, sadrul wrote:
> Can you update this test to test that there is no horizontal overscroll in
this
> case, but there is vertical overscroll?

Done.

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/18603008/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:3221:
EXPECT_EQ(OVERSCROLL_WEST, host_->overscroll_mode());
On 2013/07/18 22:51:41, sadrul wrote:
> Why does the expectation change here?

I don't know... this appears to be wrong now. Reverting it.

[rharrison, 2013-07-23 14:37:01]

Adding ben for OWNERS on content/public/browser/overscroll_configuration.h

[sadrul, 2013-07-23 14:42:22]

LGTM

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

https://codereview.chromium.org/18603008/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:3576: // nitiate
overscroll because the scroll was in the vertical direction even
*initiate

https://codereview.chromium.org/18603008/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:3581:
EXPECT_EQ(OVERSCROLL_NORTH, host_->overscroll_mode());
Send a sufficiently large unprocessed horizontal scroll after this, and show
that the overscroll-mode goes back to _NONE.

[rharrison, 2013-07-23 14:50:07]

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

https://codereview.chromium.org/18603008/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:3576: // nitiate
overscroll because the scroll was in the vertical direction even
On 2013/07/23 14:42:22, sadrul wrote:
> *initiate

Done.

https://codereview.chromium.org/18603008/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:3581:
EXPECT_EQ(OVERSCROLL_NORTH, host_->overscroll_mode());
On 2013/07/23 14:42:22, sadrul wrote:
> Send a sufficiently large unprocessed horizontal scroll after this, and show
> that the overscroll-mode goes back to _NONE.

Done.

[sky, 2013-07-23 18:03:19]

LGTM

[rharrison, 2013-07-24 13:42:08]

On 2013/07/23 18:03:19, sky wrote:
> LGTM

Looks like the changes I am waiting on ben@ to review are purely mechanical. Is
it acceptable to TBR this CL?

[rharrison, 2013-07-24 15:07:52]

Switching to jam for OWNERS review of content/public

[rharrison, 2013-07-24 15:20:30]

Adding dbeam for OWNERS review of chrome/browser/resources/gesture_config.js

[jam, 2013-07-24 18:06:30]

On 2013/07/24 15:07:52, rharrison wrote:
> Switching to jam for OWNERS review of content/public

lgtm

[rharrison, 2013-07-24 18:12:46]

Removed dbeam from review, since I already have an OWNER for the .js file
apparently...

[Dan Beam, 2013-07-24 18:23:54]

lgtm

[commit-bot: I haz the power, 2013-07-25 19:24:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/18603008/89001

[commit-bot: I haz the power, 2013-07-25 22:43:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/18603008/89001

[commit-bot: I haz the power, 2013-07-25 23:15:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/18603008/89001

[commit-bot: I haz the power, 2013-07-26 03:38:23]

Change committed as 213753