Bug 73233 – [chromium] Create a base-class CCAnimation to represent compositor animations

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Nat Duca]

We need a base class that can be used as a basis for impl-thread animations.

[Nat Duca]

Just as a strawman to start discussion:
1. Lets keep mutation separate from computation. This allows unit testing.
2. Lets allow keyframed and nonkeyframed


CCAnimationTarget {
  void setValue(float value);
}

CCAnimation {
  // returns false if the animation has ended
  virtual bool getValue(double timeStamp);
  CCAnimationTarget* target;
}

CCLayerTreeHostImpl {
  vector<CCAnimation> m_animations;
}

[James Robinson]

(In reply to comment #1)
> Just as a strawman to start discussion:
> 1. Lets keep mutation separate from computation. This allows unit testing.
> 2. Lets allow keyframed and nonkeyframed
> 
> 
> CCAnimationTarget {
>   void setValue(float value);
> }
> 
> CCAnimation {
>   // returns false if the animation has ended
>   virtual bool getValue(double timeStamp);
>   CCAnimationTarget* target;
> }

How do you get at the actual value? Does this trigger a setValue() call?

What about animated things that aren't floats - do we templatize the target?

I think an Animation also has state (active, running, paused, waiting for start is the set of states that page/animation/AnimationBase seems to use).

> 
> CCLayerTreeHostImpl {
>   vector<CCAnimation> m_animations;
> }

Created attachment 116982 [details]
Patch

This is a very rough idea for how compositor animations could eventually look. tl;dr 
 - CCLayerImpl's may optionally have animators.
 - Animators maintain (a) animation queue (b) running animations
 - Animations are made up of animation elements
    o elements are more general than keyframes (basically implements progress(t, layer), where t \in [0, 1])
    o keyframes can be implemented in terms of elements.
    o elements allow for parametric animation. Will be a simpler way to represent a fling.

[Nat Duca]

I'd really like to se something simpler than this patch.

[WebKit Review Bot]

Comment on attachment 116982 [details]
Patch

Attachment 116982 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10687021

[Nat Duca]

Comment on attachment 116982 [details]
Patch

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.

Created attachment 117480 [details]
Patch

Created attachment 117481 [details]
Patch

Created attachment 117486 [details]
Patch

(In reply to comment #7)
> (From update of attachment 116982 [details])
> 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.

Fixed.

> 
> 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.

We should talk about this again. I considered an OwnPtr method and having various parties (layer, creator, manager) actually own the pointers. None of them seemed great to me, but I may be missing something. I went with a RefPtr model so that creators can hang onto their animations if they want to do things like check them to see if they're still animating. As you mentioned it is still crucial that CCLayerImpls let the manager know to discard animations.
> 
> > 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;
> }
> 
Yep. Gone.

> > 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);
> }
> 
Have a new idea for this one based on signature. See CCFloatAnimationTargetImpl::signature().

> > 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.
> 
The point of reset was to allow animations to flush any state they may have if they're cyclic (they may, for example, cache the current state of the target when they begin animating). If we pass animations the absolute time, then this is unnecessary. I think this is a good approach.

> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationElement.h:50
> > +    void progress(double t, CCLayerImpl*);
> 
> animate
Done.
> 
> lets forbid animations from having direct pointers to the thing they're animating.

This is now done through an animation target as we discussed.
> 
> > 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.
> 
Fixed.

