[webkit-reviews] review canceled: [Bug 73233] [chromium] Create a base-class CCAnimation to represent compositor animations : [Attachment 116982] Patch

[bugzilla-daemon at webkit.org]

Nat Duca <nduca at chromium.org> has canceled vollick at chromium.org's request for
review:
Bug 73233: [chromium] Create a base-class CCAnimation to represent compositor
animations
https://bugs.webkit.org/show_bug.cgi?id=73233

Attachment 116982: Patch
https://bugs.webkit.org/attachment.cgi?id=116982&action=review

------- Additional Comments from Nat Duca <nduca at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116982&action=review


This entire patch (rightly) deals with to impl-thread-side animations. So,
everything should have impl suffixes.

We need to nail down the animation-lifetime model. As we discussed verbally,
refptr/shared ownership is really hard to get right [when moving fast] so
ideally we'd settle on a OwnPtr system. As you suggested, lets try out "creator
of the animation owns the animation, and is responsible for adding it to the
animation controller." If you let an animation have an m_animationController
that gets set on adding to a controller, then you can automatically unregister
from the controller inside ~CCAnimation().

With that model, we have to figure out how to deal with the case where the
target dies before the animation ends. Lets make sure a unit test exists for
that case.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:33
> +class CCAnimationObserver;

Lets leave observers as a followon.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:36
> +class CCAnimation {

I think this is an AnimationChain. Lets not support animation chaining for now.
Also, I think with the right design of an Animation base class, a chain of
animations is just
CCChainedAnimation : CCAnimation {
 vector<CCAnimation> m_childAnimations;
}

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:38
> +    enum AnimatableProperty {

We can push this out to a binding class. I'd prefer something like
CCTransformAnimationTarget {
  virtual void setTransform(Transform&) = 0;
}
For things like layers that might have multiple target types, you could make an
adapter
CCLayerImplAnimationTargetAdapter : CCTransformAnimationTarget, ... {
   AnimatableProperty m_animatableProperty;
   CCLayerImpl* m_layer;
   void setTransform(...) { call the appropriate m_layer setter method based on
m_property enum);
}

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationElement.h:40
> +class CCAnimationElement {

This class feels more to me like CCAnimation, with subclasses for different
kinds of animations. CCKeyframedTransformAnimation,
CCZooomAnimatorTransformation, etc.

However:
- The comments before about pulling out the 'binding' of the animation to a
CCAnimationTarget method would be good
- Do we really need a reset() method?
- We should decide if we animate() needs to support time moving backward, or if
we assume that time will always increase. We use the monotonic clock for
animation which guarantees that time moves forward, but its probably better to
either explicitly require this OR say it must support random time accesses. 
- Where does the start time of an animation get obtained? Ideally, we dont have
to wait a full frame to call onStarted. Afaict, the right way to do this is to
have AnimationController maintain a list of new animations --- then, whenever
an animation is started, post a task (no delay) that starts all the animations.


> Source/WebCore/platform/graphics/chromium/cc/CCAnimationElement.h:50
> +    void progress(double t, CCLayerImpl*);

animate

lets forbid animations from having direct pointers to the thing they're
animating.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationElement.h:66
> +    bool m_firstFrame;

Ideally, not a property of the animation. Although, I could imagine an
animation has a state ("starting", "running") as jamesr pointed out.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationElement.h:72
> +    double m_duration;

Do animations have to specify their duration? What do we get from knowing the
duration?

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationElement.h:75
> +    OwnPtr<CCResampler> m_resampler;

Lets hold this feature. Also, its probably a property of a CCKeyframedAnimation
subclass.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationObserver.h:32
> +class CCAnimationObserver {

Good stuff, but lets not do it right away.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimator.h:37
> +class CCAnimator {

This feels like an animation manager. We talked about animation controller
earlier, but on reflection, we are using Manager eg TextureManager, for
"compositor-wide" classses.

I'd like for each CCLayerImpls not to have an animation manager. Rather,
CCLayerTreeHostImpl should have an animation manager --- this way we dont have
to walk the universe to figure out what to animate. There are other wins here
later.

There are good things that this does regarding
combining/merging/canceling/preempting animations that are already in progress.
I think those should probably be another class and feature, either just part of
a CCChainedAnimation class or something new.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimator.h:86
> +    struct RunningAnimation {

I find this concept very intriguing. This begs the notion that an animation
itself mostly stateless? Or is it stateful and startTime is special? This gets
back to whether we should do
a) void CCAnimation::animate(float t/*awaysincreasing*/)
b) float CCAnimation::getValue(float t /*no guarantee on t*/) const

I have to say, I find (b) tempting. The mutable keyword could always be used
inside (b) if we got worried about the cost of looking up keyframe pairs inside
a keyframed animation impl.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:89
> +    //

I am a huuuge fan of walking over a vector and not having any smartness.