Correctly update scroll offset animations in response to scroll anchoring

Correctly update scroll offset animations in response to scroll anchoring

This CL fixes the incorrect implementation in https://crrev.com/1926473003.
In addition to the state WaitingToCancelOnCompositorButNewScroll introduced
previously, it adds a new state RunningOnCompositorButNeedsAdjustment when
an ongoing animation needs to be adjusted. We need the former to fix
issue 599876.

When an adjustment needs to be made, ScrollAnchor calls into ScrollAnimator
which adjusts the curve. When the animation is running on MT, this is enough,
but if the animation is running on CC, we abort the old animation and
push a new one with the adjusted initial and target positions (hence the new
state).

This CL also cleans up ::updateCompositorAnimations.

Added a new test case for testing MT animation and an animation running on CC
is tested by the layout test added in the initial patch.
(fast/scroll-behavior/smooth-scroll/ongoing-smooth-scroll-anchors.html)

BUG=594456
Committed: https://crrev.com/07ed51b3771572c4bff43a990351f4be1de4f4ae
Cr-Commit-Position: refs/heads/master@{#396913}

[ymalik, 2016-05-26 23:06:09]

Description was changed from

==========
Correctly update scroll offset animations in response to scroll anchoring

This CL fixes the incorrect implementation in https://crrev.com/1926473003.
In addition to the state WaitingToCancelOnCompositorButNewScroll introduced
previously, it adds a new state RunningOnCompositorButNeedsAdjustment when
an ongoing animation needs to be adjusted. We need the former to fix
issue 599876.

When an adjustment needs to be made, ScrollAnchor calls into ScrollAnimator
which adjusts the curve. When the animation is running on MT, this is enough,
but if the animation is running on CC, we abort the old animation and
push a new one with the adjusted initial and target positions (hence the new
state).

This CL also cleans up ::updateCompositorAnimations.

Added a new test case for testing MT animation and an animation running on CC
is tested by the layout test added in the initial patch.
(fast/scroll-behavior/smooth-scroll/ongoing-smooth-scroll-anchors.html)

BUG=594456
==========

to

==========
Correctly update scroll offset animations in response to scroll anchoring

This CL fixes the incorrect implementation in https://crrev.com/1926473003.
In addition to the state WaitingToCancelOnCompositorButNewScroll introduced
previously, it adds a new state RunningOnCompositorButNeedsAdjustment when
an ongoing animation needs to be adjusted. We need the former to fix
issue 599876.

When an adjustment needs to be made, ScrollAnchor calls into ScrollAnimator
which adjusts the curve. When the animation is running on MT, this is enough,
but if the animation is running on CC, we abort the old animation and
push a new one with the adjusted initial and target positions (hence the new
state).

This CL also cleans up ::updateCompositorAnimations.

Added a new test case for testing MT animation and an animation running on CC
is tested by the layout test added in the initial patch.
(fast/scroll-behavior/smooth-scroll/ongoing-smooth-scroll-anchors.html)

BUG=594456
==========

[ymalik, 2016-05-26 23:06:09]

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

[ymalik, 2016-05-26 23:07:10]

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

[commit-bot: I haz the power, 2016-05-26 23:08:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2015113003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2015113003/1

[commit-bot: I haz the power, 2016-05-27 01:39:07]

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

[commit-bot: I haz the power, 2016-05-27 01:39:08]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-27 01:39:21]

Dry run: None

[ajuma, 2016-05-27 14:21:35]

lgtm

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

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:327: // adjustment
ot the curve.
s/ot/to

[skobes, 2016-05-27 17:58:30]

Thanks for doing this!

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:245: ScrollAnimatorBase*
animator = m_scroller->existingScrollAnimator();
Could these five lines be reduced to

  m_scroller->scrollAnimator().adjustScrollPosition(adjustment,
AnchoringScroll);

? I can't think of a good reason to avoid creating the scroll animator here, and
ScrollAnimator can simply call setScrollPosition if there is no animation.

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

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:200: IntSize
actualAdjustment = compositorSizeFromBlinkSize(
This doesn't make sense to me.  An adjustment is a delta, why would the scroll
origin affect it?

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:72:
IntSize compositorSizeFromBlinkSize(IntSize);
A size is a size, it's only scroll offset that has two different concepts.  Add
an overload of compositorOffsetFromBlinkOffset if necessary (but per my other
comment I don't even think you need that).

[ymalik, 2016-05-27 20:03:11]

@skobed PTAL :)

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:245: ScrollAnimatorBase*
animator = m_scroller->existingScrollAnimator();
On 2016/05/27 17:58:30, skobes wrote:
> Could these five lines be reduced to
> 
>   m_scroller->scrollAnimator().adjustScrollPosition(adjustment,
> AnchoringScroll);
> 
> ? I can't think of a good reason to avoid creating the scroll animator here,
and
> ScrollAnimator can simply call setScrollPosition if there is no animation.

Done.

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

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:200: IntSize
actualAdjustment = compositorSizeFromBlinkSize(
On 2016/05/27 17:58:30, skobes wrote:
> This doesn't make sense to me.  An adjustment is a delta, why would the scroll
> origin affect it?

You're right. I was confusing it with the target_offset logic. Added a test for
rtl as well.

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:327: // adjustment
ot the curve.
On 2016/05/27 14:21:34, ajuma wrote:
> s/ot/to

Done.

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/2015113003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:72:
IntSize compositorSizeFromBlinkSize(IntSize);
On 2016/05/27 17:58:30, skobes wrote:
> A size is a size, it's only scroll offset that has two different concepts. 
Add
> an overload of compositorOffsetFromBlinkOffset if necessary (but per my other
> comment I don't even think you need that).

Removed this.

[skobes, 2016-05-27 20:05:45]

lgtm

[ymalik, 2016-05-27 20:10:46]

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

[ymalik, 2016-05-27 20:10:46]

+jbroman for Source/platform

[ymalik, 2016-05-31 15:20:14]

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

[ymalik, 2016-05-31 15:20:15]

+rbyers for Source/platform

[Rick Byers, 2016-05-31 16:12:23]

RS LGTM - ajuma and skobes are experts here.

[ymalik, 2016-05-31 18:37:42]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-05-31 18:37:42]

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/2015113003/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2016-05-31 18:38:16]

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

[commit-bot: I haz the power, 2016-05-31 20:25:11]

Description was changed from

==========
Correctly update scroll offset animations in response to scroll anchoring

This CL fixes the incorrect implementation in https://crrev.com/1926473003.
In addition to the state WaitingToCancelOnCompositorButNewScroll introduced
previously, it adds a new state RunningOnCompositorButNeedsAdjustment when
an ongoing animation needs to be adjusted. We need the former to fix
issue 599876.

When an adjustment needs to be made, ScrollAnchor calls into ScrollAnimator
which adjusts the curve. When the animation is running on MT, this is enough,
but if the animation is running on CC, we abort the old animation and
push a new one with the adjusted initial and target positions (hence the new
state).

This CL also cleans up ::updateCompositorAnimations.

Added a new test case for testing MT animation and an animation running on CC
is tested by the layout test added in the initial patch.
(fast/scroll-behavior/smooth-scroll/ongoing-smooth-scroll-anchors.html)

BUG=594456
==========

to

==========
Correctly update scroll offset animations in response to scroll anchoring

This CL fixes the incorrect implementation in https://crrev.com/1926473003.
In addition to the state WaitingToCancelOnCompositorButNewScroll introduced
previously, it adds a new state RunningOnCompositorButNeedsAdjustment when
an ongoing animation needs to be adjusted. We need the former to fix
issue 599876.

When an adjustment needs to be made, ScrollAnchor calls into ScrollAnimator
which adjusts the curve. When the animation is running on MT, this is enough,
but if the animation is running on CC, we abort the old animation and
push a new one with the adjusted initial and target positions (hence the new
state).

This CL also cleans up ::updateCompositorAnimations.

Added a new test case for testing MT animation and an animation running on CC
is tested by the layout test added in the initial patch.
(fast/scroll-behavior/smooth-scroll/ongoing-smooth-scroll-anchors.html)

BUG=594456
==========

[commit-bot: I haz the power, 2016-05-31 20:25:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-31 20:45:12]

Description was changed from

==========
Correctly update scroll offset animations in response to scroll anchoring

This CL fixes the incorrect implementation in https://crrev.com/1926473003.
In addition to the state WaitingToCancelOnCompositorButNewScroll introduced
previously, it adds a new state RunningOnCompositorButNeedsAdjustment when
an ongoing animation needs to be adjusted. We need the former to fix
issue 599876.

When an adjustment needs to be made, ScrollAnchor calls into ScrollAnimator
which adjusts the curve. When the animation is running on MT, this is enough,
but if the animation is running on CC, we abort the old animation and
push a new one with the adjusted initial and target positions (hence the new
state).

This CL also cleans up ::updateCompositorAnimations.

Added a new test case for testing MT animation and an animation running on CC
is tested by the layout test added in the initial patch.
(fast/scroll-behavior/smooth-scroll/ongoing-smooth-scroll-anchors.html)

BUG=594456
==========

to

==========
Correctly update scroll offset animations in response to scroll anchoring

This CL fixes the incorrect implementation in https://crrev.com/1926473003.
In addition to the state WaitingToCancelOnCompositorButNewScroll introduced
previously, it adds a new state RunningOnCompositorButNeedsAdjustment when
an ongoing animation needs to be adjusted. We need the former to fix
issue 599876.

When an adjustment needs to be made, ScrollAnchor calls into ScrollAnimator
which adjusts the curve. When the animation is running on MT, this is enough,
but if the animation is running on CC, we abort the old animation and
push a new one with the adjusted initial and target positions (hence the new
state).

This CL also cleans up ::updateCompositorAnimations.

Added a new test case for testing MT animation and an animation running on CC
is tested by the layout test added in the initial patch.
(fast/scroll-behavior/smooth-scroll/ongoing-smooth-scroll-anchors.html)

BUG=594456

Committed: https://crrev.com/07ed51b3771572c4bff43a990351f4be1de4f4ae
Cr-Commit-Position: refs/heads/master@{#396913}
==========

[commit-bot: I haz the power, 2016-05-31 20:45:14]

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