Allow impl-only animations, and return opacity values via AnimationEvents.

Allow impl-only animations, and return opacity values via AnimationEvents.

We would like to allow animations to run only on the impl thread, so we need to make sure they don't get clobbered when finished animations are removed. Also, we need a mechanism to allow opacity values to propagate back to the main-thread so that impl-only opacity animations have their values reflected on the main thread when the animation is complete.

BUG=165842
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186725

[wjmaclean, 2013-03-05 19:28:28]

Here's the impl-only animation CL, can you please take a look?

[Ian Vollick, 2013-03-06 01:00:57]

This is pretty close to what I was hoping for, but I'd like to add support for
plumbing back transform animation updates from the get-go.

https://codereview.chromium.org/12453010/diff/1/cc/animation.h
File cc/animation.h (right):

https://codereview.chromium.org/12453010/diff/1/cc/animation.h#newcode110
cc/animation.h:110: void setIsImplAnimationOnly(bool isImplOnly) {
m_isImplAnimationOnly = isImplOnly; }
nit: setIsImplOnly. The 'Animation' bit adds no extra info.

https://codereview.chromium.org/12453010/diff/1/cc/animation_events.h
File cc/animation_events.h (right):

https://codereview.chromium.org/12453010/diff/1/cc/animation_events.h#newcode23
cc/animation_events.h:23: , value(val)
nit: Since I'd like to support plumbing back transforms, I would prefer to avoid
the default parameter and have the constructor initialize the float and
transform values to 0 and I, respectively. Anyone sending back property updates
should set the value as a separate step.

https://codereview.chromium.org/12453010/diff/1/cc/animation_events.h#newcode32
cc/animation_events.h:32: float value;
I know that you're only interested in floats at the moment, but if we're going
to allow property updates to bubble back, I'd prefer to allow for transforms as
well.

https://codereview.chromium.org/12453010/diff/1/cc/layer.cc
File cc/layer.cc (right):

https://codereview.chromium.org/12453010/diff/1/cc/layer.cc#newcode880
cc/layer.cc:880: setOpacity(event.value);
Please add an 'else if' for transform.

https://codereview.chromium.org/12453010/diff/1/cc/layer_animation_controller.cc
File cc/layer_animation_controller.cc (right):

https://codereview.chromium.org/12453010/diff/1/cc/layer_animation_controller...
cc/layer_animation_controller.cc:135: if
(m_activeAnimations[i]->targetProperty() == Animation::Opacity) {
Need to check that the animation is impl only here. Will also need to accumulate
transform updates.

https://codereview.chromium.org/12453010/diff/1/cc/layer_animation_controller...
File cc/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/12453010/diff/1/cc/layer_animation_controller...
cc/layer_animation_controller_unittest.cc:171: 
I would like 3 new tests rather than these tweaks on the old tests:
1) Ensure that animations with a main thread counterpart do not issue property
update events.
2) Ensure that impl-only opacity animations send back property update events
with the correct value.
3) Same as 2), but for transform animations.

[wjmaclean, 2013-03-06 17:03:13]

https://codereview.chromium.org/12453010/diff/1/cc/animation.h
File cc/animation.h (right):

https://codereview.chromium.org/12453010/diff/1/cc/animation.h#newcode110
cc/animation.h:110: void setIsImplAnimationOnly(bool isImplOnly) {
m_isImplAnimationOnly = isImplOnly; }
On 2013/03/06 01:00:57, vollick wrote:
> nit: setIsImplOnly. The 'Animation' bit adds no extra info.

Done.

https://codereview.chromium.org/12453010/diff/1/cc/animation_events.h
File cc/animation_events.h (right):

https://codereview.chromium.org/12453010/diff/1/cc/animation_events.h#newcode23
cc/animation_events.h:23: , value(val)
On 2013/03/06 01:00:57, vollick wrote:
> nit: Since I'd like to support plumbing back transforms, I would prefer to
avoid
> the default parameter and have the constructor initialize the float and
> transform values to 0 and I, respectively. Anyone sending back property
updates
> should set the value as a separate step.

Done.

https://codereview.chromium.org/12453010/diff/1/cc/animation_events.h#newcode32
cc/animation_events.h:32: float value;
On 2013/03/06 01:00:57, vollick wrote:
> I know that you're only interested in floats at the moment, but if we're going
> to allow property updates to bubble back, I'd prefer to allow for transforms
as
> well.

Done.

https://codereview.chromium.org/12453010/diff/1/cc/layer_animation_controller.cc
File cc/layer_animation_controller.cc (right):

