First programmatic smooth scroll should run on the compositor if it can

First programmatic smooth scroll should run on the compositor if it can

CompositorAnimationPlayer owned by a ScrollableArea's ProgrammaticScrollAnimator
is not initially attached to any layer.  As a result,
ScrollAnimatorCompositorCoordinator::addAnimation returns false the first time
a programmatic scroll is initiated.

This CL attaches the animation player to the correct layer when
::updateCompositorState is called. To do so, we need the CompositorAnimationTimeline
from the ScrollingCoordinator of a ScrollableArea. This CL exposes the timeline from
the subclasses of ScrollableArea.

BUG=592799
Committed: https://crrev.com/899920c656fc71ca44cb81ea3a1b73639dab52e1
Cr-Commit-Position: refs/heads/master@{#381379}

[ymalik, 2016-03-14 16:04:33]

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

[ajuma, 2016-03-14 17:01:51]

Please expand the description to say more about what this patch does. The patch
itself lgtm.

[ymalik, 2016-03-14 17:20:37]

Description was changed from

==========
First programmatic smooth scroll should run on the compositor if it can

BUG=592799
==========

to

==========
First programmatic smooth scroll should run on the compositor if it can

CompositorAnimationPlayer owned by a ScrollableArea's ProgrammaticScrollAnimator

is not initially attached to any layer.  As a result, 
ScrollAnimatorCompositorCoordinator::addAnimation returns false the first time
a programmatic scroll is initiated.

This CL attaches the animation player to the correct layer when 
::updateCompositorState is called. To do so, we need the
CompositorAnimationTimeline
from the ScrollingCoordinator of a ScrollableArea. This CL exposes the timeline
from
the subclasses of ScrollableArea.

BUG=592799
==========

[ymalik, 2016-03-14 17:22:57]

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

[ymalik, 2016-03-14 17:22:58]

+Jeremy for Source/core and Source/platform.

[jbroman, 2016-03-14 17:35:32]

lgtm with nits

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/first-scroll-runs-on-compositor.html
(right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/first-scroll-runs-on-compositor.html:24:
shouldBeTrue("internals.getProgrammaticScrollAnimationState(document) " +
nit: equality comparison is why shouldBe exists:

shouldBe("internals.getProgrammaticScrollAnimationState(document)",
"RUNNING_ON_COMPOSITOR");

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:440: return c ?
c->compositorAnimationTimeline() : 0;
Prefer nullptr to 0.

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:727: return c ?
c->compositorAnimationTimeline() : 0;
Prefer nullptr to 0.

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1471: return
page->scrollingCoordinator() ?
page->scrollingCoordinator()->compositorAnimationTimeline() : 0;
Prefer nullptr to 0.

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:2579: return
static_cast<int>(scrollableArea->programmaticScrollAnimator().m_runState);
This seems super-internal, even for Internals. It's already here, so I guess I
won't demand it be changed now, but I'd at least prefer a string, so we won't
have layout tests breaking everywhere if we ever reorder the enumerators.

[skobes, 2016-03-14 17:37:48]

skobes@chromium.org changed reviewers:
+ skobes@chromium.org

[skobes, 2016-03-14 17:37:48]

lgtm

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/first-scroll-runs-on-compositor.html
(right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/first-scroll-runs-on-compositor.html:16:
// From ScrollingCoordinator::mainThreadScrollingReasonsAsText.
nit: actually from ScrollAnimatorCompositorCoordinator::RunState

[ymalik, 2016-03-14 21:45:40]

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/first-scroll-runs-on-compositor.html
(right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/first-scroll-runs-on-compositor.html:16:
// From ScrollingCoordinator::mainThreadScrollingReasonsAsText.
On 2016/03/14 17:37:48, skobes wrote:
> nit: actually from ScrollAnimatorCompositorCoordinator::RunState

Thanks, copy paste error.

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/first-scroll-runs-on-compositor.html:24:
shouldBeTrue("internals.getProgrammaticScrollAnimationState(document) " +
On 2016/03/14 17:35:32, jbroman wrote:
> nit: equality comparison is why shouldBe exists:
> 
> shouldBe("internals.getProgrammaticScrollAnimationState(document)",
> "RUNNING_ON_COMPOSITOR");

Done.

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:440: return c ?
c->compositorAnimationTimeline() : 0;
On 2016/03/14 17:35:32, jbroman wrote:
> Prefer nullptr to 0.

Done.

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:727: return c ?
c->compositorAnimationTimeline() : 0;
On 2016/03/14 17:35:32, jbroman wrote:
> Prefer nullptr to 0.

Done.

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1471: return
page->scrollingCoordinator() ?
page->scrollingCoordinator()->compositorAnimationTimeline() : 0;
On 2016/03/14 17:35:32, jbroman wrote:
> Prefer nullptr to 0.

Done.

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/1801753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:2579: return
static_cast<int>(scrollableArea->programmaticScrollAnimator().m_runState);
On 2016/03/14 17:35:32, jbroman wrote:
> This seems super-internal, even for Internals. It's already here, so I guess I
> won't demand it be changed now, but I'd at least prefer a string, so we won't
> have layout tests breaking everywhere if we ever reorder the enumerators.

Done. Will change it above in a follow up CL to not clutter this change with
other layout test fixes.

[ymalik, 2016-03-14 22:23:51]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-03-14 22:23:52]

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

[commit-bot: I haz the power, 2016-03-14 22:24:15]

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

[commit-bot: I haz the power, 2016-03-14 22:37:58]

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

[commit-bot: I haz the power, 2016-03-14 22:37:59]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[ymalik, 2016-03-15 17:09:41]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-03-15 17:09:41]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
skobes@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1801753002/#ps80001
(title: "fix bot compile error")

[commit-bot: I haz the power, 2016-03-15 17:10:03]

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

[commit-bot: I haz the power, 2016-03-15 21:09:55]

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

[commit-bot: I haz the power, 2016-03-15 21:09:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ymalik, 2016-03-15 22:19:20]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-03-15 22:19:35]

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

[commit-bot: I haz the power, 2016-03-16 01:52:53]

Description was changed from

==========
First programmatic smooth scroll should run on the compositor if it can

CompositorAnimationPlayer owned by a ScrollableArea's ProgrammaticScrollAnimator

is not initially attached to any layer.  As a result, 
ScrollAnimatorCompositorCoordinator::addAnimation returns false the first time
a programmatic scroll is initiated.

This CL attaches the animation player to the correct layer when 
::updateCompositorState is called. To do so, we need the
CompositorAnimationTimeline
from the ScrollingCoordinator of a ScrollableArea. This CL exposes the timeline
from
the subclasses of ScrollableArea.

BUG=592799
==========

to

==========
First programmatic smooth scroll should run on the compositor if it can

CompositorAnimationPlayer owned by a ScrollableArea's ProgrammaticScrollAnimator

is not initially attached to any layer.  As a result, 
ScrollAnimatorCompositorCoordinator::addAnimation returns false the first time
a programmatic scroll is initiated.

This CL attaches the animation player to the correct layer when 
::updateCompositorState is called. To do so, we need the
CompositorAnimationTimeline
from the ScrollingCoordinator of a ScrollableArea. This CL exposes the timeline
from
the subclasses of ScrollableArea.

BUG=592799
==========

[commit-bot: I haz the power, 2016-03-16 01:52:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-16 01:55:35]

Description was changed from

==========
First programmatic smooth scroll should run on the compositor if it can

CompositorAnimationPlayer owned by a ScrollableArea's ProgrammaticScrollAnimator

is not initially attached to any layer.  As a result, 
ScrollAnimatorCompositorCoordinator::addAnimation returns false the first time
a programmatic scroll is initiated.

This CL attaches the animation player to the correct layer when 
::updateCompositorState is called. To do so, we need the
CompositorAnimationTimeline
from the ScrollingCoordinator of a ScrollableArea. This CL exposes the timeline
from
the subclasses of ScrollableArea.

BUG=592799
==========

to

==========
First programmatic smooth scroll should run on the compositor if it can

CompositorAnimationPlayer owned by a ScrollableArea's ProgrammaticScrollAnimator
is not initially attached to any layer.  As a result,
ScrollAnimatorCompositorCoordinator::addAnimation returns false the first time
a programmatic scroll is initiated.

This CL attaches the animation player to the correct layer when
::updateCompositorState is called. To do so, we need the
CompositorAnimationTimeline
from the ScrollingCoordinator of a ScrollableArea. This CL exposes the timeline
from
the subclasses of ScrollableArea.

BUG=592799

Committed: https://crrev.com/899920c656fc71ca44cb81ea3a1b73639dab52e1
Cr-Commit-Position: refs/heads/master@{#381379}
==========

[commit-bot: I haz the power, 2016-03-16 01:55:37]

Patchset 5 (id:??) landed as
https://crrev.com/899920c656fc71ca44cb81ea3a1b73639dab52e1
Cr-Commit-Position: refs/heads/master@{#381379}