> > 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?
I thought this would be handy for animation chains in the future. Might be nice if an animation chain knew how long it was (for cycling, etc).
> 
> > 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.
Removed (though the class is still in the patch)

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationObserver.h:32
> > +class CCAnimationObserver {
> 
> Good stuff, but lets not do it right away.
Removed (though the class is still in the patch)
> 
> > 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.
Fixed.
> 
> 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.
> 
Done.
> > 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.
> 
Things became much simpler when I had animations be a bit more stateful and keep track of things like their start time and state (paused, running, etc).

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:89
> > +    //
> 
> I am a huuuge fan of walking over a vector and not having any smartness.

Fixed.

[WebKit Review Bot]

Comment on attachment 117486 [details]
Patch

Attachment 117486 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10694736

Created attachment 117652 [details]
Patch

Created attachment 119480 [details]
Patch

[Nat Duca]

Comment on attachment 119480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119480&action=review

Great work! I'd like to see the CCLayerTreeHostImpl::m_activeAnimations code as well so we can troubleshoot it.

Wk style prefers foo()/setFoo() rather than getFoo()/setFoo().

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

Add a type system here in the style of the CCDrawQuad. E.g. 
   enum Type { FloatAnimation, TransformAnimation };
   Type getType() const = 0;

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:37
> +    virtual bool hasDuration() const = 0;

Why do we support this? I thought we agreed we'd not support procedural animations. The goal is to be laser focused on CSS!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:48
> +// O(n^2) in the number of animations in the queue.

Not sure why so much commentary on the runtime complexity of these. I'd delete these comments. The function names should be self explanatory (update them if they're not) and anything that doesn't make sense in the function name should be a comment at the beginning of the function. E.g resolveConflicts might want a bit of explanation IN the function, but not here.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:49
> +void CCLayerAnimationControllerImpl::animate(double frameBeginTimeMs)

Should we use seconds in the animation system? I know I started the rest of the code using ms, but I think thats a huge mistake on my part.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:72
> +    m_lastTickTime = frameBeginTimeMs;

why are we tracking lastTickTime?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:80
> +void CCLayerAnimationControllerImpl::abort(ID id)

Not needed. Just make setState public. Minimalism!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:85
> +void CCLayerAnimationControllerImpl::pause(ID id)

Not needed.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:90
> +void CCLayerAnimationControllerImpl::resume(ID id)

Same.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:95
> +bool CCLayerAnimationControllerImpl::isAnimating() const

I think that we should probably be more precise than animating... that word is massively overloaded... isActive might be better, and explain on the .h  file what that means.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:100
> +double CCLayerAnimationControllerImpl::ControlledAnimation::duration() const

Is the idea here that when an animation loops we bump the animation time and this causes its apparent duration to go up?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:107
> +    return m_animation->hasDuration() && m_state.iterations > 0;