https://codereview.chromium.org/12453010/diff/1/cc/layer_animation_controller...
cc/layer_animation_controller.cc:135: if
(m_activeAnimations[i]->targetProperty() == Animation::Opacity) {
On 2013/03/06 01:00:57, vollick wrote:
> Need to check that the animation is impl only here. Will also need to
accumulate
> transform updates.

Done.

https://codereview.chromium.org/12453010/diff/1/cc/layer_animation_controller...
File cc/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/12453010/diff/1/cc/layer_animation_controller...
cc/layer_animation_controller_unittest.cc:171: 
On 2013/03/06 01:00:57, vollick wrote:
> I would like 3 new tests rather than these tweaks on the old tests:
> 1) Ensure that animations with a main thread counterpart do not issue property
> update events.

Now that non implOnly animations do not send back property updates, can the
existing test cover this? (once I restore the checks on the size of the events
queue to their original size ...)

> 2) Ensure that impl-only opacity animations send back property update events
> with the correct value.
> 3) Same as 2), but for transform animations.

Done.

[wjmaclean, 2013-03-06 19:24:40]

PTAL.

[Ian Vollick, 2013-03-06 23:29:45]

lgtm with nits.

https://codereview.chromium.org/12453010/diff/11001/cc/animation_events.h
File cc/animation_events.h (right):

https://codereview.chromium.org/12453010/diff/11001/cc/animation_events.h#new...
cc/animation_events.h:34: gfx::Transform transform;
nit: s/value/opacity/. Here and elsewhere.

https://codereview.chromium.org/12453010/diff/11001/cc/layer_animation_contro...
File cc/layer_animation_controller.cc (right):

https://codereview.chromium.org/12453010/diff/11001/cc/layer_animation_contro...
cc/layer_animation_controller.cc:144: event.value = opacity;
aesthetic nit: Please avoid the temporary as you do in the case of the
transform. It'll be more symmetrical.

https://codereview.chromium.org/12453010/diff/11001/cc/layer_animation_contro...
File cc/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/12453010/diff/11001/cc/layer_animation_contro...
cc/layer_animation_controller_unittest.cc:191: 
I'd also like you to confirm that we don't generate property updates if it's not
an impl only animation. I'd be happy if in one of the existing tests you added a
spot check to make sure that the animation events are sane.

https://codereview.chromium.org/12453010/diff/11001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12453010/diff/11001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:891: case AnimationEvent::Started :
nit: no space between the enumerators and the colon.

[wjmaclean, 2013-03-07 13:49:29]

https://codereview.chromium.org/12453010/diff/11001/cc/animation_events.h
File cc/animation_events.h (right):

https://codereview.chromium.org/12453010/diff/11001/cc/animation_events.h#new...
cc/animation_events.h:34: gfx::Transform transform;
On 2013/03/06 23:29:45, vollick wrote:
> nit: s/value/opacity/. Here and elsewhere.

Done.

https://codereview.chromium.org/12453010/diff/11001/cc/layer_animation_contro...
File cc/layer_animation_controller.cc (right):

https://codereview.chromium.org/12453010/diff/11001/cc/layer_animation_contro...
cc/layer_animation_controller.cc:144: event.value = opacity;
On 2013/03/06 23:29:45, vollick wrote:
> aesthetic nit: Please avoid the temporary as you do in the case of the
> transform. It'll be more symmetrical.

Done.

https://codereview.chromium.org/12453010/diff/11001/cc/layer_animation_contro...
File cc/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/12453010/diff/11001/cc/layer_animation_contro...
cc/layer_animation_controller_unittest.cc:191: 
On 2013/03/06 23:29:45, vollick wrote:
> I'd also like you to confirm that we don't generate property updates if it's
not
> an impl only animation. I'd be happy if in one of the existing tests you added
a
> spot check to make sure that the animation events are sane.

We already do that implicitly, in that the checks on events->size() would fail
here if there were any property update events. However, I can easily (and
explicitly) verify there are no property updates in the events just to confirm.

https://codereview.chromium.org/12453010/diff/11001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12453010/diff/11001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:891: case AnimationEvent::Started :
On 2013/03/06 23:29:45, vollick wrote:
> nit: no space between the enumerators and the colon.

Done.

[commit-bot: I haz the power, 2013-03-07 13:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/12453010/16001

[commit-bot: I haz the power, 2013-03-07 14:15:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/12453010/26002

[commit-bot: I haz the power, 2013-03-07 16:16:43]

Change committed as 186725