Handle direction control in compositor Animations

Handle direction control in compositor Animations

This patch is the blink side. The chromium side is handled on issue #180153010

It doesn't revert functions anymore in blink. Instead, it sets the direction parameter to WebAnimation, which will in turn pass it to the compositor.

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

[a.renevier, 2014-04-07 18:37:13]

commit was reverted in issue 220503002; create new issue to retry commit

[rafaelw1, 2014-04-07 18:40:09]

Please alert the blink gardeners when plan to land these


On Mon, Apr 7, 2014 at 11:37 AM, <a.renevier@samsung.com> wrote:

> Reviewers: rafaelw, dstockwell, ajuma, mithro, jamesr (out of office),
> abarth,
>
> Message:
> commit was reverted in issue 220503002; create new issue to retry commit
>
> Description:
> Handle direction control in compositor Animations
>
> This patch is the blink side. The chromium side is handled on issue
> #180153010
>
> It doesn't revert functions anymore in blink. Instead, it sets the
> direction
> parameter to WebAnimation, which will in turn pass it to the compositor.
>
> BUG=348071
>
> Please review this at https://codereview.chromium.org/225183014/
>
> SVN Base: https://chromium.googlesource.com/chromium/blink.git@master
>
> Affected files (+67, -209 lines):
>   M Source/core/animation/CompositorAnimations.h
>   M Source/core/animation/CompositorAnimations.cpp
>   M Source/core/animation/CompositorAnimationsImpl.h
>   M Source/core/animation/CompositorAnimationsTest.cpp
>   M Source/core/animation/CompositorAnimationsTestHelper.h
>   D Source/core/animation/CompositorAnimationsTimingFunc
> tionReverserTest.cpp
>   M Source/core/core.gypi
>   M public/platform/WebAnimation.h
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-04-09 01:46:50]

Hi a.renevier,

I'm not expert on landing patches which effect both Blink and Chromium, but the
method I have used is the following;

 * Land a patch which extends the API to support the new feature.
 * Wait for DEPS to be rolled and bots to catch up.
 * Land a patch which uses the new API.
 * Wait for DEPS to be rolled and bots to catch up.
 * Land a patch which removes the old API.

While this is a rather slow process, the tree should never be broken. When using
this process *please* make sure you land the final CL which does the clean up /
removal of the old API. It is easy to get distracted and forget to do the final
step because things are now working.

The two methods I have seen to do the API extension are;

 * Using #ifdefs something like;
-------------------------------------------------
#ifdef CHROMIUM_SUPPORTS_REVERSE
  doSomething();
#else
  doSomethingElse();
#endif
-------------------------------------------------

 * Using overloads / multiple methods;
-------------------------------------------------
bool addWithReverse(blah) {}
bool addWithoutReverse(blah) {}

if (addWithReverse(args)) {
} else {
  addWithoutReverse(args);
}
-------------------------------------------------

If you want to discuss this more, feel free to poke me on #chromium or #blink
IRC channels on irc.freenode.net (I'm mithro). Others who have more experience
might have better suggestions or alternative ideas on how to do this patch and
I'm 100% happy for you to follow their guidance too.

Looking forward to seeing the reverse code gone (despite having spent way to
long writing it). This should also enable acceleration of a number of animations
which where previously non-reversible.

Hope that helps and good luck!

Tim 'mithro' Ansell

[a.renevier, 2014-04-09 18:34:59]

Thanks a lot to everybody for the explanations.
The problem was indeed in
webkit/renderer/compositor_bindings/web_animation_unittest.cc
I only tried to build chrome. Now, I now that to make sure, i must build "all",
in order to not forget anything.
I've uploaded an updated patch.
The blink patch doesn't need any update.

[ikilpatrick, 2014-09-02 00:30:44]

On 2014/07/02 18:53:29, a.renevier wrote:

Hi a.renevier,

Thanks for adding direction control onto the compositor. I'm currently starting
to work in this area, would you mind rebasing this patch to ToT?

Thanks,
Ian

[a.renevier, 2014-09-03 23:36:53]

That's uploaded

[dstockwell, 2014-09-09 23:40:33]

dstockwell@chromium.org changed reviewers:
- abarth@chromium.org, jamesr@chromium.org, mithro@mithis.com,
rafaelw@chromium.org, rafaelw@google.com

[dstockwell, 2014-09-12 01:38:53]

lgtm

Please add a manual test (similar to
ManualTests/animation/compositor-animation-iteration-count.html)

[ikilpatrick, 2014-09-12 22:34:43]

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

[ikilpatrick, 2014-09-12 22:34:44]

lgtm

[a.renevier, 2014-09-12 22:41:50]

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

[commit-bot: I haz the power, 2014-09-12 22:42:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/225183014/60001

[commit-bot: I haz the power, 2014-09-12 22:51:37]

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

[commit-bot: I haz the power, 2014-09-12 22:51:38]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[a.renevier, 2014-09-12 22:58:55]

a.renevier@samsung.com changed reviewers:
+ abarth@chromium.org, dglazkov@chromium.org

[a.renevier, 2014-09-12 22:59:16]

added reviewers of public directory

[dglazkov, 2014-09-12 23:00:34]

lgtm

[a.renevier, 2014-09-12 23:36:31]

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

[commit-bot: I haz the power, 2014-09-12 23:37:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/225183014/60001

[commit-bot: I haz the power, 2014-09-12 23:47:11]

Committed patchset #4 (id:60001) as 181945