Handle direction control in compositor Animations

Handle direction control in compositor Animations

This patch is the compositor side. The blink side is handled in issue #178263006

Handle reverse animations in the compositor. Currently, compositor only
understand alternate direction. And in order to reverse an animation, it
needs to be passed a reversed timing function.

This patch introduces a Direction enum parameter in animation.

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

[ajuma, 2014-02-28 17:55:50]

Thanks for working on this.

Please add a bug number to the description.

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

https://codereview.chromium.org/180153010/diff/1/cc/animation/animation.cc#ne...
cc/animation/animation.cc:210: (direction_ == AlternateReverse && iteration % 2
== 0);
Looks like an extra space at the start of each of these two lines, so
double-check that you've clang-formatted.

https://codereview.chromium.org/180153010/diff/1/webkit/renderer/compositor_b...
File webkit/renderer/compositor_bindings/web_animation_impl.cc (right):

https://codereview.chromium.org/180153010/diff/1/webkit/renderer/compositor_b...
webkit/renderer/compositor_bindings/web_animation_impl.cc:105:
static_cast<cc::Animation::Direction>(direction));
I know we have existing code that assumes that enums in cc and in Blink happen
to have the same values, but we should probably avoid adding more such
dependencies for new enums. Instead, we can just use a switch here and
explicitly convert from the Blink enum to the cc enum.

[a.renevier, 2014-02-28 21:37:25]

new patch: address reviewer comment

[mithro-old, 2014-03-03 06:30:52]

Hi a.renevier,

I'm excited to see the compositor gain direction support and remove the need for
the reversing code in Blink! 

I have just two very minor comments. You **shouldn't** wait for a LGTM from me
before landing this patch.

Can you also add links between the Chrome side/Blink side of these CLs?

Tim

https://codereview.chromium.org/180153010/diff/20001/cc/animation/animation_u...
File cc/animation/animation_unittest.cc (right):

https://codereview.chromium.org/180153010/diff/20001/cc/animation/animation_u...
cc/animation/animation_unittest.cc:52: TEST(AnimationTest, TrimTimeReverse) {
This suggestion is **optional**, it doesn't block landing this patch.

It would be nice to see a simple test which showed TrimTimeReverse/Alternative
interacting with pause time, start time and offset.

https://codereview.chromium.org/180153010/diff/20001/webkit/renderer/composit...
File webkit/renderer/compositor_bindings/web_animation_impl.cc (right):

https://codereview.chromium.org/180153010/diff/20001/webkit/renderer/composit...
webkit/renderer/compositor_bindings/web_animation_impl.cc:159: #endif
Should there be the following ASSERT? (Following the other ASSERTS)

COMPILE_ASSERT_MATCHING_ENUMS(
 WebAnimation::Direction, cc:Animation::Direction);

[ajuma, 2014-03-03 15:03:20]

https://codereview.chromium.org/180153010/diff/20001/webkit/renderer/composit...
File webkit/renderer/compositor_bindings/web_animation_impl.cc (right):

https://codereview.chromium.org/180153010/diff/20001/webkit/renderer/composit...
webkit/renderer/compositor_bindings/web_animation_impl.cc:159: #endif
On 2014/03/03 06:30:52, mithro wrote:
> Should there be the following ASSERT? (Following the other ASSERTS)
> 
> COMPILE_ASSERT_MATCHING_ENUMS(
>  WebAnimation::Direction, cc:Animation::Direction);

No, note that the current version of this CL no longer assumes that the enums
match, so such an assert isn't needed.

[ajuma, 2014-03-04 19:02:38]

This looks good. Please add a bug number to the description (if there isn't
already an open bug for this work, please create one).

Also, before I can lg2m this, you'll need to sign the CLA or get yourself added
as one of Samsung's authorized contributors (see
http://dev.chromium.org/developers/contributing-code/external-contributor-che...).

https://codereview.chromium.org/180153010/diff/20001/cc/animation/animation.h
File cc/animation/animation.h (right):

https://codereview.chromium.org/180153010/diff/20001/cc/animation/animation.h...
cc/animation/animation.h:51: enum Direction { Normal = 0, Reverse, Alternate,
AlternateReverse };
Is the "= 0" needed? (The previous enum only needs it because we want it to have
the same values as the corresponding blink enum.)

[a.renevier, 2014-03-04 19:54:42]

new patch: add tests for start_time, time_offset, pause and suspend with reverse
direction; also removes =0 for first enum member

[a.renevier, 2014-03-04 19:56:35]

On 2014/03/04 19:02:38, ajuma wrote:
> 
> Also, before I can lg2m this, you'll need to sign the CLA or get yourself
added
> as one of Samsung's authorized contributors (see
>
http://dev.chromium.org/developers/contributing-code/external-contributor-che...).

I'm currently waiting for person of charge of that at Samsung to handle that.
Hopefully this will happen soon.

[ajuma, 2014-03-11 23:02:04]

Thanks for the added tests.

> Also, before I can lg2m this, you'll need to sign the CLA or get yourself
added
> as one of Samsung's authorized contributors (see
>
http://dev.chromium.org/developers/contributing-code/external-contributor-che...).

Since this has now been done, lgtm.

Note that you'll need to add yourself to AUTHORS in whichever CL you land first.

Also note that this CL still needs an OWNERS review for the compositor_bindings
changes.

[jamesr, 2014-03-11 23:05:42]

webkit/renderer/compositor_bindings/ lgtm, although you'll need to update the
enum style to match the rest of the blink public API (I left a comment on the
blink side patch).

[a.renevier, 2014-03-19 23:59:41]

new patch that fixes the enum style issue

[a.renevier, 2014-03-26 21:09:36]

The CQ bit was checked by a.renevier@samsung.com

[commit-bot: I haz the power, 2014-03-26 21:10:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.renevier@samsung.com/180153010/90001

[commit-bot: I haz the power, 2014-03-27 00:36:44]

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

[commit-bot: I haz the power, 2014-03-27 00:36:45]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[a.renevier, 2014-03-31 17:12:20]

The CQ bit was checked by a.renevier@samsung.com

[commit-bot: I haz the power, 2014-03-31 17:13:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.renevier@samsung.com/180153010/90001

[commit-bot: I haz the power, 2014-03-31 20:22:30]

Change committed as 260628