Let LayerTreeHostImpl::ScrollAnimated scroll the outer viewport.

Let LayerTreeHostImpl::ScrollAnimated scroll the outer viewport.

Scroll offset animations don't go through cc::Viewport, so we can't rely on it
to distribute to the outer viewport; FindScrollLayerForDeviceViewportPoint needs
to return the layer that actually scrolls.

This regressed with http://crrev.com/1365343002.

BUG=537393
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/de5abdb89c357265d78fc873978f1867f46f360a
Cr-Commit-Position: refs/heads/master@{#355125}

[skobes, 2015-10-19 22:00:12]

Patchset #2 (id:20001) has been deleted

[skobes, 2015-10-19 22:00:47]

Patchset #1 (id:1) has been deleted

[skobes, 2015-10-19 22:01:20]

Description was changed from

==========
[WIP] Fix smooth scrolling.

BUG=537393
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Let LayerTreeHostImpl::ScrollAnimated scroll the outer viewport.

This regressed with http://crrev.com/1365343002.

BUG=537393
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[skobes, 2015-10-19 22:13:00]

Description was changed from

==========
Let LayerTreeHostImpl::ScrollAnimated scroll the outer viewport.

This regressed with http://crrev.com/1365343002.

BUG=537393
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Let LayerTreeHostImpl::ScrollAnimated scroll the outer viewport.

Scroll offset animations don't go through cc::Viewport, so we can't rely on it
to distribute to the outer viewport; FindScrollLayerForDeviceViewportPoint needs
to return the layer that actually scrolls.

This regressed with http://crrev.com/1365343002.

BUG=537393
==========

[skobes, 2015-10-19 22:13:20]

skobes@chromium.org changed reviewers:
+ bokan@chromium.org, tdresser@chromium.org, ymalik@chromium.org

[bokan, 2015-10-19 22:24:23]

lgtm since we want to fix this for 47.

Longer term, I think we need to rethink how smooth scroll animations are handled
in CC - allowing them to be applied across 2 layers. As it is now, this will be
problematic not just on Android but on ChromeOS when the on screen keyboard
shows since the user wont be able to scroll to the bottom.

[skobes, 2015-10-19 22:26:57]

On 2015/10/19 22:24:23, bokan wrote:
> Longer term, I think we need to rethink how smooth scroll animations are
handled
> in CC - allowing them to be applied across 2 layers.

Agreed.  Maybe LayerImpl::OnScrollOffsetAnimated can hook into ApplyScroll
somehow? </handwaves>

[skobes, 2015-10-19 22:27:01]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2015-10-19 22:27:37]

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

[bokan, 2015-10-19 22:28:23]

On 2015/10/19 22:26:57, skobes wrote:
> On 2015/10/19 22:24:23, bokan wrote:
> > Longer term, I think we need to rethink how smooth scroll animations are
> handled
> > in CC - allowing them to be applied across 2 layers.
> 
> Agreed.  Maybe LayerImpl::OnScrollOffsetAnimated can hook into ApplyScroll
> somehow? </handwaves>

tdresser@ will probably have some useful insight if we can piggyback onto the
scroll customization machinery.

BTW, I'm not a CC owner :)

[skobes, 2015-10-19 22:29:30]

skobes@chromium.org changed reviewers:
+ aelias@chromium.org

[skobes, 2015-10-19 22:29:31]

+aelias for OWNERS

[ymalik, 2015-10-19 22:36:54]

Given that the CurrentlyScrolllingLayer can now be the outerViewport, I wonder
if it needs to be added here?
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

https://codereview.chromium.org/1418493003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418493003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2404: potentially_scrolling_layer_impl =
OuterViewportScrollLayer();
Instead of adding a new ScrollInputType, I wonder if its better to just check
for the smooth_scroll_enabled_ flag in the parent InputHandler class. That will
save use from doing Type = WHEEL || Type=ANIMATED_WHEEL every time. WDTY?

[skobes, 2015-10-19 22:37:06]

The CQ bit was unchecked by skobes@chromium.org

[commit-bot: I haz the power, 2015-10-19 22:37:16]

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

[commit-bot: I haz the power, 2015-10-19 22:37:17]

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

[skobes, 2015-10-19 23:03:33]

On 2015/10/19 22:36:54, ymalik1 wrote:
> Given that the CurrentlyScrolllingLayer can now be the outerViewport, I wonder
> if it needs to be added here?
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

