Connect LTHI::ScrollAnimated to cc::Viewport

Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll delta.
Our animation system does not support running two scroll offset animations and this
is something that we may need to add in future.

BUG=575019, 595421
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/99740e85f5ebeb08b80a13b34144f7f7f2d1687e
Cr-Commit-Position: refs/heads/master@{#385657}

[ymalik, 2016-04-06 00:02:26]

Description was changed from

==========
Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll
delta.
Our animation system does not support running two scroll offset animations and
this
is something that we may need to add in future.

BUG=575019,595421
==========

to

==========
Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll
delta.
Our animation system does not support running two scroll offset animations and
this
is something that we may need to add in future.

BUG=575019,595421
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-04-06 00:05:06]

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

[ymalik, 2016-04-06 00:05:34]

Description was changed from

==========
Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll
delta.
Our animation system does not support running two scroll offset animations and
this
is something that we may need to add in future.

BUG=575019,595421
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll
delta.
Our animation system does not support running two scroll offset animations and
this
is something that we may need to add in future.

BUG=575019,595421
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[bokan, 2016-04-06 13:35:53]

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc
File cc/layers/viewport.cc (right):

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:77: return std::abs(max_dim_viewport_delta) >
std::abs(max_dim_pending_delta);
If you want to animate the viewport which will scroll the most, shouldn't you
compare the sums of abs(x) and abs(y)? If you have viewport_delta(99, 99) and
pending_delta(0, 100), you'll return false even though the viewport will consume
198 but the pending is only 100.

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:97: ShouldAnimateViewport(inner_scroll.delta,
pending_delta);
Shouldn't this compare inner_scroll.delta to outer_scroll.delta? The outer
viewport might not be able to accept all or any of that pending_delta.

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:110: scroll_tree.ScrollBy(outer_node, outer_scroll.delta,
Won't this cause a visible "jump"? I would guess that scrolling only one at a
time but animated would feel better...

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.h
File cc/layers/viewport.h (right):

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.h#newcode60
cc/layers/viewport.h:60: // Returns true if viewport_delta is stricly less than
pendinf_delta.
Nit: s/pendinf_delta/pending_delta

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport_unittest.cc
File cc/layers/viewport_unittest.cc (right):

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport_unittest...
cc/layers/viewport_unittest.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
Nit: "because it's 2016"

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2744: const float kEpsilon = 0.1f;
Can you make sure this epsilon doesn't already exist somewhere else in the
scrolling code (i.e. in the scroll/property trees or in LayerImpl). If not,
please move it to be global and name it more appropriately (e.g.
kMinimumScrollEpsilon)

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:539: struct ScrollAmount {
I don't like this, there's opportunity for delta and target_offset to point to
different locations and it makes it harder to think about how things will work
given a ScrollAmount. Why not just use a delta and convert to an offset using
clamp(current_offset + delta)?

[ymalik, 2016-04-06 15:33:45]

@bokan PTAL :)

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc
File cc/layers/viewport.cc (right):

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:77: return std::abs(max_dim_viewport_delta) >
std::abs(max_dim_pending_delta);
On 2016/04/06 13:35:52, bokan wrote:
> If you want to animate the viewport which will scroll the most, shouldn't you
> compare the sums of abs(x) and abs(y)? If you have viewport_delta(99, 99) and
> pending_delta(0, 100), you'll return false even though the viewport will
consume
> 198 but the pending is only 100.

I think the animation duration is determined by the maximum dimension.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/scrol...

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:97: ShouldAnimateViewport(inner_scroll.delta,
pending_delta);
On 2016/04/06 13:35:52, bokan wrote:
> Shouldn't this compare inner_scroll.delta to outer_scroll.delta? The outer
> viewport might not be able to accept all or any of that pending_delta.

Yes, you're right! Done.

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:110: scroll_tree.ScrollBy(outer_node, outer_scroll.delta,
On 2016/04/06 13:35:52, bokan wrote:
> Won't this cause a visible "jump"? I would guess that scrolling only one at a
> time but animated would feel better...

The jump doesn't look terribly bad IMO. It also only happens when we reach the
edge of the visual viewport. It's still better than the current behavior where
we only scroll the outer viewport. We will need to have some sort of state logic
to start the second animation once the first one is finished, which may be a lot
of work.

Added comments to address this.

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.h
File cc/layers/viewport.h (right):

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.h#newcode60
cc/layers/viewport.h:60: // Returns true if viewport_delta is stricly less than
pendinf_delta.
On 2016/04/06 13:35:52, bokan wrote:
> Nit: s/pendinf_delta/pending_delta

Done.

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport_unittest.cc
File cc/layers/viewport_unittest.cc (right):

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport_unittest...
cc/layers/viewport_unittest.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2016/04/06 13:35:52, bokan wrote:
> Nit: "because it's 2016"

Done.

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2744: const float kEpsilon = 0.1f;
On 2016/04/06 13:35:52, bokan wrote:
> Can you make sure this epsilon doesn't already exist somewhere else in the
> scrolling code (i.e. in the scroll/property trees or in LayerImpl). If not,
> please move it to be global and name it more appropriately (e.g.
> kMinimumScrollEpsilon)

I don't think this will be used anywhere else so probably doesn't need to be
global.

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:539: struct ScrollAmount {
On 2016/04/06 13:35:52, bokan wrote:
> I don't like this, there's opportunity for delta and target_offset to point to
> different locations and it makes it harder to think about how things will work
> given a ScrollAmount. Why not just use a delta and convert to an offset using
> clamp(current_offset + delta)?

You're right, it's not used often enough to justify. Done!

[bokan, 2016-04-06 19:38:23]

non-owner lgtm % minor comments.

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc
File cc/layers/viewport.cc (right):

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:77: return std::abs(max_dim_viewport_delta) >
std::abs(max_dim_pending_delta);
On 2016/04/06 15:33:45, ymalik1 wrote:
> On 2016/04/06 13:35:52, bokan wrote:
> > If you want to animate the viewport which will scroll the most, shouldn't
you
> > compare the sums of abs(x) and abs(y)? If you have viewport_delta(99, 99)
and
> > pending_delta(0, 100), you'll return false even though the viewport will
> consume
> > 198 but the pending is only 100.
> 
> I think the animation duration is determined by the maximum dimension.
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/scrol...

Ok, this is fine then. IMO, std::max(viewport_delta.x(), viewport_delta.y())
would be clearer.

https://codereview.chromium.org/1866583002/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:110: scroll_tree.ScrollBy(outer_node, outer_scroll.delta,
On 2016/04/06 15:33:45, ymalik1 wrote:
> On 2016/04/06 13:35:52, bokan wrote:
> > Won't this cause a visible "jump"? I would guess that scrolling only one at
a
> > time but animated would feel better...
> 
> The jump doesn't look terribly bad IMO. It also only happens when we reach the
> edge of the visual viewport. It's still better than the current behavior where
> we only scroll the outer viewport. We will need to have some sort of state
logic
> to start the second animation once the first one is finished, which may be a
lot
> of work.
> 
> Added comments to address this. 

Acknowledged.

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1866583002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2744: const float kEpsilon = 0.1f;
On 2016/04/06 15:33:45, ymalik1 wrote:
> On 2016/04/06 13:35:52, bokan wrote:
> > Can you make sure this epsilon doesn't already exist somewhere else in the
> > scrolling code (i.e. in the scroll/property trees or in LayerImpl). If not,
> > please move it to be global and name it more appropriately (e.g.
> > kMinimumScrollEpsilon)
> 
> I don't think this will be used anywhere else so probably doesn't need to be
> global.

Ok, don't make it static though.

[ajuma, 2016-04-06 19:53:49]

lgtm % bokan

[ymalik, 2016-04-06 21:56:22]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-04-06 21:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1866583002/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2016-04-06 21:57:33]

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

[commit-bot: I haz the power, 2016-04-07 01:45:06]

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

[commit-bot: I haz the power, 2016-04-07 01:45:07]

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

[ymalik, 2016-04-07 01:55:21]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-04-07 01:55:51]

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

[commit-bot: I haz the power, 2016-04-07 04:18:21]

Description was changed from

==========
Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll
delta.
Our animation system does not support running two scroll offset animations and
this
is something that we may need to add in future.

BUG=575019,595421
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll
delta.
Our animation system does not support running two scroll offset animations and
this
is something that we may need to add in future.

BUG=575019,595421
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-07 04:18:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-07 04:20:08]

Description was changed from

==========
Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll
delta.
Our animation system does not support running two scroll offset animations and
this
is something that we may need to add in future.

BUG=575019,595421
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Connect LTHI::ScrollAnimated to cc::Viewport

This CL removed to hack in FindScrollLayerForDeviceViewportPoint to ensure
that we scroll the outer viewport and connects the LTHI::ScrollAnimated function
to the viewport class.

The viewport class distributes the scroll delta between the inner and outer
viewports and starts animation the layer which consumes more of the scroll
delta.
Our animation system does not support running two scroll offset animations and
this
is something that we may need to add in future.

BUG=575019,595421
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/99740e85f5ebeb08b80a13b34144f7f7f2d1687e
Cr-Commit-Position: refs/heads/master@{#385657}
==========

[commit-bot: I haz the power, 2016-04-07 04:20:12]

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