the iterations field is a bit ambiguous it seems.... as named, its not clear if it starts at zero or 1.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:112
> +{
> +    // TODO(vollick): eventually we will want to notify observers here.

WK style prefers // FIXME: foo without a name

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:132
> +{

Are these functions needed? It seems they have only one caller. Can we just fold the code into there?

Also, probably want to add lots of defensive asserts to wherever this lands to avoid casting errors.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:42
> +    typedef int ID;

Copy the style for IDs in LayerChromium/CCLayerImpl --- on the impl side, CCLayerImpls are constructed with an ID. In this case, I think its the State struct...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:58
> +    enum Property {

Seems to me that until we can avoid this for now if we have CCAnimation::Type. I know its not as general, but since we have only two targets, that will suffice for now. Dont you think?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImplClient.h:32
> +#define CCLayerAnimationControllerImplClient_h

Typically we put clients with their class. E.g in the main animationcontrollerimpl.h

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImplClient.h:48
> +    virtual const IntSize& renderBoxBorderSize() const = 0;

We need to figrue out what that is and map it to whatever CCLayerImpl understands it to be... if you need help figuring that out, hit me up on chat.

Created attachment 120177 [details]
Patch

(In reply to comment #15)
> (From update of attachment 119480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119480&action=review
> 
> Great work! I'd like to see the CCLayerTreeHostImpl::m_activeAnimations code as well so we can troubleshoot it.
> 

The controller's client (CCLayerImpl) doesn't seem to have access to the CCLayerTreeHostImpl (I may be missing it though). It seems like there are two options -- (1) give all the CCLayerImpl's back references to the LTHI so they can add/remove anim controllers from active list themselves, or (2) the main thread can manage the active list when animations are added and layers are deleted. Since it might take a bit to sort this out, I was hoping put this off until the next CL if you're alright with it.

> Wk style prefers foo()/setFoo() rather than getFoo()/setFoo().

Noted. I've used foo()/setFoo() for trivial getters and setters, but CCAnimation still has getFloat and getTransformOperations since they're not simple getters. Is that ok?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:32
> > +class CCAnimation {
> 
> Add a type system here in the style of the CCDrawQuad. E.g. 
>    enum Type { FloatAnimation, TransformAnimation };
>    Type getType() const = 0;
> 
Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:37
> > +    virtual bool hasDuration() const = 0;
> 
> Why do we support this? I thought we agreed we'd not support procedural animations. The goal is to be laser focused on CSS!

Not having a duration has a big effect on looping, scheduling and blending, and I thought that if we added support for duration-less animations right from the beginning, when new features are added, they would be designed with this in mind. Otherwise, it might require major surgery to support animations without a duration down the road.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:48
> > +// O(n^2) in the number of animations in the queue.
> 
> Not sure why so much commentary on the runtime complexity of these. I'd delete these comments. The function names should be self explanatory (update them if they're not) and anything that doesn't make sense in the function name should be a comment at the beginning of the function. E.g resolveConflicts might want a bit of explanation IN the function, but not here.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:49
> > +void CCLayerAnimationControllerImpl::animate(double frameBeginTimeMs)
> 
> Should we use seconds in the animation system? I know I started the rest of the code using ms, but I think thats a huge mistake on my part.
Switched to seconds.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:72
> > +    m_lastTickTime = frameBeginTimeMs;
> 
> why are we tracking lastTickTime?
It's used in pause/resume.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:80
> > +void CCLayerAnimationControllerImpl::abort(ID id)
> 
> Not needed. Just make setState public. Minimalism!
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:85
> > +void CCLayerAnimationControllerImpl::pause(ID id)
> 
> Not needed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:90
> > +void CCLayerAnimationControllerImpl::resume(ID id)
> 
> Same.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:95
> > +bool CCLayerAnimationControllerImpl::isAnimating() const
> 
> I think that we should probably be more precise than animating... that word is massively overloaded... isActive might be better, and explain on the .h  file what that means.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:100
> > +double CCLayerAnimationControllerImpl::ControlledAnimation::duration() const
> 
> Is the idea here that when an animation loops we bump the animation time and this causes its apparent duration to go up?
> 
Yes. This value in combination with the start time lets the controller know when the animation is finished.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:107
> > +    return m_animation->hasDuration() && m_state.iterations > 0;
> 
> the iterations field is a bit ambiguous it seems.... as named, its not clear if it starts at zero or 1.

I'm following the lead of WebCore/platform/animation/Animation.h. Actually, iterationCount was used in that case  -- would that be better?

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:112
> > +{
> > +    // TODO(vollick): eventually we will want to notify observers here.
> 
> WK style prefers // FIXME: foo without a name
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:132
> > +{
> 
> Are these functions needed? It seems they have only one caller. Can we just fold the code into there?
I was thinking ahead to when multiple float or transform properties were animatable. The inlined code would need to be refactored when that happened anyhow, so it made sense to do it now. Also, it makes the switch statement a little cleaner.
> 
> Also, probably want to add lots of defensive asserts to wherever this lands to avoid casting errors.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:42
> > +    typedef int ID;
> 
> Copy the style for IDs in LayerChromium/CCLayerImpl --- on the impl side, CCLayerImpls are constructed with an ID. In this case, I think its the State struct...
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:58
> > +    enum Property {
> 
> Seems to me that until we can avoid this for now if we have CCAnimation::Type. I know its not as general, but since we have only two targets, that will suffice for now. Dont you think?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImplClient.h:32
> > +#define CCLayerAnimationControllerImplClient_h
> 
> Typically we put clients with their class. E.g in the main animationcontrollerimpl.h
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImplClient.h:48
> > +    virtual const IntSize& renderBoxBorderSize() const = 0;
> 
> We need to figrue out what that is and map it to whatever CCLayerImpl understands it to be... if you need help figuring that out, hit me up on chat.
Removed. After doing some digging, it looks like CCLayerImpl does not have access to that information (which, I think, is why it was plumbed down into GraphicsLayer::addAnimation). What I think should ultimately happen is this: after a call to addAnimation, the main thread's controller will be told to start up an animation. At that point, it will have the size handy and can just replace the transforms that operate in terms of percentages with equivalent transforms that operate in terms of absolute pixel distances. The cc animation framework should never need to know about this detail.

[WebKit Review Bot]

Attachment 120177 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   5282799..4428c79  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 103439 = 5282799a1fb6729d1acbccc6f581466f9694b675
r103445 = 4428c79e3922c44a4537acf0b21a4e4540a3eecb
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167249 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

Comment on attachment 120177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120177&action=review

I think you're working too hard at keeping funcitonality here protected/private. Please keep in mind, all of this stuff lives on the impl thread, the only people that can get at these data structures is the compositor. As a result, adding protected-to-prevent-ill-goings-on is only necessary when there is real danger.

Similarly, I still see that you like to add helper functions ("it makes the function cleaner/etc"). Again, I think that's not the right approach here. The problem with these is that a new person to the code has to understand these helper functions as well -- and those helper functions themselves might have side effects. For example, your AnimationControllerImpl::add method really just constructs an ActiveAnimation --- but I don't know that by reading the call site -- I have to instead go look at the ::add method to see that its just a wrapper around a constructor. As a similar example, AnimationControllerImpl::setState just wrapps ActiveAnimation::SetState BUT has the side effect of purging deleted animations. This might be nice from a user-facing API, but from a backend point of view, it just makes it hard to reason about side effects.

> Source/WebCore/ChangeLog:8
> +        Unit tests for the controller are in CCLayerAnimationControllerImplTest.cpp

You can probably just delete this line.

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

Comment this class explaining its role.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:39
> +    virtual bool hasDuration() const = 0;

We've gone back and forth on this far too much. The simple fact of the matter is that CSS doesn't require this. The main use cases of this system are css and chrome/aura. Neither of those need implicit animations.

So, Decision Time: we will not explicitly not support for duration-less animations. We do not have enough use cases for them that warrant the investment. SG?

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:44
> +public:

We try to group base-class overrides together with a comment indicating which are overrides. See the style in CCThreadProxy for example.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:47
> +    virtual bool getFloat(double t, float* value) = 0;

getValue?

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:55
> +    virtual bool getTransformOperations(double t, TransformOperations* value) = 0;

getValue?

> Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:65
> +inline CCTransformAnimation* toTransformAnimation(CCAnimation* object)

Please copy the approach Enne uses in CCDrawQuad --- the toXXX are member functions on the base class.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:39
> +{ }

This style is a bit funky. I think we tend to expand fully.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:41
> +// static

We don't typically annotate static methods

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:66
> +void CCLayerAnimationControllerImpl::add(PassOwnPtr<CCAnimation> anim, const AnimationState& state)

Again, this feels like sugar.

I'd prefer we just have addActiveAnimation(PassOwnPtr<CCLayerAnimationControllerImpl::ActiveAnimation>)

That makes it extremely clear that the add method isn't doing anything else.

As written, I have to go look at the ::add method to see if its doing anything on the base class.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:156
> +            m_animations[i]->setRunState(state, m_lastTickTime);

I'd prefer we not have this m_lastTickTime field. Can we structure this so that anyone who needs the "current time" gets it passed in?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:159
> +    purgeFinishedAnimations();

Can't this be held until the animation tick? This seems like "GC" and if we're setting the state on 10 animations, it seems like overkill to go looking for finished animations at this point.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:71
> +    enum Property {

TargetProperty?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:78
> +    struct AnimationState {

Why can't this stuff just be on the ActiveAnimation class? Either I've forgotten something over the holiday, or it seems to me that this is unnecessary. All this code is inaccessible to users of our API, remember!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:96
> +    void abort(ID id) { setState(id, Aborted); }

I really dont feel that this sugar is necessary. Why not have a method called
  getActivateANimation(ID id)

You can put pause/abort/resume on the activeanimation.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:128
> +        bool getFloat(double t, float* value);

Sorry, I don't buy it. Remove these. Do the conversion inline. If and only if we have multiple places doing this conversion should we add this marshalling code.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:153
> +    void setState(ID, RunState);

If you persist this method [which I dont think you need to do?], try setRunState for consistency.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:163
> +    Vector<OwnPtr<ActiveAnimation> > m_animations;

Any reason this is a vector and not a hashtable/map/something? Its indexed by ID...

> Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:41
> +class DummyControllerClient : public CCLayerAnimationControllerImplClient {

I think we've setttled on the word Fake, fwiw.

> Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:50
> +    virtual const IntSize& renderBoxBorderSize() const { return m_size; }

no longer needed?

> Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:118
> +

Probably want test of didActivate.... you've got all the plumbing for it, it seems, just not the tests.

> Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:119
> +void addAnimation(CCLayerAnimationControllerImpl* controller, PassOwnPtr<CCAnimation> animation, int id, CCLayerAnimationControllerImpl::Property property, CCLayerAnimationControllerImpl::RunState runState, double when, int iterations)

I think this makes the case for CCLayerAnimationControllerImpl::ActiveAnimation to be directly constructable by end code and directly addable to the AnimationControllerImpl.

Also, what do you think about making take ActiveAnimation and make it a toplevel class, e.g. CCActiveAnimation? The RunState enum might become an internal member of CCActiveAnimation instead of being on the animationcontroller.... Since the activeanimation class has some logic on its own, I almost think it should be its own file and have its own tests independent of the controller...

> Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:124
> +// Transition opacity from 0 to 1

Comments are complete sentences.

> Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:182
> +    addAnimation(controller.get(),

Though its more typing, I think this would read better if you made ActiveAnimations start up in a fixed state and reasonable defaults, then had to set their state explicitly as followup methods. Decoding what the positional arguments mean here is pretty hard as a noob to the code.

Created attachment 121290 [details]
Patch

(In reply to comment #19)
> (From update of attachment 120177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120177&action=review
> 
> I think you're working too hard at keeping funcitonality here protected/private. Please keep in mind, all of this stuff lives on the impl thread, the only people that can get at these data structures is the compositor. As a result, adding protected-to-prevent-ill-goings-on is only necessary when there is real danger.
> 
> Similarly, I still see that you like to add helper functions ("it makes the function cleaner/etc"). Again, I think that's not the right approach here. The problem with these is that a new person to the code has to understand these helper functions as well -- and those helper functions themselves might have side effects. For example, your AnimationControllerImpl::add method really just constructs an ActiveAnimation --- but I don't know that by reading the call site -- I have to instead go look at the ::add method to see that its just a wrapper around a constructor. As a similar example, AnimationControllerImpl::setState just wrapps ActiveAnimation::SetState BUT has the side effect of purging deleted animations. This might be nice from a user-facing API, but from a backend point of view, it just makes it hard to reason about side effects.
> 
> > Source/WebCore/ChangeLog:8
> > +        Unit tests for the controller are in CCLayerAnimationControllerImplTest.cpp
> 
> You can probably just delete this line.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:32
> > +class CCAnimation {
> 
> Comment this class explaining its role.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:39
> > +    virtual bool hasDuration() const = 0;
> 
> We've gone back and forth on this far too much. The simple fact of the matter is that CSS doesn't require this. The main use cases of this system are css and chrome/aura. Neither of those need implicit animations.
> 
> So, Decision Time: we will not explicitly not support for duration-less animations. We do not have enough use cases for them that warrant the investment. SG?
Removed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:44
> > +public:
> 
> We try to group base-class overrides together with a comment indicating which are overrides. See the style in CCThreadProxy for example.
Fixed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:47
> > +    virtual bool getFloat(double t, float* value) = 0;
> 
> getValue?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:55
> > +    virtual bool getTransformOperations(double t, TransformOperations* value) = 0;
> 
> getValue?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimation.h:65
> > +inline CCTransformAnimation* toTransformAnimation(CCAnimation* object)
> 
> Please copy the approach Enne uses in CCDrawQuad --- the toXXX are member functions on the base class.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:39
> > +{ }
> 
> This style is a bit funky. I think we tend to expand fully.
Fixed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:41
> > +// static
> 
> We don't typically annotate static methods
Removed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:66
> > +void CCLayerAnimationControllerImpl::add(PassOwnPtr<CCAnimation> anim, const AnimationState& state)
> 
> Again, this feels like sugar.
> 
> I'd prefer we just have addActiveAnimation(PassOwnPtr<CCLayerAnimationControllerImpl::ActiveAnimation>)
> 
> That makes it extremely clear that the add method isn't doing anything else.
> 
> As written, I have to go look at the ::add method to see if its doing anything on the base class.
Done. It does have the side effect of activating the controller as you requsted, though.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:156
> > +            m_animations[i]->setRunState(state, m_lastTickTime);
> 
> I'd prefer we not have this m_lastTickTime field. Can we structure this so that anyone who needs the "current time" gets it passed in?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:159
> > +    purgeFinishedAnimations();
> 
> Can't this be held until the animation tick? This seems like "GC" and if we're setting the state on 10 animations, it seems like overkill to go looking for finished animations at this point.
Removed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:71
> > +    enum Property {
> 
> TargetProperty?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:78
> > +    struct AnimationState {
> 
> Why can't this stuff just be on the ActiveAnimation class? Either I've forgotten something over the holiday, or it seems to me that this is unnecessary. All this code is inaccessible to users of our API, remember!
Removed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:96
> > +    void abort(ID id) { setState(id, Aborted); }
> 
> I really dont feel that this sugar is necessary. Why not have a method called
>   getActivateANimation(ID id)

The reason for not adding this method is that multiple animations may have the same ID. This is by design. Groups of animations with the same ID are run as a block (that is, they're started at the same time, and queued animations have to wait for the whole block to finish before they start). This is how queued animations can be synchronized with no skew.

Given the way that ID's are used, ID may be a poor name. Maybe 'sequence number' would be more appropriate. What do you think?

> 
> You can put pause/abort/resume on the activeanimation.
> 
Since we are exposing the RunState anyhow (consumers can now create CCActiveAnimations) it makes no sense to prevent the sole consumer (the main thread's compositor) from setting the active animations' state directly. I've replaced these three functions with setRunState as you had suggested in an earlier review.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:128
> > +        bool getFloat(double t, float* value);
> 
> Sorry, I don't buy it. Remove these. Do the conversion inline. If and only if we have multiple places doing this conversion should we add this marshalling code.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:153
> > +    void setState(ID, RunState);
> 
> If you persist this method [which I dont think you need to do?], try setRunState for consistency.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:163
> > +    Vector<OwnPtr<ActiveAnimation> > m_animations;
> 
> Any reason this is a vector and not a hashtable/map/something? Its indexed by ID...

It's because multiple animations may have the same ID (see note above). I think that the typical use case is to schedule a couple of animations to run together (scale down and fade out, for example). These two animations will have the same id. It's unlikely that there are other animations in the queue (I think that queuing/scheduling is a rare case), so you'd end up with an associative container where all the items have the same key. Given that the container would usually be in this degenerate state, and given that the number of animations in flight for a single layer will be very small, a Vector seemed like the right choice.

> 
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:41
> > +class DummyControllerClient : public CCLayerAnimationControllerImplClient {
> 
> I think we've setttled on the word Fake, fwiw.
Fixed
> 
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:50
> > +    virtual const IntSize& renderBoxBorderSize() const { return m_size; }
> 
> no longer needed?
Removed.
> 
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:118
> > +
> 
> Probably want test of didActivate.... you've got all the plumbing for it, it seems, just not the tests.
Added.
> 
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:119
> > +void addAnimation(CCLayerAnimationControllerImpl* controller, PassOwnPtr<CCAnimation> animation, int id, CCLayerAnimationControllerImpl::Property property, CCLayerAnimationControllerImpl::RunState runState, double when, int iterations)
> 
> I think this makes the case for CCLayerAnimationControllerImpl::ActiveAnimation to be directly constructable by end code and directly addable to the AnimationControllerImpl.
> 
> Also, what do you think about making take ActiveAnimation and make it a toplevel class, e.g. CCActiveAnimation? The RunState enum might become an internal member of CCActiveAnimation instead of being on the animationcontroller.... Since the activeanimation class has some logic on its own, I almost think it should be its own file and have its own tests independent of the controller...
Done.
> 
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:124
> > +// Transition opacity from 0 to 1
> 
> Comments are complete sentences.
Fixed.
> 
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:182
> > +    addAnimation(controller.get(),
> 
> Though its more typing, I think this would read better if you made ActiveAnimations start up in a fixed state and reasonable defaults, then had to set their state explicitly as followup methods. Decoding what the positional arguments mean here is pretty hard as a noob to the code.

Fixed.

[James Robinson]

Comment on attachment 121290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121290&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:41
> +    , m_startTime(0.0)

don't suffix these with ".0". see http://www.webkit.org/coding/coding-style.html "Floating point literals"

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:47
> +void CCActiveAnimation::setRunState(RunState runState, double t)

here and elsewhere: we don't normally use abbreviations and never use one-letter variable names in WebKit except for 'i' and 'j' as loop counters, so this needs to be an actual word. See http://www.webkit.org/coding/coding-style.html "Names"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:72
> +    CCLayerAnimationControllerImpl(CCLayerAnimationControllerImplClient*);

please add the 'explicit' keyword to all 1-arg constructors, unless you truly do want implicit conversion behavior (which is rarely the case)

[Nat Duca]

Comment on attachment 121290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121290&action=review

Any sense in calling CCAnimation a CCAnimationCurve? NBD if thats a pain to rename...

I definitely think making IDs unique and then dealing with intra-property chaining using vector<ID> m_animationsThatMustBeFinishedBeforeWeRun; // or something like that

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:48
> +{

give t in this function a better name? is it "now"?

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:49
> +    // FIXME: eventually we will want to notify observers here.

This comment probably should get snipped, dont think it adds value.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:50
> +    switch (runState) {

this switch can probably be written as a few if statements for special cases...

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:70
> +    if (runState() == Finished || runState() == Aborted)

is there a reason you're using runState() vs m_runState?

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:85
> +    // all time spent paused

End sentence with .

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:107
> +

should this file have unit tests?

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:42
> +    enum RunState {

Might put a basic explanation of the state transition graph here.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:33
> +namespace {

You dont need to namespace the typedef, I think?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:35
> +}

Bit puzzled by this type name

> Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:411
> +// Tests that adding an animation to the controller calls the appropriate callback on the controller client

Is this sufficient coverage?

Does aborting one animation have any vulnerability points?

[WebKit Review Bot]

Comment on attachment 121290 [details]
Patch

Attachment 121290 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11145129

Created attachment 121735 [details]
Patch

(In reply to comment #23)
> (From update of attachment 121290 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121290&action=review
> 
> Any sense in calling CCAnimation a CCAnimationCurve? NBD if thats a pain to rename...
Done. That's a much better name.
> 
> I definitely think making IDs unique and then dealing with intra-property chaining using vector<ID> m_animationsThatMustBeFinishedBeforeWeRun; // or something like that
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:48
> > +{
> 
> give t in this function a better name? is it "now"?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:49
> > +    // FIXME: eventually we will want to notify observers here.
> 
> This comment probably should get snipped, dont think it adds value.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:50
> > +    switch (runState) {
> 
> this switch can probably be written as a few if statements for special cases...
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:70
> > +    if (runState() == Finished || runState() == Aborted)
> 
> is there a reason you're using runState() vs m_runState?
Nope. Switched to m_runState.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:85
> > +    // all time spent paused
> 
> End sentence with .
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:107
> > +
> 
> should this file have unit tests?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:42
> > +    enum RunState {
> 
> Might put a basic explanation of the state transition graph here.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:33
> > +namespace {
> 
> You dont need to namespace the typedef, I think?
Removed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:35
> > +}
> 
> Bit puzzled by this type name
Added a comment.
> 
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerImplTest.cpp:411
> > +// Tests that adding an animation to the controller calls the appropriate callback on the controller client
> 
> Is this sufficient coverage?
> 
> Does aborting one animation have any vulnerability points?
There's a concern when one of the animations is in a group is aborted, scheduling will proceed correctly. Added another test for this case.

[Nat Duca]

Comment on attachment 121735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121735&action=review

Nearly LGTM. Nits and setGroupID/ GroupID typedef

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:62
> +bool CCActiveAnimation::isFinishedAt(double now) const

now->time?

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:35
> +// A CCActiveAnimation, contains all the state required to play a CCAnimation.

CCAnimation referenc

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationCurve.h:34
> +// An animation is essentially a function that returns a value given a time.

Remove "essentially" :)

An animation -> An animation curve

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationCurve.h:35
> +// There are currently only two types of animation, float and transform.

types of animation -> types of curves

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationCurve.h:53
> +    // Returns true if the animation is finished.

Do we need to return bool now that we know duration?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:107
> +        // if it's enqueued

Lets steer clear of "Enqueued" as a word.

Also, complete setnence comments or none at all. Probably this comment isn't very useful?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:109
> +            // collect all properties for animations with the same id (they should all also be enqueued)

collect -> Collect

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:121
> +            // if there's nothing in common, start them up.

if -> If

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:159
> +    // each iteration, m_activeAnimations.size() decreases or i increments

Capitalize

Created attachment 121870 [details]
Patch

(In reply to comment #27)
> (From update of attachment 121735 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121735&action=review
> 
> Nearly LGTM. Nits and setGroupID/ GroupID typedef
GroupID added.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:62
> > +bool CCActiveAnimation::isFinishedAt(double now) const
> 
> now->time?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:35
> > +// A CCActiveAnimation, contains all the state required to play a CCAnimation.
> 
> CCAnimation referenc
Switched to CCAnimationCurve
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationCurve.h:34
> > +// An animation is essentially a function that returns a value given a time.
> 
> Remove "essentially" :)
Removed.
> 
> An animation -> An animation curve
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationCurve.h:35
> > +// There are currently only two types of animation, float and transform.
> 
> types of animation -> types of curves
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationCurve.h:53
> > +    // Returns true if the animation is finished.
> 
> Do we need to return bool now that we know duration?
Nope. The getValue functions now return the value.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:107
> > +        // if it's enqueued
> 
> Lets steer clear of "Enqueued" as a word.
Removed.
> 
> Also, complete setnence comments or none at all. Probably this comment isn't very useful?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:109
> > +            // collect all properties for animations with the same id (they should all also be enqueued)
> 
> collect -> Collect
Capitalized.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:121
> > +            // if there's nothing in common, start them up.
> 
> if -> If
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:159
> > +    // each iteration, m_activeAnimations.size() decreases or i increments
> 
> Capitalize
Done.

[Nat Duca]

Comment on attachment 121870 [details]
Patch

LGTM. @jamesr, your turn.

[Kenneth Russell]

Comment on attachment 121870 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121870&action=review

rs=me based on nduca's review. Nice tests.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:67
> +    for (size_t i = 0; i < m_activeAnimations.size(); ++i)

Note: it's OK to use braces for a for loop like this, since its body is more than one line long.

Created attachment 122977 [details]
Patch

(In reply to comment #31)
> (From update of attachment 121870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121870&action=review
> 
> rs=me based on nduca's review. Nice tests.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:67
> > +    for (size_t i = 0; i < m_activeAnimations.size(); ++i)
> 
> Note: it's OK to use braces for a for loop like this, since its body is more than one line long.

I've rebased and updated the braces as you mentioned in the latest patch -- it looks a lot nicer.

Created attachment 122985 [details]
Patch

Another attempt at rebasing

[WebKit Review Bot]

Comment on attachment 122985 [details]
Patch

Rejecting attachment 122985 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11211808

[Kenneth Russell]

Comment on attachment 122985 [details]
Patch

rs=me again

[WebKit Review Bot]

Comment on attachment 122985 [details]
Patch

Clearing flags on attachment: 122985

Committed r105377: <http://trac.webkit.org/changeset/105377>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.