It looks like this is only used for flings.  I've renamed it to be less
ambiguous.

>
https://codereview.chromium.org/1418493003/diff/40001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:2404: potentially_scrolling_layer_impl =
> OuterViewportScrollLayer();
> Instead of adding a new ScrollInputType, I wonder if its better to just check
> for the smooth_scroll_enabled_ flag in the parent InputHandler class. That
will
> save use from doing Type = WHEEL || Type=ANIMATED_WHEEL every time. WDTY?

Hmm, I'd rather not add plumbing to InputHandler for checking this.  There were
only two places doing type == WHEEL that needed updating so it doesn't seem that
burdensome.

[ymalik, 2015-10-19 23:07:22]

lgtm

[ymalik, 2015-10-19 23:07:38]

On 2015/10/19 23:07:22, ymalik1 wrote:
> lgtm

looks good thanks :)

[tdresser, 2015-10-20 12:29:40]

Test failures look real.

Thanks for figuring this out!

https://codereview.chromium.org/1418493003/diff/60001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/1418493003/diff/60001/cc/input/input_handler....
cc/input/input_handler.h:155: // root layer, or on some other sublayer.
Should we update this comment?

https://codereview.chromium.org/1418493003/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418493003/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2489: // behavior as ScrollBy to determine
which layer to animate, but we do not
We might want to update the comment here - we don't use the same bubbling
behavior as ScrollBy anymore.

[tdresser, 2015-10-20 12:29:59]

On 2015/10/20 12:29:40, tdresser wrote:
> Test failures look real.
> 
> Thanks for figuring this out!
> 
> https://codereview.chromium.org/1418493003/diff/60001/cc/input/input_handler.h
> File cc/input/input_handler.h (right):
> 
>
https://codereview.chromium.org/1418493003/diff/60001/cc/input/input_handler....
> cc/input/input_handler.h:155: // root layer, or on some other sublayer.
> Should we update this comment?
> 
>
https://codereview.chromium.org/1418493003/diff/60001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1418493003/diff/60001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:2489: // behavior as ScrollBy to determine
> which layer to animate, but we do not
> We might want to update the comment here - we don't use the same bubbling
> behavior as ScrollBy anymore.

And by test failures, I mean compile failures...

[aelias_OOO_until_Jul13, 2015-10-20 17:18:27]

lgtm since this is release blocker, but to me this indicates we probably should
be reserving scroll offset animations to developer javascript use cases, and use
a path more like the fling curves for wheel smooth scrolls instead.

[skobes, 2015-10-20 18:28:16]

https://codereview.chromium.org/1418493003/diff/60001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/1418493003/diff/60001/cc/input/input_handler....
cc/input/input_handler.h:155: // root layer, or on some other sublayer.
On 2015/10/20 12:29:40, tdresser wrote:
> Should we update this comment?

Done.

https://codereview.chromium.org/1418493003/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418493003/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2489: // behavior as ScrollBy to determine
which layer to animate, but we do not
On 2015/10/20 12:29:40, tdresser wrote:
> We might want to update the comment here - we don't use the same bubbling
> behavior as ScrollBy anymore.

Done.

[skobes, 2015-10-20 18:34:44]

On 2015/10/20 17:18:27, aelias wrote:
> lgtm since this is release blocker, but to me this indicates we probably
should
> be reserving scroll offset animations to developer javascript use cases, and
use
> a path more like the fling curves for wheel smooth scrolls instead.

Yeah this is something to think about.  I think we will still want to leverage
LayerAnimationController, but we may need to distribute the animation between
multiple layers to support inner/outer viewport and scroll customization.  I
don't think this blocks desktop launch though.

[skobes, 2015-10-20 18:34:52]

The CQ bit was checked by skobes@chromium.org

[skobes, 2015-10-20 18:34:52]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
aelias@chromium.org, ymalik@chromium.org
Link to the patchset: https://codereview.chromium.org/1418493003/#ps100001
(title: "fix compile error for realz")

[commit-bot: I haz the power, 2015-10-20 18:36:11]

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

[commit-bot: I haz the power, 2015-10-20 19:16:46]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-10-20 19:17:48]

Patchset 4 (id:??) landed as
https://crrev.com/de5abdb89c357265d78fc873978f1867f46f360a
Cr-Commit-Position: refs/heads/master@{#355125}