For SPv2, attach element to compositor animation player without a CLM.

For SPv2, attach element to compositor animation player without a CLM.

BUG=674317
Review-Url: https://codereview.chromium.org/2643283003
Cr-Commit-Position: refs/heads/master@{#445494}
Committed: https://chromium.googlesource.com/chromium/src/+/653f787c53cd2f9104e85dc61238d9ef25e9d347

[wkorman, 2017-01-20 23:27:35]

wkorman@chromium.org changed reviewers:
+ ajuma@chromium.org, pdr@chromium.org

[wkorman, 2017-01-20 23:27:36]

I spent about an hour working on a unit test and concluded it could take half a
day or more. Given the old code path will become obsolete and this change is
fairly straightforward I thought punting could be reasonable but open to
thought.

The problem is that the existing unit tests are all geared around testing other
logic around whether an animation should be composited, and its timing.

I started work to add a RenderingTest to check the logic modified in this test
but am having trouble accessing the animation in question, and once I had a
sense of time remaining to do the work, decided to seek feedback.

[wkorman, 2017-01-20 23:45:47]

https://codereview.chromium.org/2643283003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/2643283003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:461:
compositorPlayer->attachElement(createCompositorElementId(
This will end up at this check:

 
https://cs.chromium.org/chromium/src/cc/animation/animation_player.cc?q=anima...

  if (animation_host_)
      RegisterPlayer();

I'm not yet sure whether animation_host_ will be true when we need it to be.
Further changes may be needed but this seems an incremental step with what I
know currently.

[wkorman, 2017-01-20 23:51:36]

On 2017/01/20 at 23:45:47, wkorman wrote:
>
https://codereview.chromium.org/2643283003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp
(right):
> 
>
https://codereview.chromium.org/2643283003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:461:
compositorPlayer->attachElement(createCompositorElementId(
> This will end up at this check:
> 
>  
https://cs.chromium.org/chromium/src/cc/animation/animation_player.cc?q=anima...
> 
>   if (animation_host_)
>       RegisterPlayer();
> 
> I'm not yet sure whether animation_host_ will be true when we need it to be.
Further changes may be needed but this seems an incremental step with what I
know currently.

I added logging and checked locally and it looks like we do have animation_host_
set up at this point for a simple composited animation test case.

[ajuma, 2017-01-23 14:55:08]

On 2017/01/20 23:27:36, wkorman wrote:
> I spent about an hour working on a unit test and concluded it could take half
a
> day or more. Given the old code path will become obsolete and this change is
> fairly straightforward I thought punting could be reasonable but open to
> thought.
> 
> The problem is that the existing unit tests are all geared around testing
other
> logic around whether an animation should be composited, and its timing.

Yeah, I don't know of a straightforward way to unit test just the logic that's
being changed here. Given that this change is a step towards getting the
virtual/threaded/animations tests passing with SPv2, landing this as-is lgtm.

[pdr., 2017-01-23 19:12:18]

On 2017/01/23 at 14:55:08, ajuma wrote:
> On 2017/01/20 23:27:36, wkorman wrote:
> > I spent about an hour working on a unit test and concluded it could take
half a
> > day or more. Given the old code path will become obsolete and this change is
> > fairly straightforward I thought punting could be reasonable but open to
> > thought.
> > 
> > The problem is that the existing unit tests are all geared around testing
other
> > logic around whether an animation should be composited, and its timing.
> 
> Yeah, I don't know of a straightforward way to unit test just the logic that's
being changed here. Given that this change is a step towards getting the
virtual/threaded/animations tests passing with SPv2, landing this as-is lgtm.

LGTM

[pdr., 2017-01-23 19:12:30]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-01-23 19:13:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 20:18:06]

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

[commit-bot: I haz the power, 2017-01-23 20:18:08]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[wkorman, 2017-01-23 20:45:06]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-01-23 20:45:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 21:41:42]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485204306792500, "parent_rev":
"6ce567dadd1500a586fb40d667a4bc618a46b87b", "commit_rev":
"653f787c53cd2f9104e85dc61238d9ef25e9d347"}

[commit-bot: I haz the power, 2017-01-23 21:42:18]

Description was changed from

==========
For SPv2, attach element to compositor animation player without a CLM.

BUG=674317
==========

to

==========
For SPv2, attach element to compositor animation player without a CLM.

BUG=674317

Review-Url: https://codereview.chromium.org/2643283003
Cr-Commit-Position: refs/heads/master@{#445494}
Committed:
https://chromium.googlesource.com/chromium/src/+/653f787c53cd2f9104e85dc61238...
==========

[commit-bot: I haz the power, 2017-01-23 21:42:19]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/653f787c53cd2f9104e85dc61238...

[loyso (OOO), 2017-01-24 00:48:20]

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

[loyso (OOO), 2017-01-24 00:48:20]

https://codereview.chromium.org/2643283003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/2643283003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:461:
compositorPlayer->attachElement(createCompositorElementId(
cc::AnimationPlayer::animation_host_ is not nullptr if
1) Player is added to timeline
2) Timeline is added to AnimationHost.