Restore handling of new scroll when waiting to cancel current scroll on cc

Restore handling of new scroll when waiting to cancel current scroll on cc

https://crrev.com/1678713002 caused this regression. This CL restores the
old handling of a new user scroll when in state WaitingToCancelOnCompositor.

The current behavior is pretty broken when we are in WaitingToCancelAnimation
and a new user scroll arrives. willAnimateToOffset updates the target offset and
goes into WaitingToSendToCompositor. Consequently, ::updateCompositorAnimation
doesn't abort the previous animation and starts a new one with the updated
target and the two animations compete for scroll offsets.

With this CL, when we are in WaitingToCancelAnimation and a new user scroll
arrives, we do nothing and let ::updateCompositorAnimation cancel the running
animation. As subsequent user scrolls arrive, we will now just start a new
animation with the updated target (and the bug is that we will ignore the first
user scroll when in WaitingToCancelAnimation).

The remaining bug is less serious because in general this is a special case when
we are cancelling in the middle of a user scroll and then starting another
scroll right away. This happens in cases where a site listens for the scroll and
updates the scroll offset (programmatic scroll cancels user scroll), and the
user continues to scroll. FWIW, this is the current behavior in M49, and no real
issues have been reported around it. crbug.com/599876 tracks the correct
handling for this case.

BUG=598548
Committed: https://crrev.com/d115573365c24d26fc93fe9b9d0976d93725e91d
Cr-Commit-Position: refs/heads/master@{#384714}

[ymalik, 2016-04-01 16:20:49]

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

[ajuma, 2016-04-01 17:38:06]

Can you say a bit more about how the behavior change this introduces? That is,
what the current behavior is, why that's a problem, how this improves things,
and why the remaining bug is less serious.

[ymalik, 2016-04-01 18:05:39]

On 2016/04/01 17:38:06, ajuma wrote:
> Can you say a bit more about how the behavior change this introduces? That is,
> what the current behavior is, why that's a problem, how this improves things,
> and why the remaining bug is less serious.

The current behavior is pretty broken when we are in WaitingToCancelAnimation
and a new user scroll arrives. willAnimateToOffset updates the target offset and
goes into WaitingToSendToCompositor. Consequently, ::updateCompositorAnimation
doesn't abort the previous animation and starts a new one with the updated
target and the two animations compete for scroll offsets.

With this CL, when we are in WaitingToCancelAnimation and a new user scroll
arrives, we do nothing and let ::updateCompositorAnimation cancel the running
animation. As subsequent user scrolls arrive, we will now just start a new
animation with the updated target (and the bug is that we will ignore the first
user scroll when in WaitingToCancelAnimation).

The remaining bug is less serious because in general this is a special case when
we are cancelling in the middle of a user scroll and then starting another
scroll right away. This happens in cases where a site listens for the scroll and
updates the scroll offset (programmatic scroll cancels user scroll), and the
user continues to scroll. FWIW, this is the current behavior in M49, and no real
issues have been reported around it.

[ajuma, 2016-04-01 18:10:33]

On 2016/04/01 18:05:39, ymalik1 wrote:
> On 2016/04/01 17:38:06, ajuma wrote:
> > Can you say a bit more about how the behavior change this introduces? That
is,
> > what the current behavior is, why that's a problem, how this improves
things,
> > and why the remaining bug is less serious.
> 
> The current behavior is pretty broken when we are in WaitingToCancelAnimation
> and a new user scroll arrives. willAnimateToOffset updates the target offset
and
> goes into WaitingToSendToCompositor. Consequently, ::updateCompositorAnimation
> doesn't abort the previous animation and starts a new one with the updated
> target and the two animations compete for scroll offsets.
> 
> With this CL, when we are in WaitingToCancelAnimation and a new user scroll
> arrives, we do nothing and let ::updateCompositorAnimation cancel the running
> animation. As subsequent user scrolls arrive, we will now just start a new
> animation with the updated target (and the bug is that we will ignore the
first
> user scroll when in WaitingToCancelAnimation).
> 
> The remaining bug is less serious because in general this is a special case
when
> we are cancelling in the middle of a user scroll and then starting another
> scroll right away. This happens in cases where a site listens for the scroll
and
> updates the scroll offset (programmatic scroll cancels user scroll), and the
> user continues to scroll. FWIW, this is the current behavior in M49, and no
real
> issues have been reported around it.

lgtm, thanks for the explanation! Please expand the description by adding some
(or even all?) of this.

[ymalik, 2016-04-01 18:13:11]

Description was changed from

==========
Restore handling of new scroll when waiting to cancel current scroll on cc

https://crrev.com/1678713002 caused this regression. This CL restores the
old handling of a new user scroll when in state WaitingToCancelOnCompositor.

This is still not the correct way of handling this case but doing this for
now to keep the merge into M50 low-risk. crbug.com/599876 tracks the correct
handling for this case.

BUG=598548
==========

to

==========
Restore handling of new scroll when waiting to cancel current scroll on cc

https://crrev.com/1678713002 caused this regression. This CL restores the
old handling of a new user scroll when in state WaitingToCancelOnCompositor.

The current behavior is pretty broken when we are in WaitingToCancelAnimation
and a new user scroll arrives. willAnimateToOffset updates the target offset and
goes into WaitingToSendToCompositor. Consequently, ::updateCompositorAnimation
doesn't abort the previous animation and starts a new one with the updated
target and the two animations compete for scroll offsets.

With this CL, when we are in WaitingToCancelAnimation and a new user scroll
arrives, we do nothing and let ::updateCompositorAnimation cancel the running
animation. As subsequent user scrolls arrive, we will now just start a new
animation with the updated target (and the bug is that we will ignore the first
user scroll when in WaitingToCancelAnimation).

The remaining bug is less serious because in general this is a special case when
we are cancelling in the middle of a user scroll and then starting another
scroll right away. This happens in cases where a site listens for the scroll and
updates the scroll offset (programmatic scroll cancels user scroll), and the
user continues to scroll. FWIW, this is the current behavior in M49, and no real
issues have been reported around it. crbug.com/599876 tracks the correct
handling for this case.

BUG=598548
==========

[ymalik, 2016-04-01 18:13:11]

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

[ymalik, 2016-04-01 18:13:26]

+Jeremy for owners

[ymalik, 2016-04-01 18:14:09]

On 2016/04/01 18:10:33, ajuma wrote:
> On 2016/04/01 18:05:39, ymalik1 wrote:
> > On 2016/04/01 17:38:06, ajuma wrote:
> > > Can you say a bit more about how the behavior change this introduces? That
> is,
> > > what the current behavior is, why that's a problem, how this improves
> things,
> > > and why the remaining bug is less serious.
> > 
> > The current behavior is pretty broken when we are in
WaitingToCancelAnimation
> > and a new user scroll arrives. willAnimateToOffset updates the target offset
> and
> > goes into WaitingToSendToCompositor. Consequently,
::updateCompositorAnimation
> > doesn't abort the previous animation and starts a new one with the updated
> > target and the two animations compete for scroll offsets.
> > 
> > With this CL, when we are in WaitingToCancelAnimation and a new user scroll
> > arrives, we do nothing and let ::updateCompositorAnimation cancel the
running
> > animation. As subsequent user scrolls arrive, we will now just start a new
> > animation with the updated target (and the bug is that we will ignore the
> first
> > user scroll when in WaitingToCancelAnimation).
> > 
> > The remaining bug is less serious because in general this is a special case
> when
> > we are cancelling in the middle of a user scroll and then starting another
> > scroll right away. This happens in cases where a site listens for the scroll
> and
> > updates the scroll offset (programmatic scroll cancels user scroll), and the
> > user continues to scroll. FWIW, this is the current behavior in M49, and no
> real
> > issues have been reported around it.
> 
> lgtm, thanks for the explanation! Please expand the description by adding some
> (or even all?) of this.

Done! :)

