Animations: Port ProgrammaticScrollAnimator to use compositor timelines

Animations: Port ProgrammaticScrollAnimator to use compositor timelines

Use AnimationPlayer to add animations if new system enabled.

Previous episode: https://codereview.chromium.org/1119763003/
What we want to achieve: https://codereview.chromium.org/1131833002

BUG=394777
R=ajuma@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199766

[loyso (OOO), 2015-05-07 04:18:27]

loyso@chromium.org changed reviewers:
+ vollick@chromium.org

[loyso (OOO), 2015-07-10 02:42:23]

PTAL!

[Ian Vollick, 2015-07-10 03:06:45]

https://codereview.chromium.org/1133513002/diff/20001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/1133513002/diff/20001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.cpp:97: if
(RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() &&
Platform::current()->compositorSupport()) {
AFAICT, most of the time we assume that compositorSupport() will not return null
in blink. I do see a few call sites that do a nullity check, but looking at
render_thread_impl.cc, it looks like we EnsureWebKitInitialized at the
appropriate times to guarantee we have one. Is this check really necessary?

https://codereview.chromium.org/1133513002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right):

https://codereview.chromium.org/1133513002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/ProgrammaticScrollAnimator.cpp:178: bool animatonAdded =
false;
nit: animation.

https://codereview.chromium.org/1133513002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/ProgrammaticScrollAnimator.cpp:179: if
(m_compositorPlayer && m_compositorPlayer->isLayerAttached()) {
If we have a compositor player, should we expect to have the layer attached at
this point?

[loyso (OOO), 2015-07-22 05:24:23]

https://codereview.chromium.org/1133513002/diff/20001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/1133513002/diff/20001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.cpp:97: if
(RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() &&
Platform::current()->compositorSupport()) {
On 2015/07/10 03:06:45, vollick wrote:
> AFAICT, most of the time we assume that compositorSupport() will not return
null
> in blink. I do see a few call sites that do a nullity check, but looking at
> render_thread_impl.cc, it looks like we EnsureWebKitInitialized at the
> appropriate times to guarantee we have one. Is this check really necessary?
Done.

https://codereview.chromium.org/1133513002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right):

https://codereview.chromium.org/1133513002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/ProgrammaticScrollAnimator.cpp:178: bool animatonAdded =
false;
On 2015/07/10 03:06:45, vollick (ooo until July 27) wrote:
> nit: animation.

Done.

https://codereview.chromium.org/1133513002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/ProgrammaticScrollAnimator.cpp:179: if
(m_compositorPlayer && m_compositorPlayer->isLayerAttached()) {
On 2015/07/10 03:06:45, vollick (ooo until July 27) wrote:
> If we have a compositor player, should we expect to have the layer attached at
> this point?
If it's not attached, it runs on main thread (!sentToCompositor).
Reworked a bit.

[ajuma, 2015-07-22 13:38:30]

One nit about the virtual test suite. lgtm if the tests still pass after
addressing that.

https://codereview.chromium.org/1133513002/diff/40001/LayoutTests/VirtualTest...
File LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1133513002/diff/40001/LayoutTests/VirtualTest...
LayoutTests/VirtualTestSuites:51: "base": "fast/scroll-behavior",
How does having two virtual test suites with the same prefix+base work (since we
already have virtual/threaded/fast/scroll-behavior, defined above)? That is,
will virtual/threaded/fast/scroll-behavior run with the args above, or the ones
below, or somehow run twice? In any case, please change the prefix to something
like "threaded_animation_timelines" to avoid confusion.

[loyso (OOO), 2015-07-23 01:18:04]

https://codereview.chromium.org/1133513002/diff/40001/LayoutTests/VirtualTest...
File LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1133513002/diff/40001/LayoutTests/VirtualTest...
LayoutTests/VirtualTestSuites:51: "base": "fast/scroll-behavior",
On 2015/07/22 13:38:29, ajuma wrote:
> How does having two virtual test suites with the same prefix+base work (since
we
> already have virtual/threaded/fast/scroll-behavior, defined above)? That is,
> will virtual/threaded/fast/scroll-behavior run with the args above, or the
ones
> below, or somehow run twice? In any case, please change the prefix to
something
> like "threaded_animation_timelines" to avoid confusion.

Done.

[loyso (OOO), 2015-07-29 03:30:13]

PTAL!

[ajuma, 2015-07-29 13:14:27]

On 2015/07/29 03:30:13, loyso wrote:
> PTAL!

Still lgtm.

[Ian Vollick, 2015-07-30 21:56:38]

On 2015/07/29 13:14:27, ajuma wrote:
> On 2015/07/29 03:30:13, loyso wrote:
> > PTAL!
> 
> Still lgtm.

lgtm.

[Ian Vollick, 2015-07-30 21:56:41]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2015-07-30 21:57:05]

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

[commit-bot: I haz the power, 2015-07-30 23:01:38]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199766

[johnme, 2015-07-31 10:15:44]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1254963011/ by johnme@chromium.org.

The reason for reverting is: Since this landed, 8 out of 9 runs of
virtual/threaded_animation_timelines/fast/scroll-behavior/overflow-scroll-root-frame-animates.html
have timed out on WebKit Linux MSAN:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=vir...

There has also been a timeout on WebKit Win Oilpan (dbg):

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

[ajuma, 2015-07-31 14:29:07]

On 2015/07/31 10:15:44, johnme wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/1254963011/ by mailto:johnme@chromium.org.
> 
> The reason for reverting is: Since this landed, 8 out of 9 runs of
>
virtual/threaded_animation_timelines/fast/scroll-behavior/overflow-scroll-root-frame-animates.html
> have timed out on WebKit Linux MSAN:
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=vir...
> 
> There has also been a timeout on WebKit Win Oilpan (dbg):
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20....

loyso, I think you need to add the following expectations (to match those we
already have for virtual/threaded/fast/scroll-behavior):
-skip the entire suite on Linux ASAN bots (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
-mark the entire suite as slow (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)

If that's enough to make the tests pass, feel free to re-land with those changes
without waiting for another review.

[loyso (OOO), 2015-08-03 01:36:50]

On 2015/07/31 14:29:07, ajuma (OOO till Aug. 4th) wrote:
> loyso, I think you need to add the following expectations (to match those we
> already have for virtual/threaded/fast/scroll-behavior):
> -skip the entire suite on Linux ASAN bots (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> -mark the entire suite as slow (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> 
> If that's enough to make the tests pass, feel free to re-land with those
changes
> without waiting for another review.
Thanks, Ali! That's what we need.
I created a re-land CL here: https://codereview.chromium.org/1263103003/