Move overscroll logic out of EventHandler.

Move overscroll logic out of EventHandler.

Overscrolling now happens from the viewport apply scroll callback so it doesn't
have much connection to EventHandler. I've moved all the overscrolling logic
into a new OverscrollController class, owned by FrameHost, that handles related
state and forwards the calls to the content layer.

This patch should have no behavior changes.

BUG=591124
Committed: https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46
Cr-Commit-Position: refs/heads/master@{#390150}

[bokan, 2016-04-23 16:54:43]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-23 16:54:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915783002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915783002/1

[bokan, 2016-04-25 13:44:10]

Description was changed from

==========
Move overscroll logic out of EventHandler.

Overscrolling now happens from the viewport apply scroll callback so it doesn't
have much connection to EventHandler. I've moved all the overscrolling logic
into a new OverscrollController class, owned by FrameHost, that handles related
state and forwards the calls to the content layer.

This patch should have no behavior changes.

BUG=591124
==========

to

==========
Move overscroll logic out of EventHandler.

Overscrolling now happens from the viewport apply scroll callback so it doesn't
have much connection to EventHandler. I've moved all the overscrolling logic
into a new OverscrollController class, owned by FrameHost, that handles related
state and forwards the calls to the content layer.

This patch should have no behavior changes.

BUG=591124
==========

[bokan, 2016-04-25 13:44:10]

bokan@chromium.org changed reviewers:
+ dtapuska@chromium.org

[bokan, 2016-04-25 13:44:21]

ptal.

https://codereview.chromium.org/1915783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/OverscrollController.h
(right):

https://codereview.chromium.org/1915783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/OverscrollController.h:48:
WeakMember<VisualViewport> m_visualViewport;
This should really be `const VisualViewport` but WeakMember blows up with a
const type (Memeber works fine). I've sent off a query to oilpan-reviews@ about
this but made it non-const in the mean time.

[bokan, 2016-04-26 01:07:59]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-26 01:08:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915783002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915783002/80001

[bokan, 2016-04-26 01:08:42]

https://codereview.chromium.org/1915783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/OverscrollController.h
(right):

https://codereview.chromium.org/1915783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/OverscrollController.h:48:
WeakMember<VisualViewport> m_visualViewport;
On 2016/04/25 13:44:21, bokan wrote:
> This should really be `const VisualViewport` but WeakMember blows up with a
> const type (Memeber works fine). I've sent off a query to oilpan-reviews@
about
> this but made it non-const in the mean time.

Oilpan team fixed so I made this const again.

[commit-bot: I haz the power, 2016-04-26 01:22:12]

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

[commit-bot: I haz the power, 2016-04-26 01:22:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2016-04-26 02:31:25]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-26 02:32:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915783002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915783002/100001

[commit-bot: I haz the power, 2016-04-26 05:00:49]

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

[commit-bot: I haz the power, 2016-04-26 05:00:50]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2016-04-26 13:52:48]

On 2016/04/26 05:00:50, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

lgtm % nits

[dtapuska, 2016-04-26 13:53:11]

https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameHost.cpp (right):

https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameHost.cpp:100: OverscrollController&
FrameHost::overscrollController() const
I got shamed for this last week and was explicitly told:
https://google.github.io/styleguide/cppguide.html#Use_of_const

explicitly; 'do not return non-const reference to a data member'

Now looking at again the interpretation is a little weird; but I'd assume you
could remove the const from the method definition and it would be fine.

Context:
https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...

But I agree this matches what other methods are doing here as well so at least
it is consistently not following the guidance.

https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:2614:
host->overscrollController().resetAccumulated(true, true);
Can you add /*resetX*/, /*resetY*/ comments so it is clear what the booleans
represent?

[bokan, 2016-04-26 16:09:53]

Fixed up FrameHost to be const-correct. Still l-g-t-m?

https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameHost.cpp (right):

https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameHost.cpp:100: OverscrollController&
FrameHost::overscrollController() const
On 2016/04/26 13:53:11, dtapuska wrote:
> I got shamed for this last week and was explicitly told:
> https://google.github.io/styleguide/cppguide.html#Use_of_const
> 
> explicitly; 'do not return non-const reference to a data member'
> 
> Now looking at again the interpretation is a little weird; but I'd assume you
> could remove the const from the method definition and it would be fine.
> 
> Context:
>
https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
> 
> But I agree this matches what other methods are doing here as well so at least
> it is consistently not following the guidance.

