OverscrollController consumes scroll updates only during gesture-nav.

OverscrollController consumes scroll updates only during gesture-nav.

Previously the OverscrollController consume all scroll updates, which
broke touchmove throttling.

The test ensures that touchmove throttling interacts correctly we
gesture-nav.

BUG=403329
TEST=WebContentsViewAuraTest.OverscrollNavigationTouchThrottling

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

[tdresser, 2014-08-15 23:10:33]

I'm not particularly happy with the state of the browser test, but this is an
M37 stable blocker, so I'm hoping to find a solution reasonably quickly. Any
ideas on how to clean this up?

It may make sense to land this as is (with a different TimeDelta in
|GiveItSomeTime|), and then try to clean things up later.

https://codereview.chromium.org/478023002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/478023002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:49:
base::TimeDelta::FromMillisecondsD(0.001));
0.001ms is obviously a terrible value, I'm only using it to reduce the chance
that flaky behavior will pass the trybots.

https://codereview.chromium.org/478023002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:888:
WaitAFrame();
Dana, these WaitAFrame() calls are obviously terrible, with the current
implementation of WaitAFrame(). I'm not sure what else to wait on though. Just
waiting on the events acks doesn't do the trick in many cases. Is there a better
way to wait until the next frame?

[tdresser, 2014-08-18 19:11:43]

On 2014/08/15 23:10:33, tdresser wrote:
> I'm not particularly happy with the state of the browser test, but this is an
> M37 stable blocker, so I'm hoping to find a solution reasonably quickly. Any
> ideas on how to clean this up?
> 
> It may make sense to land this as is (with a different TimeDelta in
> |GiveItSomeTime|), and then try to clean things up later.
> 
>
https://codereview.chromium.org/478023002/diff/1/content/browser/web_contents...
> File content/browser/web_contents/web_contents_view_aura_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/478023002/diff/1/content/browser/web_contents...
> content/browser/web_contents/web_contents_view_aura_browsertest.cc:49:
> base::TimeDelta::FromMillisecondsD(0.001));
> 0.001ms is obviously a terrible value, I'm only using it to reduce the chance
> that flaky behavior will pass the trybots.
> 
>
https://codereview.chromium.org/478023002/diff/1/content/browser/web_contents...
> content/browser/web_contents/web_contents_view_aura_browsertest.cc:888:
> WaitAFrame();
> Dana, these WaitAFrame() calls are obviously terrible, with the current
> implementation of WaitAFrame(). I'm not sure what else to wait on though. Just
> waiting on the events acks doesn't do the trick in many cases. Is there a
better
> way to wait until the next frame?

Talked to Dana, I'm inclined to leave the current behavior as is for now.
Perhaps at some point in the future, these tests should be reworked. Dana
suggested waiting to get back the LatencyInfo, so you know exactly where the
event has been.

ben@, can you take a look at content/browser/web_contents?
sadrul@, can you take a look at content/browser/renderer_host, and
overscroll_navigation.html?

[sadrul, 2014-08-18 22:54:09]

