Run smooth scroll animations on the compositor when possible

Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1z76m-GMDgLI-clkfroLJ-5yhsHkmriZr_swP0kn5iw4

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in RAF)

BUG=552556
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5534a2b2972534b523014c803e1e0e0541855cc7
Cr-Commit-Position: refs/heads/master@{#366685}

[ymalik, 2015-12-18 16:00:38]

Description was changed from

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1tPSoq-qItcmyV-MvMiC9qZPiiaO1zmzkKgc3sDZ8C...

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

BUG=552556
==========

to

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1tPSoq-qItcmyV-MvMiC9qZPiiaO1zmzkKgc3sDZ8C...

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

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

[ymalik, 2015-12-18 16:06:43]

Description was changed from

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1tPSoq-qItcmyV-MvMiC9qZPiiaO1zmzkKgc3sDZ8C...

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

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

to

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1tPSoq-qItcmyV-MvMiC9qZPiiaO1zmzkKgc3sDZ8C...

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

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

[ymalik, 2015-12-18 16:06:44]

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

[ajuma, 2015-12-18 19:02:54]

This looks great. A couple comments:

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html
(right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html:27:
if (window.scrollX == testCase.expectedX && window.scrollY ==
testCase.expectedY) {
Would this test also pass if all scrolls were instant?

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:83: void
cancelAnimation() { }
Missing 'override'

[skobes, 2015-12-18 19:05:58]

Awesome :)

Just a few nits.

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html
(right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html:22:
test is to ensure that smooth scrolling on the compositor\
This doesn't actually test that the scroll is composited, does it?  I don't
think layout tests can tell the difference, except maybe with repaint
tracking...

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.h (right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.h:46: class
PLATFORM_EXPORT ScrollAnimator final : public ScrollAnimatorBase  {
extra space before {

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.h:75: FloatPoint
m_targetOffset;
Why is this needed here if it's also stored in m_animationCurve?

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.h:76: FloatPoint
m_initialOffset;
This seems unused?

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebLayer.h:163: // Aborts all animations with
the given id.
Explain how aborting is different from removing.

[ymalik, 2015-12-18 19:35:47]

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html
(right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html:22:
test is to ensure that smooth scrolling on the compositor\
On 2015/12/18 19:05:57, skobes wrote:
> This doesn't actually test that the scroll is composited, does it?  I don't
> think layout tests can tell the difference, except maybe with repaint
> tracking...

So what I mean by that comment is that we really care about this test when it's
run by the /threaded virtual suite (with the compositor thread). I updated the
comment to make that more clear. Does that make sense?

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html:27:
if (window.scrollX == testCase.expectedX && window.scrollY ==
testCase.expectedY) {
On 2015/12/18 19:02:54, ajuma wrote:
> Would this test also pass if all scrolls were instant?

Yes. Is that bad?

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.h (right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.h:46: class
PLATFORM_EXPORT ScrollAnimator final : public ScrollAnimatorBase  {
On 2015/12/18 19:05:58, skobes wrote:
> extra space before {

Done.

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.h:75: FloatPoint
m_targetOffset;
On 2015/12/18 19:05:57, skobes wrote:
> Why is this needed here if it's also stored in m_animationCurve?

m_animationCurve is actually set when updateCompositorAnimations is called.
m_targetOffset is the variable that stores the new target offset from when
ScrollAnimator::userScroll is called.

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.h:76: FloatPoint
m_initialOffset;
On 2015/12/18 19:05:58, skobes wrote:
> This seems unused?

Oops. Forgot to remove after testing. Thanks.

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:83: void
cancelAnimation() { }
On 2015/12/18 19:02:54, ajuma wrote:
> Missing 'override'

Good catch. Thanks!

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebLayer.h:163: // Aborts all animations with
the given id.
On 2015/12/18 19:05:58, skobes wrote:
> Explain how aborting is different from removing.

Done.

[skobes, 2015-12-18 19:50:02]

lgtm

Use this link in the CL description (viewable outside google.com):
https://docs.google.com/drawings/d/1z76m-GMDgLI-clkfroLJ-5yhsHkmriZr_swP0kn5iw4

[ajuma, 2015-12-18 19:51:12]

lgtm too

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html
(right):

https://codereview.chromium.org/1534813004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/keyboard-scroll.html:27:
if (window.scrollX == testCase.expectedX && window.scrollY ==
testCase.expectedY) {
On 2015/12/18 19:35:47, ymalik1 wrote:
> On 2015/12/18 19:02:54, ajuma wrote:
> > Would this test also pass if all scrolls were instant?
> 
> Yes. Is that bad?

Depends on what you're trying to test :)  If it's that scrolls eventually reach
their intended destination, it's fine. But if you also want to test that smooth
scrolls are really non-instant, you'd need to check that they don't reach their
destination immediately after the input event.

[ymalik, 2015-12-18 20:11:10]

Description was changed from

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1tPSoq-qItcmyV-MvMiC9qZPiiaO1zmzkKgc3sDZ8C...

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

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

to

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1z76m-GMDgLI-clkfroLJ-5yhsHkmriZr_swP0kn5iw4

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

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

[ymalik, 2015-12-22 20:42:29]

ymalik@chromium.org changed reviewers:
+ rbyers@chromium.org, vollick@chromium.org

[ymalik, 2015-12-22 20:43:35]

+Rick for WebKit/public/platform/
+Ian for WebKit/Source/platform/

[Rick Byers, 2015-12-22 20:56:09]

public/ RS LGTM (ajuma@ is an expert on the blink/cc integration impacted by
these new APIs)

[Ian Vollick, 2015-12-22 21:54:06]

On 2015/12/22 20:56:09, Rick Byers wrote:
> public/ RS LGTM (ajuma@ is an expert on the blink/cc integration impacted by
> these new APIs)

Source/platform/ lgtm.

[ymalik, 2015-12-22 21:58:01]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-12-22 21:58:01]

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

[commit-bot: I haz the power, 2015-12-22 21:58:13]

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

[commit-bot: I haz the power, 2015-12-22 23:17:26]

Description was changed from

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1z76m-GMDgLI-clkfroLJ-5yhsHkmriZr_swP0kn5iw4

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

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

to

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1z76m-GMDgLI-clkfroLJ-5yhsHkmriZr_swP0kn5iw4

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

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

[commit-bot: I haz the power, 2015-12-22 23:17:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-22 23:18:31]

Description was changed from

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1z76m-GMDgLI-clkfroLJ-5yhsHkmriZr_swP0kn5iw4

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

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

to

==========
Run smooth scroll animations on the compositor when possible

This CL implements "immediate retargeting" shown in the following diagram:
https://docs.google.com/drawings/d/1z76m-GMDgLI-clkfroLJ-5yhsHkmriZr_swP0kn5iw4

This CL does the following
- Add a new state to ScrollAnimatorCompositorCoordinator to update the target
offset on the compositor
- Use the ScrollAnimatorCompositorCoordinator to schedule anim
- Add plumbing from the animators to the animation controller to abort
animations

Sample test pages:
yashmalik.com/nested_scroll_mousewheel_listener.html
yashmalik.com/nested_scroll_mousewheel_listener_15ms_janky.html (tight loop in
RAF)

BUG=552556
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5534a2b2972534b523014c803e1e0e0541855cc7
Cr-Commit-Position: refs/heads/master@{#366685}
==========

[commit-bot: I haz the power, 2015-12-22 23:18:32]

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

[Noel Gordon, 2015-12-23 01:26:50]

Per
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg...

virtual/threaded/fast/scroll-behavior/overflow-scroll-root-frame-animates.html
virtual/threaded/fast/scroll-behavior/overflow-scroll-animates.html

started failing.  Was it due to this patch?  (And we can't revert at this time
to find out, chromium tree is closed).

[Noel Gordon, 2015-12-23 09:13:49]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1548883002/ by noel@chromium.org.

The reason for reverting is: Appears to have made Linux debug builds fail the
following tests:

virtual/threaded/fast/scroll-behavior/overflow-scroll-root-frame-animates.html
virtual/threaded/fast/scroll-behavior/overflow-scroll-animates.html

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg...

Reverting to green the Blink tree.


.

[Noel Gordon, 2015-12-23 09:18:03]

Also failing in Olipan Win Debug:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%2...