[jbroman, 2016-04-01 19:09:38]

https://codereview.chromium.org/1852753002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right):

https://codereview.chromium.org/1852753002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:465:
.WillRepeatedly(Return(true));
These expectations are not actually verified, because the MockScrollableArea
outlives the test.

e.g. writing
EXPECT_CALL(*scrollableArea, registerForAnimation()).Times(9000);

still passes. Suggest making sure that expectations are verified, e.g. by
forcing a collection in the test.

[ymalik, 2016-04-01 19:53:59]

On 2016/04/01 19:09:38, jbroman wrote:
>
https://codereview.chromium.org/1852753002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right):
> 
>
https://codereview.chromium.org/1852753002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:465:
> .WillRepeatedly(Return(true));
> These expectations are not actually verified, because the MockScrollableArea
> outlives the test.
> 
> e.g. writing
> EXPECT_CALL(*scrollableArea, registerForAnimation()).Times(9000);
> 
> still passes. Suggest making sure that expectations are verified, e.g. by
> forcing a collection in the test.


@Jeremy, PTAL :)

[ymalik, 2016-04-01 19:54:45]

https://codereview.chromium.org/1852753002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right):

https://codereview.chromium.org/1852753002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:465:
.WillRepeatedly(Return(true));
On 2016/04/01 19:09:38, jbroman wrote:
> These expectations are not actually verified, because the MockScrollableArea
> outlives the test.
> 
> e.g. writing
> EXPECT_CALL(*scrollableArea, registerForAnimation()).Times(9000);
> 
> still passes. Suggest making sure that expectations are verified, e.g. by
> forcing a collection in the test.

Done. Thanks!

[jbroman, 2016-04-01 19:58:50]

rs lgtm

[ymalik, 2016-04-01 20:01:58]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-04-01 20:01:59]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1852753002/#ps20001
(title: "rebase master   review feedback")

[commit-bot: I haz the power, 2016-04-01 20:02:31]

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

[commit-bot: I haz the power, 2016-04-01 22:52:00]

Description was changed from

==========
Restore handling of new scroll when waiting to cancel current scroll on cc

