Update RootFrameViewport::userScroll to distribute scrolls between viewports.

Update RootFrameViewport::userScroll to distribute scrolls between viewports.

This is needed to delegate main thread user scroll animations to the compositor.

The problem with calling RFV's scroll animator is that the animation complete
notification from the compositor goes to the GraphicLayers's scrollable area
which is the FrameView and not RFV.

No tests added because many layout tests cover the RFV::userScroll method.

BUG=552556
Committed: https://crrev.com/4206032fc8056015a6c8cce304c2b6997644fd9e
Cr-Commit-Position: refs/heads/master@{#364583}

[ymalik, 2015-12-03 21:51:34]

Description was changed from

==========
Update RootFrameViewport::userScroll to distribute scrolls between viewports.

This is needed to delegate main thread user scroll animations to the compositor.

The problem with calling RFV's scroll animator is that the animation complete
notification from the compositor goes to the GraphicLayers's scrollable area
which is the FrameView and not RFV.

No tests added because many layout tests cover the RFV::userScroll method.

BUG=552556
==========

to

==========
Update RootFrameViewport::userScroll to distribute scrolls between viewports.

This is needed to delegate main thread user scroll animations to the compositor.

The problem with calling RFV's scroll animator is that the animation complete
notification from the compositor goes to the GraphicLayers's scrollable area
which is the FrameView and not RFV.

No tests added because many layout tests cover the RFV::userScroll method.

BUG=552556
==========

[ymalik, 2015-12-03 21:51:34]

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

[bokan, 2015-12-04 19:39:02]

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:261: delta *= step;
I don't think you need to do this. I believe the fact that the
ScrollAnimator::userScroll returns the unused delta in terms of steps rather
than pixels is a bug. I suspect the only reason we haven't seen it in real
behaviour is that overscrolling only happens on things with step 1 like touch
gestures and mac wheel events.

I'd remove this, fix ScrollAnimator to return the correct values, and use
primary/secondary.userScroll below. That should simplify this quite a bit.

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:273: if
(pResult.unusedScrollDelta == 0)
Use ScrollResultOneDimensional::didScroll for this check instead

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:282: return
ScrollResultOneDimensional(true, sResult.unusedScrollDelta / step);
You can merge the last two returns together in the first parameter. Also, wrt
above comment, don't divide the step out of the delta.

[ymalik, 2015-12-04 20:11:01]

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:261: delta *= step;
On 2015/12/04 19:39:02, bokan wrote:
> I don't think you need to do this. I believe the fact that the
> ScrollAnimator::userScroll returns the unused delta in terms of steps rather
> than pixels is a bug. I suspect the only reason we haven't seen it in real
> behaviour is that overscrolling only happens on things with step 1 like touch
> gestures and mac wheel events.
> 
> I'd remove this, fix ScrollAnimator to return the correct values, and use
> primary/secondary.userScroll below. That should simplify this quite a bit.

So the current flow is as follows
delta -> delta * step -> unused delta

Given that step >= 1, we are always actually scrolling more pixels than the
caller expects us to. It would be odd to return a higher unused delta than what
the function was called with. Ideally the userScroll method would get called
with the right pixels to scroll, but perhaps that's work for another CL. I just
want to unblock crbug.com/552556 and I can add a bug to clean this up on a whole
(along with your TODO on ScrollablaArea::userScrollHelper)?

[ymalik, 2015-12-04 20:13:54]

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:273: if
(pResult.unusedScrollDelta == 0)
On 2015/12/04 19:39:02, bokan wrote:
> Use ScrollResultOneDimensional::didScroll for this check instead

didScroll can be true while we still want to distribute unsusedScrollDelta to
the secondary viewport. This check specifically checks whether we need to
delegate the remainder of the scroll delta to the secondary viewport.

[bokan, 2015-12-04 20:42:52]

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:261: delta *= step;
On 2015/12/04 20:11:01, ymalik1 wrote:
> On 2015/12/04 19:39:02, bokan wrote:
> > I don't think you need to do this. I believe the fact that the
> > ScrollAnimator::userScroll returns the unused delta in terms of steps rather
> > than pixels is a bug. I suspect the only reason we haven't seen it in real
> > behaviour is that overscrolling only happens on things with step 1 like
touch
> > gestures and mac wheel events.
> > 
> > I'd remove this, fix ScrollAnimator to return the correct values, and use
> > primary/secondary.userScroll below. That should simplify this quite a bit.
> 
> So the current flow is as follows
> delta -> delta * step -> unused delta
> 
> Given that step >= 1, we are always actually scrolling more pixels than the
> caller expects us to. It would be odd to return a higher unused delta than
what
> the function was called with. Ideally the userScroll method would get called
> with the right pixels to scroll, but perhaps that's work for another CL. I
just
> want to unblock crbug.com/552556 and I can add a bug to clean this up on a
whole
> (along with your TODO on ScrollablaArea::userScrollHelper)? 

Ok, just had a look at ScrollAnimatorMac and it seems that does the same thing
so this is fine for now. Please add a TODO.

You still have a problem though :). It seems ScrollAnimator::userScroll always
returns 0 for unusedScrollDelta if its going to animate. You'll have to fix that
on the Animator side.

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:265: ScrollableArea&
primary = !m_invertScrollOrder ? layoutViewport() : visualViewport();
Replace primary with visual and secondary with layout. I could have sworn I
removed this weeks ago but I just checked and the CL is sitting uncommitted in
my queue with all the l-g-t-m :(

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:273: if
(pResult.unusedScrollDelta == 0)
On 2015/12/04 20:13:54, ymalik1 wrote:
> On 2015/12/04 19:39:02, bokan wrote:
> > Use ScrollResultOneDimensional::didScroll for this check instead
> 
> didScroll can be true while we still want to distribute unsusedScrollDelta to
> the secondary viewport. This check specifically checks whether we need to
> delegate the remainder of the scroll delta to the secondary viewport.

Right, right, my bad.

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:282: return
ScrollResultOneDimensional(true, sResult.unusedScrollDelta / step);
You can still merge these two together (in fact, your second one should be a sum
of unused deltas, right?

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:138: orientation =
VerticalScrollbar;
No need for the temp, just return VerticalScrollbar or HorizontalScrollbar

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:279: static
ScrollbarOrientation scrollbarOrientationFromDirection(ScrollDirectionPhysical);
Why public? Also, just make it a normal method like scrollStep

[ymalik, 2015-12-07 17:53:14]

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:261: delta *= step;
On 2015/12/04 20:42:52, bokan wrote:
> On 2015/12/04 20:11:01, ymalik1 wrote:
> > On 2015/12/04 19:39:02, bokan wrote:
> > > I don't think you need to do this. I believe the fact that the
> > > ScrollAnimator::userScroll returns the unused delta in terms of steps
rather
> > > than pixels is a bug. I suspect the only reason we haven't seen it in real
> > > behaviour is that overscrolling only happens on things with step 1 like
> touch
> > > gestures and mac wheel events.
> > > 
> > > I'd remove this, fix ScrollAnimator to return the correct values, and use
> > > primary/secondary.userScroll below. That should simplify this quite a bit.
> > 
> > So the current flow is as follows
> > delta -> delta * step -> unused delta
> > 
> > Given that step >= 1, we are always actually scrolling more pixels than the
> > caller expects us to. It would be odd to return a higher unused delta than
> what
> > the function was called with. Ideally the userScroll method would get called
> > with the right pixels to scroll, but perhaps that's work for another CL. I
> just
> > want to unblock crbug.com/552556 and I can add a bug to clean this up on a
> whole
> > (along with your TODO on ScrollablaArea::userScrollHelper)? 
> 
> Ok, just had a look at ScrollAnimatorMac and it seems that does the same thing
> so this is fine for now. Please add a TODO.
> 
> You still have a problem though :). It seems ScrollAnimator::userScroll always
> returns 0 for unusedScrollDelta if its going to animate. You'll have to fix
that
> on the Animator side. 

Hmm, so the ScrollAnimator::userScroll returns 0 for unsedScrollDelta so that
the scroll latches and does not cause the parent layer to scroll. So we can't
reply on the unused delta returned by the animators. That's fine because we
already know how much each viewport can scroll. Updated to give
visualviewport.userscroll the portion of delta that it can scroll for sure.

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:265: ScrollableArea&
primary = !m_invertScrollOrder ? layoutViewport() : visualViewport();
On 2015/12/04 20:42:52, bokan wrote:
> Replace primary with visual and secondary with layout. I could have sworn I
> removed this weeks ago but I just checked and the CL is sitting uncommitted in
> my queue with all the l-g-t-m :(

Done.

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:282: return
ScrollResultOneDimensional(true, sResult.unusedScrollDelta / step);
On 2015/12/04 20:42:52, bokan wrote:
> You can still merge these two together (in fact, your second one should be a
sum
> of unused deltas, right?

Merged. layoutResult.unusedScrollDelta is the amount out of delta that we
couldn't scroll.

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:138: orientation =
VerticalScrollbar;
On 2015/12/04 20:42:52, bokan wrote:
> No need for the temp, just return VerticalScrollbar or HorizontalScrollbar

Done.

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/1496693005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:279: static
ScrollbarOrientation scrollbarOrientationFromDirection(ScrollDirectionPhysical);
On 2015/12/04 20:42:52, bokan wrote:
> Why public? Also, just make it a normal method like scrollStep

Done.

[bokan, 2015-12-07 18:43:17]

https://codereview.chromium.org/1496693005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1496693005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:62: float
ScrollAnimator::usedScrollDelta(ScrollbarOrientation orientation, float
pixelDelta) const
I'd make this "computeTargetPosition" and use it in this and other scroll
animators' ::userScroll method so that we're not duplicating this code (if
someone changes the computation in userScroll they'd likely forget to update
this). You can then compare this to the animator's current position in
ScrollableArea::userScroll to get the delta.

[bokan, 2015-12-07 19:52:54]

https://codereview.chromium.org/1496693005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1496693005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:62: float
ScrollAnimator::usedScrollDelta(ScrollbarOrientation orientation, float
pixelDelta) const
On 2015/12/07 18:43:17, bokan wrote:
> I'd make this "computeTargetPosition" and use it in this and other scroll
> animators' ::userScroll method so that we're not duplicating this code (if
> someone changes the computation in userScroll they'd likely forget to update
> this). You can then compare this to the animator's current position in
> ScrollableArea::userScroll to get the delta.

ok, delta is fine, but can we call it 'computeDeltaToConsume' or something more
descriptive? imo, usedScrollDelta is a bit confusing.

[ymalik, 2015-12-07 20:52:33]

https://codereview.chromium.org/1496693005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1496693005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:62: float
ScrollAnimator::usedScrollDelta(ScrollbarOrientation orientation, float
pixelDelta) const
On 2015/12/07 19:52:54, bokan wrote:
> On 2015/12/07 18:43:17, bokan wrote:
> > I'd make this "computeTargetPosition" and use it in this and other scroll
> > animators' ::userScroll method so that we're not duplicating this code (if
> > someone changes the computation in userScroll they'd likely forget to update
> > this). You can then compare this to the animator's current position in
> > ScrollableArea::userScroll to get the delta.
> 
> ok, delta is fine, but can we call it 'computeDeltaToConsume' or something
more
> descriptive? imo, usedScrollDelta is a bit confusing.

Renamed.

[bokan, 2015-12-08 22:23:42]

lgtm!

[ymalik, 2015-12-08 22:29:01]

ymalik@chromium.org changed reviewers:
+ jbroman@chromium.org

[ymalik, 2015-12-08 22:30:09]

+Jeremy for third_party/WebKit/Source/platform

Hey Jeremy, need your stamp for platform, logic has been stamped by bokan@

[jbroman, 2015-12-09 15:29:35]

lgtm with nits

https://codereview.chromium.org/1496693005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1496693005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:153: step =
pixelStep(orientation);
nit: This line is unreachable; please remove duplicate copy. (While you're here,
you could consider simply using "return" instead of having an intermediate
variable, too, but I won't push on that.)

[jbroman, 2015-12-09 15:31:22]

https://codereview.chromium.org/1496693005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1496693005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:153: step =
pixelStep(orientation);
On 2015/12/09 at 15:29:35, jbroman wrote:
> nit: This line is unreachable; please remove duplicate copy. (While you're
here, you could consider simply using "return" instead of having an intermediate
variable, too, but I won't push on that.)

If this line isn't duplicated in your code, please ignore. In Rietveld this
renders twice for me, but I now suspect that to be a bug in the UI.

[ymalik, 2015-12-09 15:59:42]

https://codereview.chromium.org/1496693005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1496693005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:153: step =
pixelStep(orientation);
On 2015/12/09 15:31:22, jbroman wrote:
> On 2015/12/09 at 15:29:35, jbroman wrote:
> > nit: This line is unreachable; please remove duplicate copy. (While you're
> here, you could consider simply using "return" instead of having an
intermediate
> variable, too, but I won't push on that.)
> 
> If this line isn't duplicated in your code, please ignore. In Rietveld this
> renders twice for me, but I now suspect that to be a bug in the UI.

Yes. Not duplicated. Moved to using return. Thanks!

[ymalik, 2015-12-09 21:55:42]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-12-09 21:55:42]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1496693005/#ps200001
(title: "worked on nits")

[commit-bot: I haz the power, 2015-12-09 21:56:21]

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

[commit-bot: I haz the power, 2015-12-09 23:46:41]

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

[commit-bot: I haz the power, 2015-12-09 23:46:42]

Exceeded global retry quota

[ymalik, 2015-12-10 05:04:01]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-12-10 05:04:15]

The CQ bit was unchecked by ymalik@chromium.org

[ymalik, 2015-12-10 05:06:43]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2015-12-10 05:07:41]

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

[commit-bot: I haz the power, 2015-12-10 05:41:02]

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

[commit-bot: I haz the power, 2015-12-10 05:41:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ymalik, 2015-12-10 16:36:33]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-12-10 16:36:33]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1496693005/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-12-10 16:37:20]

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

[commit-bot: I haz the power, 2015-12-10 20:42:29]

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

[commit-bot: I haz the power, 2015-12-10 20:42:30]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ymalik, 2015-12-11 01:57:56]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2015-12-11 01:59:28]

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

