currentlyscrollinglayer cleared in scrollAnimatedBegin.

currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer after each animation.
The dependency will resolved in follow up patches that will address latching on other devices.

Similar changes is done for scrollAnimated function in:
https://codereview.chromium.org/2256733003

BUG=642370
TEST=LayerTreeHostImplTest.SecondScrollAnimatedBeginNotIgnored

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[sahel, 2016-08-30 21:58:14]

Description was changed from

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer
after each animation. The dependency will resolved in follow up patches
that will address latching on other devices.

BUG=642370
==========

to

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer
after each animation. The dependency will resolved in follow up patches
that will address latching on other devices.

BUG=642370
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sahel, 2016-08-30 22:16:50]

sahel@chromium.org changed reviewers:
+ ericrk@chromium.org, tdresser@chromium.org, ymalik@chromium.org

[sahel, 2016-08-30 22:16:51]

tdresser@chromium.org: Please review changes in 

ericrk@chromium.org: Please review changes in

[tdresser, 2016-08-31 13:10:05]

Just making sure - you've tested this locally?

LGTM to land, but as a followup, we ought to have a test that ensures that we
scroll on the compositor thread in the simple case.

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

https://codereview.chromium.org/2290233004/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2805: // impelemented, the current scrolling
layer should not get cleared after
implemented

[ymalik, 2016-08-31 14:01:55]

lgtm

can you also link to your previous patch that added the clearing logic in scroll
begin?

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

https://codereview.chromium.org/2290233004/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2808: ClearCurrentlyScrollingLayer();
I suspect that this is working correctly now because of your change above
(scroll_state_data.is_in_inertial_phase = false) and not because of your call to
ClearCurrentlyScrollingLayer().

Can you verify that this is the case?

If so, please update the description to better explain this case.

[sahel, 2016-08-31 14:24:19]

Description was changed from

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer
after each animation. The dependency will resolved in follow up patches
that will address latching on other devices.

BUG=642370
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer
after each animation. The dependency will resolved in follow up patches
that will address latching on other devices.

BUG=642370
TEST=LayerTreeHostImplTest.SecondScrollAnimatedBeginNotIgnored

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sahel, 2016-08-31 14:25:10]

I added a unit test that fails with the old code and passes with this patch.

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

https://codereview.chromium.org/2290233004/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2805: // impelemented, the current scrolling
layer should not get cleared after
On 2016/08/31 13:10:05, tdresser wrote:
> implemented

Done.

https://codereview.chromium.org/2290233004/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2808: ClearCurrentlyScrollingLayer();
On 2016/08/31 14:01:55, ymalik wrote:
> I suspect that this is working correctly now because of your change above
> (scroll_state_data.is_in_inertial_phase = false) and not because of your call
to
> ClearCurrentlyScrollingLayer().
> 
> Can you verify that this is the case?
> 
> If so, please update the description to better explain this case. 
>   

It is working because of the call. The problem was that after calling
animatedBegin once and not clearing the scrolling layer, the second time that
animatedBegin was called, it would fall on the part that scroll gets ignored and
return before calling scrollBegin.

Correcting the is_in_initial_phase value is for scrollBegin function. In the
scrollBegin if the is_in_inertial_phase is true fling code is called, and that's
why is_in_initial_phase should be iff true in the case of a fling.

[tdresser, 2016-08-31 14:44:42]

Thanks for the test.

Still LGTM!

[ymalik, 2016-08-31 14:45:52]

On 2016/08/31 14:25:10, sahel wrote:
> I added a unit test that fails with the old code and passes with this patch.
> 
>
https://codereview.chromium.org/2290233004/diff/1/cc/trees/layer_tree_host_im...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2290233004/diff/1/cc/trees/layer_tree_host_im...
> cc/trees/layer_tree_host_impl.cc:2805: // impelemented, the current scrolling
> layer should not get cleared after
> On 2016/08/31 13:10:05, tdresser wrote:
> > implemented
> 
> Done.
> 
>
https://codereview.chromium.org/2290233004/diff/1/cc/trees/layer_tree_host_im...
> cc/trees/layer_tree_host_impl.cc:2808: ClearCurrentlyScrollingLayer();
> On 2016/08/31 14:01:55, ymalik wrote:
> > I suspect that this is working correctly now because of your change above
> > (scroll_state_data.is_in_inertial_phase = false) and not because of your
call
> to
> > ClearCurrentlyScrollingLayer().
> > 
> > Can you verify that this is the case?
> > 
> > If so, please update the description to better explain this case. 
> >   
> 
> It is working because of the call. The problem was that after calling
> animatedBegin once and not clearing the scrolling layer, the second time that
> animatedBegin was called, it would fall on the part that scroll gets ignored
and
> return before calling scrollBegin.
> 
> Correcting the is_in_initial_phase value is for scrollBegin function. In the
> scrollBegin if the is_in_inertial_phase is true fling code is called, and
that's
> why is_in_initial_phase should be iff true in the case of a fling.

Thanks! Still LGTM too

[sahel, 2016-08-31 16:01:22]

Description was changed from

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer
after each animation. The dependency will resolved in follow up patches
that will address latching on other devices.