Yah, I was just being consistent but it's true that this isn't const-correct. In
general, lots of Blink code isn't const-correct and writing new code is made
more difficult because of this. I've added const and non-const overloads for
each method in this class to buck the trend.

https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:2614:
host->overscrollController().resetAccumulated(true, true);
On 2016/04/26 13:53:11, dtapuska wrote:
> Can you add /*resetX*/, /*resetY*/ comments so it is clear what the booleans
> represent?

Done. (Used vars instead of comments)

[dtapuska, 2016-04-27 16:13:10]

On 2016/04/26 16:09:53, bokan wrote:
> Fixed up FrameHost to be const-correct. Still l-g-t-m?
> 
>
https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/FrameHost.cpp (right):
> 
>
https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameHost.cpp:100: OverscrollController&
> FrameHost::overscrollController() const
> On 2016/04/26 13:53:11, dtapuska wrote:
> > I got shamed for this last week and was explicitly told:
> > https://google.github.io/styleguide/cppguide.html#Use_of_const
> > 
> > explicitly; 'do not return non-const reference to a data member'
> > 
> > Now looking at again the interpretation is a little weird; but I'd assume
you
> > could remove the const from the method definition and it would be fine.
> > 
> > Context:
> >
>
https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
> > 
> > But I agree this matches what other methods are doing here as well so at
least
> > it is consistently not following the guidance.
> 
> Yah, I was just being consistent but it's true that this isn't const-correct.
In
> general, lots of Blink code isn't const-correct and writing new code is made
> more difficult because of this. I've added const and non-const overloads for
> each method in this class to buck the trend.
> 
>
https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1915783002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/EventHandler.cpp:2614:
> host->overscrollController().resetAccumulated(true, true);
> On 2016/04/26 13:53:11, dtapuska wrote:
> > Can you add /*resetX*/, /*resetY*/ comments so it is clear what the booleans
> > represent?
> 
> Done. (Used vars instead of comments)

lgtm

[bokan, 2016-04-27 17:02:06]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-04-27 17:02:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915783002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915783002/140001

[commit-bot: I haz the power, 2016-04-27 19:39:36]

Description was changed from

==========
Move overscroll logic out of EventHandler.

Overscrolling now happens from the viewport apply scroll callback so it doesn't
have much connection to EventHandler. I've moved all the overscrolling logic
into a new OverscrollController class, owned by FrameHost, that handles related
state and forwards the calls to the content layer.

This patch should have no behavior changes.

BUG=591124
==========

to

==========
Move overscroll logic out of EventHandler.

Overscrolling now happens from the viewport apply scroll callback so it doesn't
have much connection to EventHandler. I've moved all the overscrolling logic
into a new OverscrollController class, owned by FrameHost, that handles related
state and forwards the calls to the content layer.

This patch should have no behavior changes.

BUG=591124
==========

[commit-bot: I haz the power, 2016-04-27 19:39:37]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-30 17:11:58]

Patchset 8 (id:??) landed as
https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46
Cr-Commit-Position: refs/heads/master@{#390150}

[commit-bot: I haz the power, 2016-04-30 17:51:17]

Description was changed from

==========
Move overscroll logic out of EventHandler.

Overscrolling now happens from the viewport apply scroll callback so it doesn't
have much connection to EventHandler. I've moved all the overscrolling logic
into a new OverscrollController class, owned by FrameHost, that handles related
state and forwards the calls to the content layer.

This patch should have no behavior changes.

BUG=591124
==========

to

==========
Move overscroll logic out of EventHandler.

Overscrolling now happens from the viewport apply scroll callback so it doesn't
have much connection to EventHandler. I've moved all the overscrolling logic
into a new OverscrollController class, owned by FrameHost, that handles related
state and forwards the calls to the content layer.

This patch should have no behavior changes.

BUG=591124

Committed: https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46
Cr-Commit-Position: refs/heads/master@{#390150}
==========