[commit-bot: I haz the power, 2015-12-11 02:08:13]

Description was changed from

==========
Update RootFrameViewport::userScroll to distribute scrolls between viewports.

This is needed to delegate main thread user scroll animations to the compositor.

The problem with calling RFV's scroll animator is that the animation complete
notification from the compositor goes to the GraphicLayers's scrollable area
which is the FrameView and not RFV.

No tests added because many layout tests cover the RFV::userScroll method.

BUG=552556
==========

to

==========
Update RootFrameViewport::userScroll to distribute scrolls between viewports.

This is needed to delegate main thread user scroll animations to the compositor.

The problem with calling RFV's scroll animator is that the animation complete
notification from the compositor goes to the GraphicLayers's scrollable area
which is the FrameView and not RFV.

No tests added because many layout tests cover the RFV::userScroll method.

BUG=552556
==========

[commit-bot: I haz the power, 2015-12-11 02:08:14]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-12-11 02:09:12]

Description was changed from

==========
Update RootFrameViewport::userScroll to distribute scrolls between viewports.

This is needed to delegate main thread user scroll animations to the compositor.

The problem with calling RFV's scroll animator is that the animation complete
notification from the compositor goes to the GraphicLayers's scrollable area
which is the FrameView and not RFV.

No tests added because many layout tests cover the RFV::userScroll method.

BUG=552556
==========

to

==========
Update RootFrameViewport::userScroll to distribute scrolls between viewports.

This is needed to delegate main thread user scroll animations to the compositor.

The problem with calling RFV's scroll animator is that the animation complete
notification from the compositor goes to the GraphicLayers's scrollable area
which is the FrameView and not RFV.

No tests added because many layout tests cover the RFV::userScroll method.

BUG=552556

Committed: https://crrev.com/4206032fc8056015a6c8cce304c2b6997644fd9e
Cr-Commit-Position: refs/heads/master@{#364583}
==========

[commit-bot: I haz the power, 2015-12-11 02:09:12]

Patchset 12 (id:??) landed as
https://crrev.com/4206032fc8056015a6c8cce304c2b6997644fd9e
Cr-Commit-Position: refs/heads/master@{#364583}