https://codereview.chromium.org/478023002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/478023002/diff/20001/content/browser/renderer...
content/browser/renderer_host/overscroll_controller.cc:216:
ProcessOverscroll(wheel.deltaX * wheel.accelerationRatioX,
How about here?

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/gesture_nav_simple.h (right):

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/gesture_nav_simple.h:40: // Returns true if
the scroll update was consumed.
Remove. We generally do not dup the comment for OVERRIDEs.

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1301: return
translate.x() || translate.y();
return !translate.IsZero();

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.h (right):

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.h:142: // Returns true if
the scroll update was consumed.
remove

[tdresser, 2014-08-19 12:51:38]

https://codereview.chromium.org/478023002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/478023002/diff/20001/content/browser/renderer...
content/browser/renderer_host/overscroll_controller.cc:216:
ProcessOverscroll(wheel.deltaX * wheel.accelerationRatioX,
On 2014/08/18 22:54:08, sadrul wrote:
> How about here?

Done.

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/gesture_nav_simple.h (right):

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/gesture_nav_simple.h:40: // Returns true if
the scroll update was consumed.
On 2014/08/18 22:54:09, sadrul wrote:
> Remove. We generally do not dup the comment for OVERRIDEs.

Done.

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1301: return
translate.x() || translate.y();
On 2014/08/18 22:54:09, sadrul wrote:
> return !translate.IsZero();

Done.

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.h (right):

https://codereview.chromium.org/478023002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.h:142: // Returns true if
the scroll update was consumed.
On 2014/08/18 22:54:09, sadrul wrote:
> remove

Done.

[tdresser, 2014-08-19 16:24:25]

sky@, can you take a look at content/browser/web_contents? ben@ is OOO.

Looks like the browsertest is flaky on windows (just like all the other
overscroll tests...)

I'll disable it on Windows.

[sadrul, 2014-08-19 16:51:30]

lgtm

[sky, 2014-08-19 19:39:50]

https://codereview.chromium.org/478023002/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/478023002/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:44: void
GiveItSomeTime() {
This is awful. Don't do this. Isn't there an observer or something you can add
to track what you need?

[tdresser, 2014-08-19 19:49:35]

On 2014/08/19 19:39:50, sky wrote:
>
https://codereview.chromium.org/478023002/diff/60001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_view_aura_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/478023002/diff/60001/content/browser/web_cont...
> content/browser/web_contents/web_contents_view_aura_browsertest.cc:44: void
> GiveItSomeTime() {
> This is awful. Don't do this. Isn't there an observer or something you can add
> to track what you need?

I agree that this is awful.
I've talked with sadrul@ and dana@, and I haven't found any solution that isn't
a significant amount of effort.

This is the approach that similar browsertests take.

Long term we definitely should clean this up, but this is blocking M37, so I
intend to land this as is, and file a bug to clean this up.

Does that sounds reasonable?

[sky, 2014-08-19 19:50:54]

Add a TODO and LGTM

On Tue, Aug 19, 2014 at 12:49 PM,  <tdresser@chromium.org> wrote:
> On 2014/08/19 19:39:50, sky wrote:
>
>
https://codereview.chromium.org/478023002/diff/60001/content/browser/web_cont...
>>
>> File content/browser/web_contents/web_contents_view_aura_browsertest.cc
>
> (right):
>
>
>
https://codereview.chromium.org/478023002/diff/60001/content/browser/web_cont...
>>
>> content/browser/web_contents/web_contents_view_aura_browsertest.cc:44:
>> void
>> GiveItSomeTime() {
>> This is awful. Don't do this. Isn't there an observer or something you can
>> add
>> to track what you need?
>
>
> I agree that this is awful.
> I've talked with sadrul@ and dana@, and I haven't found any solution that
> isn't
> a significant amount of effort.
>
> This is the approach that similar browsertests take.
>
> Long term we definitely should clean this up, but this is blocking M37, so I
> intend to land this as is, and file a bug to clean this up.
>
> Does that sounds reasonable?
>
>
>
> https://codereview.chromium.org/478023002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-08-19 19:56:07]

Add TODO and LGTM

[tdresser, 2014-08-19 21:03:10]

On 2014/08/19 19:56:07, sky wrote:
> Add TODO and LGTM

TODO added, and bug filed.

[tdresser, 2014-08-19 21:03:14]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-08-19 21:04:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/478023002/80001

[commit-bot: I haz the power, 2014-08-19 22:55:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-19 23:24:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-19 23:24:09]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tdresser, 2014-08-20 02:34:45]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-08-20 02:35:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/478023002/80001

[commit-bot: I haz the power, 2014-08-20 03:17:03]

Committed patchset #5 (80001) as 290740

[danakj, 2015-04-02 18:41:55]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-04-02 18:41:56]

https://codereview.chromium.org/478023002/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/478023002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:44: //
TODO(tdresser): Find a way to avoid sleeping like this. See crbug.com/405282
Would be awesome to follow up on this. I keep seeing this pattern being copy
pasted.