Adds AnimationContainer, which can be used to group a set of

Adds AnimationContainer, which can be used to group a set of
animations to have the same timer. By default each animation has one
animationcontainer, but I'm going to change the tab renderer to share
the animationcontainer so that the pulse effects happen in unison.

Also updated the BoundsAnimator so that I can use it by the TabStrip.

BUG=none
TEST=none

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

[sky, 2010-04-01 18:04:24]

Redirecting to Jay as Ben is busy.

  -Scott

[Jay Civelli, 2010-04-01 20:33:40]

http://codereview.chromium.org/1575011/diff/8001/9002
File app/animation.h (right):

http://codereview.chromium.org/1575011/diff/8001/9002#newcode113
app/animation.h:113: // Sets the container used to manage the timer. A value of
null results in
Nit: null -> NULL

http://codereview.chromium.org/1575011/diff/8001/9005
File app/animation_container_unittest.cc (right):

http://codereview.chromium.org/1575011/diff/8001/9005#newcode46
app/animation_container_unittest.cc:46: // Make sure the animation ups the ref
count of the container and releases it
Nit: Make -> Makes

http://codereview.chromium.org/1575011/diff/8001/9005#newcode51
app/animation_container_unittest.cc:51: scoped_ptr<Animation> animation(new
TestAnimation(&delegate));
Not that it matters much, but couldn't the Animation object be allocated on the
stack? (same question for tests below)

http://codereview.chromium.org/1575011/diff/8001/9005#newcode79
app/animation_container_unittest.cc:79: MessageLoop::current()->Run();
Could you add a comment mentioning the test delegate will quit the message loop
when the animation is finished? (it was not clear to me at first)

http://codereview.chromium.org/1575011/diff/8001/9010
File app/test_animation_delegate.h (right):

http://codereview.chromium.org/1575011/diff/8001/9010#newcode15
app/test_animation_delegate.h:15: TestAnimationDelegate() :
Semi-colon on next line.

http://codereview.chromium.org/1575011/diff/8001/9010#newcode16
app/test_animation_delegate.h:16: canceled_(false),
Indent should be 4 spaces.

http://codereview.chromium.org/1575011/diff/12001/2019
File views/animation/bounds_animator.cc (right):

http://codereview.chromium.org/1575011/diff/12001/2019#newcode132
views/animation/bounds_animator.cc:132: Data& data = data_[view];
Would it be usefull to DCHECK there is a Data for this view?

http://codereview.chromium.org/1575011/diff/12001/2019#newcode154
views/animation/bounds_animator.cc:154:
animation_to_view_.erase(data_[view].animation);
Consider using old_animation instead of data_[view].animation

http://codereview.chromium.org/1575011/diff/12001/2020
File views/animation/bounds_animator.h (right):

http://codereview.chromium.org/1575011/diff/12001/2020#newcode49
views/animation/bounds_animator.h:49: // Starts animating |view| from it's
current bounds to |target|. If
Nit: since you are editing this, it's -> its

http://codereview.chromium.org/1575011/diff/12001/2020#newcode123
views/animation/bounds_animator.h:123: // Removes references to |view| and it's
animation as well as deleting |view|
Nit: it's -> its

[sky, 2010-04-01 21:24:43]

I think I got it all. New version updated.

http://codereview.chromium.org/1575011/diff/8001/9005
File app/animation_container_unittest.cc (right):

http://codereview.chromium.org/1575011/diff/8001/9005#newcode51
app/animation_container_unittest.cc:51: scoped_ptr<Animation> animation(new
TestAnimation(&delegate));
On 2010/04/01 20:33:40, Jay Civelli wrote:
> Not that it matters much, but couldn't the Animation object be allocated on
the
> stack? (same question for tests below)

I think I copied parts of this code from the animation test without thinking
about it. Updated.

http://codereview.chromium.org/1575011/diff/12001/2019
File views/animation/bounds_animator.cc (right):

http://codereview.chromium.org/1575011/diff/12001/2019#newcode132
views/animation/bounds_animator.cc:132: Data& data = data_[view];
On 2010/04/01 20:33:40, Jay Civelli wrote:
> Would it be usefull to DCHECK there is a Data for this view?

It won't hurt.

[Jay Civelli, 2010-04-01 21:32:20]

LGTM

http://codereview.chromium.org/1575011/diff/18001/19019
File views/animation/bounds_animator.h (right):

http://codereview.chromium.org/1575011/diff/18001/19019#newcode52
views/animation/bounds_animator.h:52: // completes. If there is already an
animation running for the view its
I think this one should remain 'it's'