https://crrev.com/1678713002 caused this regression. This CL restores the
old handling of a new user scroll when in state WaitingToCancelOnCompositor.

The current behavior is pretty broken when we are in WaitingToCancelAnimation
and a new user scroll arrives. willAnimateToOffset updates the target offset and
goes into WaitingToSendToCompositor. Consequently, ::updateCompositorAnimation
doesn't abort the previous animation and starts a new one with the updated
target and the two animations compete for scroll offsets.

With this CL, when we are in WaitingToCancelAnimation and a new user scroll
arrives, we do nothing and let ::updateCompositorAnimation cancel the running
animation. As subsequent user scrolls arrive, we will now just start a new
animation with the updated target (and the bug is that we will ignore the first
user scroll when in WaitingToCancelAnimation).

The remaining bug is less serious because in general this is a special case when
we are cancelling in the middle of a user scroll and then starting another
scroll right away. This happens in cases where a site listens for the scroll and
updates the scroll offset (programmatic scroll cancels user scroll), and the
user continues to scroll. FWIW, this is the current behavior in M49, and no real
issues have been reported around it. crbug.com/599876 tracks the correct
handling for this case.

BUG=598548
==========

to

==========
Restore handling of new scroll when waiting to cancel current scroll on cc

https://crrev.com/1678713002 caused this regression. This CL restores the
old handling of a new user scroll when in state WaitingToCancelOnCompositor.

The current behavior is pretty broken when we are in WaitingToCancelAnimation
and a new user scroll arrives. willAnimateToOffset updates the target offset and
goes into WaitingToSendToCompositor. Consequently, ::updateCompositorAnimation
doesn't abort the previous animation and starts a new one with the updated
target and the two animations compete for scroll offsets.

With this CL, when we are in WaitingToCancelAnimation and a new user scroll
arrives, we do nothing and let ::updateCompositorAnimation cancel the running
animation. As subsequent user scrolls arrive, we will now just start a new
animation with the updated target (and the bug is that we will ignore the first
user scroll when in WaitingToCancelAnimation).

The remaining bug is less serious because in general this is a special case when
we are cancelling in the middle of a user scroll and then starting another
scroll right away. This happens in cases where a site listens for the scroll and
updates the scroll offset (programmatic scroll cancels user scroll), and the
user continues to scroll. FWIW, this is the current behavior in M49, and no real
issues have been reported around it. crbug.com/599876 tracks the correct
handling for this case.

BUG=598548
==========

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

Committed patchset #2 (id:20001)

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

Description was changed from

==========
Restore handling of new scroll when waiting to cancel current scroll on cc

https://crrev.com/1678713002 caused this regression. This CL restores the
old handling of a new user scroll when in state WaitingToCancelOnCompositor.

The current behavior is pretty broken when we are in WaitingToCancelAnimation
and a new user scroll arrives. willAnimateToOffset updates the target offset and
goes into WaitingToSendToCompositor. Consequently, ::updateCompositorAnimation
doesn't abort the previous animation and starts a new one with the updated
target and the two animations compete for scroll offsets.

With this CL, when we are in WaitingToCancelAnimation and a new user scroll
arrives, we do nothing and let ::updateCompositorAnimation cancel the running
animation. As subsequent user scrolls arrive, we will now just start a new
animation with the updated target (and the bug is that we will ignore the first
user scroll when in WaitingToCancelAnimation).

The remaining bug is less serious because in general this is a special case when
we are cancelling in the middle of a user scroll and then starting another
scroll right away. This happens in cases where a site listens for the scroll and
updates the scroll offset (programmatic scroll cancels user scroll), and the
user continues to scroll. FWIW, this is the current behavior in M49, and no real
issues have been reported around it. crbug.com/599876 tracks the correct
handling for this case.

BUG=598548
==========

to

==========
Restore handling of new scroll when waiting to cancel current scroll on cc

https://crrev.com/1678713002 caused this regression. This CL restores the
old handling of a new user scroll when in state WaitingToCancelOnCompositor.

The current behavior is pretty broken when we are in WaitingToCancelAnimation
and a new user scroll arrives. willAnimateToOffset updates the target offset and
goes into WaitingToSendToCompositor. Consequently, ::updateCompositorAnimation
doesn't abort the previous animation and starts a new one with the updated
target and the two animations compete for scroll offsets.

With this CL, when we are in WaitingToCancelAnimation and a new user scroll
arrives, we do nothing and let ::updateCompositorAnimation cancel the running
animation. As subsequent user scrolls arrive, we will now just start a new
animation with the updated target (and the bug is that we will ignore the first
user scroll when in WaitingToCancelAnimation).

The remaining bug is less serious because in general this is a special case when
we are cancelling in the middle of a user scroll and then starting another
scroll right away. This happens in cases where a site listens for the scroll and
updates the scroll offset (programmatic scroll cancels user scroll), and the
user continues to scroll. FWIW, this is the current behavior in M49, and no real
issues have been reported around it. crbug.com/599876 tracks the correct
handling for this case.

BUG=598548

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

[commit-bot: I haz the power, 2016-04-01 22:53:08]

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