BUG=642370
TEST=LayerTreeHostImplTest.SecondScrollAnimatedBeginNotIgnored

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer after each
animation.
The dependency will resolved in follow up patches that will address latching on
other devices.

Similar changes is done for scrollAnimated function in:
https://codereview.chromium.org/2256733003


BUG=642370
TEST=LayerTreeHostImplTest.SecondScrollAnimatedBeginNotIgnored

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[ericrk, 2016-08-31 18:29:42]

I think this makes sense. I'd like to better understand the impact of clearing
the currently scrolling layer.

From your comments it sounds like clearing the currently scrolling layer is only
needed for the non-latching cases. Why is this needed now, but not before the
latching changes?

I see the bug you mention (if you don't clear the layer, you end up in the
early-out in ScrollAnimatedBegin), but isn't this what happened before? Why is
this early out wrong now when it wasn't before?

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

https://codereview.chromium.org/2290233004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2791: scroll_state_data.is_in_inertial_phase =
false;
nit: This is already the default value of is_in_inertial_phase, no need to set
it. There are lots of other fields on ScrollStateData which we don't explicitly
set if left at default, so not setting this seems consistent.

Also update the other occurrence in ScrollAnimated

[sahel, 2016-08-31 19:16:35]

On 2016/08/31 18:29:42, ericrk wrote:
> I think this makes sense. I'd like to better understand the impact of clearing
> the currently scrolling layer.
> 
> From your comments it sounds like clearing the currently scrolling layer is
only
> needed for the non-latching cases. Why is this needed now, but not before the
> latching changes?
> 
> I see the bug you mention (if you don't clear the layer, you end up in the
> early-out in ScrollAnimatedBegin), but isn't this what happened before? Why is
> this early out wrong now when it wasn't before?
> 
>
https://codereview.chromium.org/2290233004/diff/20001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2290233004/diff/20001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:2791: scroll_state_data.is_in_inertial_phase
=
> false;
> nit: This is already the default value of is_in_inertial_phase, no need to set
> it. There are lots of other fields on ScrollStateData which we don't
explicitly
> set if left at default, so not setting this seems consistent.
> 
> Also update the other occurrence in ScrollAnimated

Previously, the clearing function it was getting called in all cases within
ScrollEnd function. So, calling scrollEnd in the end of scroll animation
functions was enough to clear the scrolling layer. Now that we don't clear the
currently scrolling layer in ScrollEnd(to be able to latch the fling to its
related scroll), I had to explicitly call the clearing functions after
scrollEnd.

[sahel, 2016-08-31 19:16:47]

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

https://codereview.chromium.org/2290233004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2791: scroll_state_data.is_in_inertial_phase =
false;
On 2016/08/31 18:29:42, ericrk wrote:
> nit: This is already the default value of is_in_inertial_phase, no need to set
> it. There are lots of other fields on ScrollStateData which we don't
explicitly
> set if left at default, so not setting this seems consistent.
> 
> Also update the other occurrence in ScrollAnimated

Done.

[ericrk, 2016-08-31 20:18:13]

lgtm

Ok, that makes sense. Thanks!

[sahel, 2016-08-31 20:30:52]

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

[commit-bot: I haz the power, 2016-08-31 20:31:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 21:27:08]

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

[commit-bot: I haz the power, 2016-08-31 21:27:09]

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

[sahel, 2016-08-31 22:47:42]

The CQ bit was checked by sahel@chromium.org

[sahel, 2016-08-31 22:47:42]

The patchset sent to the CQ was uploaded after l-g-t-m from ymalik@chromium.org,
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2290233004/#ps40001
(title: "nit fixed.")

[commit-bot: I haz the power, 2016-08-31 22:48:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 00:14:17]

Description was changed from

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer after each
animation.
The dependency will resolved in follow up patches that will address latching on
other devices.

Similar changes is done for scrollAnimated function in:
https://codereview.chromium.org/2256733003


BUG=642370
TEST=LayerTreeHostImplTest.SecondScrollAnimatedBeginNotIgnored

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer after each
animation.
The dependency will resolved in follow up patches that will address latching on
other devices.

Similar changes is done for scrollAnimated function in:
https://codereview.chromium.org/2256733003


BUG=642370
TEST=LayerTreeHostImplTest.SecondScrollAnimatedBeginNotIgnored

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-09-01 00:14:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-01 00:16:31]

Description was changed from

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer after each
animation.
The dependency will resolved in follow up patches that will address latching on
other devices.

Similar changes is done for scrollAnimated function in:
https://codereview.chromium.org/2256733003


BUG=642370
TEST=LayerTreeHostImplTest.SecondScrollAnimatedBeginNotIgnored

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
currentlyscrollinglayer cleared in scrollAnimatedBegin.

For now the latching is implemented only for mac, and the
scrollAnimation logic is dependent on clearing the scrolling layer after each
animation.
The dependency will resolved in follow up patches that will address latching on
other devices.

Similar changes is done for scrollAnimated function in:
https://codereview.chromium.org/2256733003

BUG=642370
TEST=LayerTreeHostImplTest.SecondScrollAnimatedBeginNotIgnored

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/ebcfc3309fba892b25d84f943d26bbdab6d100ee
Cr-Commit-Position: refs/heads/master@{#415814}
==========

[commit-bot: I haz the power, 2016-09-01 00:16:32]

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