Explicit animation support

Explicit animation support

High level description:
 - LayerPropertySetter is now LayerAnimator since it manages implicit/explicit animations and the animation queue.
 - LayerAnimationElement represents an animation curve.
 - LayerAnimationSequence owns a collection of elements.
 - The animator works as follows:
   o Has a queue of sequences and a collection of running sequences.
   o It knows the start time of each running sequence.
   o While there are running sequences, LayerAnimator::Step(base::TimeTicks now) is called periodically, and each of the running sequences are updated.

BUG=None
TEST=compositor_unittests, base_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106768

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106915

[sky, 2011-10-14 16:39:51]

Much improved. The only thing I would like to see changed are using ref counted
everywhere. Doing so makes the ownership a little nebulous. Don't get me wrong,
ref counted is useful in some situations but I don't think this is one of them.

I didn't do an in depth review, mostly just getting a high level feel for what's
there and offering a couple of suggestions. You'll need to add tests too.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer.h#ne...
ui/gfx/compositor/layer.h:86: LayerAnimator& GetAnimator();
LayerAnimation*

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animation_element.cc (right):

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.cc:32: virtual void Progress(double t,
LayerAnimationDelegate* delegate) {
Use OVERRIDE on all these.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animation_element.h (right):

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.h:26: class LayerAnimationElement :
public base::RefCounted<LayerAnimationElement> {
Can we make the ownership model trivial and have the animator own these?

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.h:37: const Transform& transform,
base::TimeDelta duration);
wrap each param.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.h:47: // next. If the animation is not
aborted, it is guaranteed that animate will
animate -> progress

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.h:49: virtual void Progress(double t,
LayerAnimationDelegate* delegate) = 0;
I think this should be time based, otherwise we could never support infinite
animations.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.h:55: virtual const
AnimatableProperties& Properties() const = 0;
Can you think of a case where the properties or duration need to change after
constructed? It seems better to make these concrete with the values provided to
the constructor.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animation_preemption_strategy.h (right):

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_preemption_strategy.h:16: class
COMPOSITOR_EXPORT LayerAnimationPreemptionStrategy {
This seems better served by an enum. Is there a reason we need this level of
flexibility?

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animation_sequence.h (right):

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_sequence.h:19: // Contains a collection of
layer animation elements. Although it has a similar
Clarify this runs the elements one after another.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_sequence.h:21: // it is not permitted to have
a sequence in a sequence).
Also document who owns these

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_sequence.h:25: class LayerAnimationSequence :
public base::RefCounted<LayerAnimationSequence> {
same comment about refcounted here.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_sequence.h:30:
LayerAnimationSequence(LayerAnimationElement* element);
explicit

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_sequence.h:57: // Returns true, if the
sequences share a common property
nit: end with '.'.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_sequence.h:71: base::TimeDelta last_start_;
I think you want timeticks for this sort of stuff.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animator.cc (right):

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.cc:21: // TODO(vollick) match kennedy theme.
?

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.cc:25: class LayerAnimation : public Animation
{
I think you'll be better off implementating AnimationContainerElement. Less
baggage.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.cc:262: RunningAnimation(sequence,
base::TimeTicks::Now()));
The expectations of multiple calls to start an animation during the same event
are that they all start at the same time. Doing this results in skew.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animator.h (right):

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.h:78: void SetAnimation(LayerAnimationSequence*
animation);
The name 'Set' implies you can only have one. How about Replace.

http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.h:81: void
EnqueueAnimation(LayerAnimationSequence* animation);
How about Add.

[Ian Vollick, 2011-10-17 20:29:27]

High level changes:
 - elements and sequences are no longer ref counted.
 - Scrapped LayerAnimationPreemptionStrategy for an enum
 - Animator keeps track of the last tick time. This is used for synchronizing
the start time of multiple sequences, preventing skew.
 - SetAnimation -> StartAnimation
 - EnqueueAnimation -> ScheduleAnimation (to match SchedulePaint, etc).
 - Added ScheduleTogether for starting animations at the same time. If there are
no animations running, calling ScheduleAnimation multiple times will achieve the
same effect, but say you're in the process of animating the transform, and you
want to schedule a simultaneous animation of transform and bounds. If you just
call ScheduleAnimation twice, the bounds anim will start right away (since the
property is free) and the transform anim will be added to the queue. This
function makes sure that neither animation starts until both bounds and
transform are free.

NOTE: there are still no tests.

Direct responses below.

On 2011/10/14 16:39:51, sky wrote:
> Much improved. The only thing I would like to see changed are using ref
counted
> everywhere. Doing so makes the ownership a little nebulous. Don't get me
wrong,
> ref counted is useful in some situations but I don't think this is one of
them.
> 
> I didn't do an in depth review, mostly just getting a high level feel for
what's
> there and offering a couple of suggestions. You'll need to add tests too.
> 
> http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer.h
> File ui/gfx/compositor/layer.h (right):
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer.h#ne...
> ui/gfx/compositor/layer.h:86: LayerAnimator& GetAnimator();
> LayerAnimation*
> 
Done.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animation_element.cc (right):
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_element.cc:32: virtual void Progress(double
t,
> LayerAnimationDelegate* delegate) {
> Use OVERRIDE on all these.
> 
Done.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animation_element.h (right):
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_element.h:26: class LayerAnimationElement :
> public base::RefCounted<LayerAnimationElement> {
> Can we make the ownership model trivial and have the animator own these?
> 
Done.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_element.h:37: const Transform& transform,
> base::TimeDelta duration);
> wrap each param.
> 
Done.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_element.h:47: // next. If the animation is
not
> aborted, it is guaranteed that animate will
> animate -> progress
> 
Done.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_element.h:49: virtual void Progress(double
t,
> LayerAnimationDelegate* delegate) = 0;
> I think this should be time based, otherwise we could never support infinite
> animations.
> 
I did this intentionally to emphasize that elements are finite. A change of
opacity from 0 to 1, for example. My hope was that if you wanted an infinite
animation, you would use a sequence. Sequences can certainly be infinite and
their Progress function is time based.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_element.h:55: virtual const
> AnimatableProperties& Properties() const = 0;
> Can you think of a case where the properties or duration need to change after
> constructed? It seems better to make these concrete with the values provided
to
> the constructor.
> 
No, the collection of affected properties should never change. I'm not sure what
you're suggesting, though. Should I add a constructor to LayerAnimationElement
that takes a collection of properties and make this function non-virtual?
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animation_preemption_strategy.h (right):
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_preemption_strategy.h:16: class
> COMPOSITOR_EXPORT LayerAnimationPreemptionStrategy {
> This seems better served by an enum. Is there a reason we need this level of
> flexibility?
> 
Nope, switched for an enum.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animation_sequence.h (right):
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_sequence.h:19: // Contains a collection of
> layer animation elements. Although it has a similar
> Clarify this runs the elements one after another.
> 
Done.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_sequence.h:21: // it is not permitted to
have
> a sequence in a sequence).
> Also document who owns these
> 
Done.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_sequence.h:25: class LayerAnimationSequence
:
> public base::RefCounted<LayerAnimationSequence> {
> same comment about refcounted here.
> 
Fixed.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_sequence.h:30:
> LayerAnimationSequence(LayerAnimationElement* element);
> explicit
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_sequence.h:57: // Returns true, if the
> sequences share a common property
> nit: end with '.'.
> 
Done.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_sequence.h:71: base::TimeDelta last_start_;
> I think you want timeticks for this sort of stuff.
> 
My hope was that the sequences would be ignorant of their start times and could
determine what to do based on the time elapsed rather than the absolute time.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animator.cc (right):
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animator.cc:21: // TODO(vollick) match kennedy theme.
> ?
Whoops. Removed.
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animator.cc:25: class LayerAnimation : public
Animation
> {
> I think you'll be better off implementating AnimationContainerElement. Less
> baggage.
> 
I did try this and hit several snags which convinced me to go back to
subclassing animation. Since I wanted the LayerAnimation class to be a
completely hidden implementation detail, I was holding onto a pointer to its
base class. This was Animation previously, which worked fine, but
AnimationContainerElement has a protected destructor and therefore can't be held
by scoped_ptr, etc. The idea of holding a raw pointer and deleting it in the
destructor seemed ugly, as did the other workarounds I thought of. Also, a chunk
of the baggage that comes with Animation (the container, whether it was started)
needed to be moved into the animator which made the animator a bit
uglier/complicated.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animator.cc:262: RunningAnimation(sequence,
> base::TimeTicks::Now()));
> The expectations of multiple calls to start an animation during the same event
> are that they all start at the same time. Doing this results in skew.
> 
Fixed. When an animation is started, its start time is either now (if the
animator is not stepping) or the last step time. When an animation is started, I
now call Step with the start time. This has two nice side effects:
  - It sets the last step time to be the element's start time. This will be
picked up by the next element that is started.
  - It guarantees that every sequence will be Progress'd with a TimeDelta of 0
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animator.h (right):
> 
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animator.h:78: void
SetAnimation(LayerAnimationSequence*
> animation);
> The name 'Set' implies you can only have one. How about Replace.
> 
I changed it to StartAnimation since there may not be an animation to replace.
>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animator.h:81: void
> EnqueueAnimation(LayerAnimationSequence* animation);
> How about Add.

I changed it to ScheduleAnimation since it implies that it will be run in the
future and it matches SchedulePaint and ScheduleDraw

Ian

[Ian Vollick, 2011-10-17 22:00:34]

Have added unit tests for LayerAnimationSequence and LayerAnimationElement.
LayerAnimator still requires tests.

[Ian Vollick, 2011-10-18 18:39:19]

Added LayerAnimator unit tests.

On 2011/10/17 22:00:34, vollick wrote:
> Have added unit tests for LayerAnimationSequence and LayerAnimationElement.
> LayerAnimator still requires tests.

[sky, 2011-10-18 23:04:23]

>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
>>
>> ui/gfx/compositor/layer_animation_element.h:49: virtual void
>> Progress(double
>
> t,
>>
>> LayerAnimationDelegate* delegate) = 0;
>> I think this should be time based, otherwise we could never support
>> infinite
>> animations.
>
> I did this intentionally to emphasize that elements are finite. A change of
> opacity from 0 to 1, for example. My hope was that if you wanted an infinite
> animation, you would use a sequence. Sequences can certainly be infinite and
> their Progress function is time based.

That makes sense.

>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
>>
>> ui/gfx/compositor/layer_animation_element.h:55: virtual const
>> AnimatableProperties& Properties() const = 0;
>> Can you think of a case where the properties or duration need to change
>> after
>> constructed? It seems better to make these concrete with the values
>> provided
>
> to
>>
>> the constructor.
>
> No, the collection of affected properties should never change. I'm not sure
> what
> you're suggesting, though. Should I add a constructor to
> LayerAnimationElement
> that takes a collection of properties and make this function non-virtual?

Yes, along with Duration.

>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
>>
>> ui/gfx/compositor/layer_animation_sequence.h:71: base::TimeDelta
>> last_start_;
>> I think you want timeticks for this sort of stuff.
>
> My hope was that the sequences would be ignorant of their start times and
> could
> determine what to do based on the time elapsed rather than the absolute
> time.

I think you could make TimeDelta work here, but IMO it's a mismatch
with what you're representing. You're trying to represent the amount
of time this sequence has been running, which seems different than a
delta.

>
http://codereview.chromium.org/8247009/diff/2001/ui/gfx/compositor/layer_anim...
>>
>> ui/gfx/compositor/layer_animator.cc:25: class LayerAnimation : public
>
> Animation
>>
>> {
>> I think you'll be better off implementating AnimationContainerElement.
>> Less
>> baggage.
>
> I did try this and hit several snags which convinced me to go back to
> subclassing animation. Since I wanted the LayerAnimation class to be a
> completely hidden implementation detail, I was holding onto a pointer to its
> base class.

You could get this effect by making it an inner class of LayerAnimator.

More comments on the latest patch shortly.

  -Scott

[sky, 2011-10-19 00:06:33]

I didn't finish LayerAnimator. I'll take a look tomorrow.

In hopes of landing this, could you push the non-compositor changes over to a
follow on patch? 

Thanks!

  -Scott

http://codereview.chromium.org/8247009/diff/26001/base/memory/scoped_vector.h
File base/memory/scoped_vector.h (right):

http://codereview.chromium.org/8247009/diff/26001/base/memory/scoped_vector.h...
base/memory/scoped_vector.h:24: typedef typename std::vector<T*>::size_type
size_type;
We use size_t every where instead of this.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:114: if (!animator_.get()) {
no {}

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:346: DropTextures();
Why do we want to do drop all child textures here? Maybe this should be moved
into a separate patch if needed.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:82: // handles blending of animations.
Mention this takes ownership of LayerAnimator and by default a Layer does not
have a LayerAnimator.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:85: // Returns the layer's animator.
Mention this creates a LayerAnimator if null.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_element.cc (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:39: AnimatableProperties
properties_;
As mentioned, promote these to LayerAnimationElement and make them const.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:45: class TransitionBase : public
LayerAnimationElement {
Add a description of this class. Maybe it should be called
BaseLayerAnimationElement or something.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:58: bool first_frame() const {
return first_frame_; }
Add a description.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:62: OnProgress(t, delegate);
It looks like all subclasses care about OnProgress when first_frame_ is true.
Maybe you should have a separate method that this calls rather than
first_frame(). Maybe OnPrepare or something.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:128: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_element.h (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.h:25: // between 0 and 1 (0 for the
last keyframe, and one for the next).
nit: the part in () is confusing. Maybe just 0 for initial, 1 for final?

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.h:36: static LayerAnimationElement*
CreateTransformElement(
each param on its own line when you wrap like this. Also document caller owns
return value.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.h:45: virtual ~LayerAnimationElement()
{}
this should be before static methods.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.h:48: // range [0, 1] where 0
represents the last keyframe and 1 represents the
same comment as with class description.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.h:53: // Called if the animation is
not allowed to complete.
Mention this may be invoked without a call to Progress (at least I think that's
the case).

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_sequence.cc (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.cc:31:
TRACE_EVENT0("LayerAnimationSequence", "Progress");
Early out if empty?

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.cc:35: if (!is_cyclic_ &&
last_element_ >= elements_.size())
If is_cyclic is true, I think this loop is problematic. In particular lets say
there is an animation going, you close the lid on your laptop and open it up the
following day. We're going to do this loop a LOT.
Also, I'm not a fan of while(true) loops, I always get worried it's going to get
stuck and run forever.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.cc:78: while (iter !=
element->Properties().end()) {
Use a for loop and pull 76 into it.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_sequence.h (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.h:30: typedef
ScopedVector<LayerAnimationElement> Elements;
Move this to the private section.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.h:37: 
remove one of these lines.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.h:64: // Returns true, if the
sequences share a common property.
How about: returns true if this sequence has at least one element whose
properties match |other|.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.h:70:
LayerAnimationElement::AnimatableProperties properties_;
Description?

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_unittest.cc (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_unittest.cc:70:
TEST(LayerAnimationElementTest, TransformElement) {
It's better form to create _unittest for each of the files you're going to test.
We sometimes don't do that if it's too hard to test each piece in isolation, but
that isn't the case here. You should be able to write tests for
LayerAnimationSequence and LayerAnimator in isolate of the rest, so that I would
separate out the two types of tests. If you need to share classes, like your
DummyLaerAnimationDelegate, move it into a .h/cc. See ui/aura/test/test* for
examples of shared classes.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_unittest.cc:96:
TEST(LayerAnimationElementTest, BoundsElement) {
Add descriptions for each of your tests.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.cc (left):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:53: animation_->SetContainer(container);
Make all animations share the same container like the old code was doing.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.cc (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:35: double GetCurrentValue() const { return
0.0; }
virtual/OVERRIDE

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:36: virtual void Step(base::TimeTicks
time_now) { animator_->Step(time_now); }
OVERRIDE

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:39: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:43: /* static */
/* static */ -> // static

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.h (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.h:32: // We need to keep track of the start
time of every running animation.
Make these (and typedefs private).

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.h:61: virtual ~LayerAnimator();
constructors before other methods

[sky, 2011-10-19 15:43:25]

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.cc (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:85: if (!delegate_) {
remove {} Why does no delegate imply you need to clear the animations?

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:122: const
std::vector<LayerAnimationSequence*>& animations) {
nit: spacing

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:125:
std::vector<LayerAnimationSequence*>::const_iterator iter =
move iterator into for loop.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:128:
LayerAnimationElement::AnimatableProperties::const_iterator prop_iter =
Can this be animated_properties.insert((*iter)->properties().begin(), ...end())?

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:139: // are free.
free? Do you mean existing animations done?

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:155: return animation_queue_.size() > 0;
!empty(). You can inline this if you want.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:176: RunningAnimations
running_animations_copy = running_animations_;
Document why you need to make a copy.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:177: RunningAnimations::iterator iter =
running_animations_copy.begin();
move iterator into for lop.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:206: RunningAnimations::iterator iter =
running_animations_.begin();
move into for loop.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:262: RunningAnimations::iterator iter =
running_animations_.begin();
move into for loop.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:433: : base::TimeTicks::Now();
For the animating case this effectively starts the animation at 0, which isn't
always the case. Don't you need to adjust based on how far over the last
sequences duration you went?

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.h (right):

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.h:63: // Sets the transform on the Layer.
Document what these does (the 3 setters). Also, do they need to be virtual?

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.h:77: // a property is set during an animation.
This class does not own the
Remove last sentence and document what the default is.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.h:110: void Step(base::TimeTicks now);
This shouldn't be public.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.h:120: void UpdateAnimationState();
Add descriptions for these methods.

http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.h:142: // This is the queue of animations to
run
end sentences with periods.

[Ian Vollick, 2011-10-19 22:36:31]

> In hopes of landing this, could you push the non-compositor changes over to a
> follow on patch? 
> Thanks!

Done. I couldn't include my changes to layer to make the new framework live, or
I'd break the build, so I've left layer essentially as it was. There is one
noteworthy change: I've renamed the old LayerAnimator, LayerAnimationManager so
it won't conflict with the new LayerAnimator I'm adding. This is only temporary,
of course. LayerAnimationManager will be removed in the follow on patch.

>
http://codereview.chromium.org/8247009/diff/26001/base/memory/scoped_vector.h...
> base/memory/scoped_vector.h:24: typedef typename std::vector<T*>::size_type
> size_type;
> We use size_t every where instead of this.
Fixed.
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.cc#...
> ui/gfx/compositor/layer.cc:114: if (!animator_.get()) {
> no {}
Fixed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.cc#...
> ui/gfx/compositor/layer.cc:346: DropTextures();
> Why do we want to do drop all child textures here? Maybe this should be moved
> into a separate patch if needed.
Removed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.h#n...
> ui/gfx/compositor/layer.h:82: // handles blending of animations.
> Mention this takes ownership of LayerAnimator and by default a Layer does not
> have a LayerAnimator.
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer.h#n...
> ui/gfx/compositor/layer.h:85: // Returns the layer's animator.
> Mention this creates a LayerAnimator if null.
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.cc:39: AnimatableProperties
> properties_;
> As mentioned, promote these to LayerAnimationElement and make them const.
Done.
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.cc:62: OnProgress(t, delegate);
> It looks like all subclasses care about OnProgress when first_frame_ is true.
> Maybe you should have a separate method that this calls rather than
> first_frame(). Maybe OnPrepare or something.
Done (added OnStart).
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.cc:128: };
> DISALLOW_COPY_AND_ASSIGN
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.h:25: // between 0 and 1 (0 for the
> last keyframe, and one for the next).
> nit: the part in () is confusing. Maybe just 0 for initial, 1 for final?
Sounds good. Used your wording.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.h:36: static LayerAnimationElement*
> CreateTransformElement(
> each param on its own line when you wrap like this. Also document caller owns
> return value.
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.h:45: virtual
~LayerAnimationElement()
> {}
> this should be before static methods.
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.h:48: // range [0, 1] where 0
> represents the last keyframe and 1 represents the
> same comment as with class description.
Fixed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.h:53: // Called if the animation is
> not allowed to complete.
> Mention this may be invoked without a call to Progress (at least I think
that's
> the case).
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_sequence.cc:31:
> TRACE_EVENT0("LayerAnimationSequence", "Progress");
> Early out if empty?
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_sequence.cc:35: if (!is_cyclic_ &&
> last_element_ >= elements_.size())
> If is_cyclic is true, I think this loop is problematic. In particular lets say
> there is an animation going, you close the lid on your laptop and open it up
the
> following day. We're going to do this loop a LOT.
> Also, I'm not a fan of while(true) loops, I always get worried it's going to
get
> stuck and run forever.
Good point. If the delta is huge, I now advance by approximately floor(delta /
duration) to avoid looping for ages. I've added a unit test for this, too. It is
no longer a while(true) loop, either.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_sequence.cc:78: while (iter !=
> element->Properties().end()) {
> Use a for loop and pull 76 into it.
> 
Fixed.
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_sequence.h:30: typedef
> ScopedVector<LayerAnimationElement> Elements;
> Move this to the private section.
> 
Fixed.
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_sequence.h:37: 
> remove one of these lines.
> 
Fixed.
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_sequence.h:64: // Returns true, if the
> sequences share a common property.
> How about: returns true if this sequence has at least one element whose
> properties match |other|.
> 
Fixed. Changed the wording to something similar.
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_sequence.h:70:
> LayerAnimationElement::AnimatableProperties properties_;
> Description?
> 
Fixed.
> It's better form to create _unittest for each of the files you're going to
test.
> We sometimes don't do that if it's too hard to test each piece in isolation,
but
> that isn't the case here. You should be able to write tests for
> LayerAnimationSequence and LayerAnimator in isolate of the rest, so that I
would
> separate out the two types of tests. If you need to share classes, like your
> DummyLaerAnimationDelegate, move it into a .h/cc. See ui/aura/test/test* for
> examples of shared classes.
Fixed. Split the unit tests into three files and added test_utils and
test_layer_animation_delegate.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_unittest.cc:96:
> TEST(LayerAnimationElementTest, BoundsElement) {
> Add descriptions for each of your tests.
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:53: animation_->SetContainer(container);
> Make all animations share the same container like the old code was doing.
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:43: /* static */
> /* static */ -> // static
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animator.h (right):
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.h:32: // We need to keep track of the start
> time of every running animation.
> Make these (and typedefs private).
Done.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.h:61: virtual ~LayerAnimator();
> constructors before other methods
Done.

[Ian Vollick, 2011-10-19 22:59:17]

>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:85: if (!delegate_) {
> remove {} Why does no delegate imply you need to clear the animations?
delegate should not be NULL. Added a DCHECK and I no longer clear the
animations.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:122: const
> std::vector<LayerAnimationSequence*>& animations) {
> nit: spacing
Fixed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:125:
> std::vector<LayerAnimationSequence*>::const_iterator iter =
> move iterator into for loop.
Fixed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:128:
> LayerAnimationElement::AnimatableProperties::const_iterator prop_iter =
> Can this be animated_properties.insert((*iter)->properties().begin(),
...end())?
Yep. I've fixed this everywhere.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:139: // are free.
> free? Do you mean existing animations done?
Changed the wording. By 'property is free' I meant 'there is no running
animation affecting that property'
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:155: return animation_queue_.size() > 0;
> !empty(). You can inline this if you want.
Fixed, inlined.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:176: RunningAnimations
> running_animations_copy = running_animations_;
> Document why you need to make a copy.
Don't actually need the copy. Fixed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:177: RunningAnimations::iterator iter =
> running_animations_copy.begin();
> move iterator into for lop.
Done (fixed all these loops).
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:433: : base::TimeTicks::Now();
> For the animating case this effectively starts the animation at 0, which isn't
> always the case. Don't you need to adjust based on how far over the last
> sequences duration you went?
I think that this works, but I may be wrong. Here was my logic when writing
this: if you're the first animation, is_animating() should be false and so your
time is Now() and we call Step with sets last_step_time_ to your start time. If
you're not the first animation, then is_animating() should be true and
last_step_time_ should have a valid value.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.h:63: // Sets the transform on the Layer.
> Document what these does (the 3 setters). Also, do they need to be virtual?
Done. And, yes, I override them in the screen rotation animator.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.h:77: // a property is set during an
animation.
> This class does not own the
> Remove last sentence and document what the default is.
Fixed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.h:110: void Step(base::TimeTicks now);
> This shouldn't be public.
Fixed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.h:120: void UpdateAnimationState();
> Add descriptions for these methods.
Fixed.
> 
>
http://codereview.chromium.org/8247009/diff/26001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.h:142: // This is the queue of animations to
> run
> end sentences with periods.
Fixed.

[sky, 2011-10-20 16:33:03]

I'm not an OWNER of base, so you'll need to land that separately.

  -Scott

http://codereview.chromium.org/8247009/diff/31042/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8247009/diff/31042/base/base.gypi#newcode174
base/base.gypi:174: 'memory/scoped_deque.h',
sorting is off.

http://codereview.chromium.org/8247009/diff/31042/base/memory/scoped_deque.h
File base/memory/scoped_deque.h (right):

http://codereview.chromium.org/8247009/diff/31042/base/memory/scoped_deque.h#...
base/memory/scoped_deque.h:100: private:
newline between 99 and 100.

http://codereview.chromium.org/8247009/diff/31042/ui/base/animation/tween.cc
File ui/base/animation/tween.cc (right):

http://codereview.chromium.org/8247009/diff/31042/ui/base/animation/tween.cc#...
ui/base/animation/tween.cc:15: #include "ui/gfx/interpolated_transform.h"
sort

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_element.cc (right):

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:31: AnimatableProperties
properties_;
Remove these.

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:68: Transform target_;
const

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:103: gfx::Rect target_;
const

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element.cc:136: float target_;
const

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_element_unittest.cc (right):

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_element_unittest.cc:13: #include
"ui/gfx/compositor/layer_animation_element.h"
Tests should include the .h first, as you would in a non test .cc file.

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_sequence.cc (right):

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.cc:67: }
Do you want something like:
if (is_cyclic_)
  last_element_ = last_element_ % elements_.size();
Otherwise I think Abort is problematic.

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/test_laye...
File ui/gfx/compositor/test_layer_animation_delegate.h (right):

http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/test_laye...
ui/gfx/compositor/test_layer_animation_delegate.h:39: 
remove one of these lines.

[Ian Vollick, 2011-10-20 17:38:12]

On 2011/10/20 16:33:03, sky wrote:
> I'm not an OWNER of base, so you'll need to land that separately.
I've put it up for review by brettw
http://codereview.chromium.org/8361004/
> 
> http://codereview.chromium.org/8247009/diff/31042/base/base.gypi
> File base/base.gypi (right):
> 
> http://codereview.chromium.org/8247009/diff/31042/base/base.gypi#newcode174
> base/base.gypi:174: 'memory/scoped_deque.h',
> sorting is off.
> 
Fixed.
> http://codereview.chromium.org/8247009/diff/31042/base/memory/scoped_deque.h
> File base/memory/scoped_deque.h (right):
> 
>
http://codereview.chromium.org/8247009/diff/31042/base/memory/scoped_deque.h#...
> base/memory/scoped_deque.h:100: private:
> newline between 99 and 100.
> 
Fixed.
> http://codereview.chromium.org/8247009/diff/31042/ui/base/animation/tween.cc
> File ui/base/animation/tween.cc (right):
> 
>
http://codereview.chromium.org/8247009/diff/31042/ui/base/animation/tween.cc#...
> ui/base/animation/tween.cc:15: #include "ui/gfx/interpolated_transform.h"
> sort
> 
Done.
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animation_element.cc (right):
> 
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.cc:31: AnimatableProperties
> properties_;
> Remove these.
> 
Done.
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.cc:68: Transform target_;
> const
> 
Done.
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.cc:103: gfx::Rect target_;
> const
> 
Done.
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element.cc:136: float target_;
> const
> 
Done.
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animation_element_unittest.cc (right):
> 
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_element_unittest.cc:13: #include
> "ui/gfx/compositor/layer_animation_element.h"
> Tests should include the .h first, as you would in a non test .cc file.
> 
Fixed for all three.
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animation_sequence.cc (right):
> 
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_sequence.cc:67: }
> Do you want something like:
> if (is_cyclic_)
>   last_element_ = last_element_ % elements_.size();
> Otherwise I think Abort is problematic.
> 
Abort did have problems with cyclic sequences. I do think that last_element_
should continue to increase, so I've updated Abort instead and added a unit
test.
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/test_laye...
> File ui/gfx/compositor/test_layer_animation_delegate.h (right):
> 
>
http://codereview.chromium.org/8247009/diff/31042/ui/gfx/compositor/test_laye...
> ui/gfx/compositor/test_layer_animation_delegate.h:39: 
> remove one of these lines.

Fixed.

[sky, 2011-10-20 18:25:01]

LGTM

[Ian Vollick, 2011-10-20 18:58:10]

On 2011/10/20 18:25:01, sky wrote:
> LGTM

On brettw's recommendation, I got rid of ScopedDeque and stopped using
ScopedVector, too. Used standard containers with linked_ptr's instead. Still
LGTY?

[sky, 2011-10-20 19:05:56]

On Thu, Oct 20, 2011 at 11:58 AM,  <vollick@chromium.org> wrote:
> On 2011/10/20 18:25:01, sky wrote:
>>
>> LGTM
>
> On brettw's recommendation, I got rid of ScopedDeque and stopped using
> ScopedVector, too. Used standard containers with linked_ptr's instead. Still
> LGTY?

SLGTM

  -Scott