Attempt at fixing crash in BoundsAnimator. This patch largely just

cleans things up and fixes some possible usage problems by
TabStrip. I'm not confident this fixes the crash, but we'll see.

BUG=41538
TEST=none

Review URL: http://codereview.chromium.org/1710003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@45602 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 67 insertions, 59 deletions

diff --git a/views/animation/bounds_animator.cc b/views/animation/bounds_animator.cc
index aee3675..40cae9c 100644
--- a/views/animation/bounds_animator.cc
+++ b/views/animation/bounds_animator.cc
@@ -31,14 +31,13 @@ BoundsAnimator::~BoundsAnimator() {
     CleanupData(false, &(i->second), i->first);
 }
 
-void BoundsAnimator::AnimateViewTo(View* view,
-                                   const gfx::Rect& target,
-                                   bool delete_when_done) {
+void BoundsAnimator::AnimateViewTo(View* view, const gfx::Rect& target) {
+  DCHECK(view);
   DCHECK_EQ(view->GetParent(), parent_);
 
   Data existing_data;
 
-  if (data_.count(view) > 0) {
+  if (IsAnimating(view)) {
     // Don't immediatly delete the animation, that might trigger a callback from
     // the animationcontainer.
     existing_data = data_[view];
@@ -55,7 +54,6 @@ void BoundsAnimator::AnimateViewTo(View* view,
   data.start_bounds = view->bounds();
   data.target_bounds = target;
   data.animation = CreateAnimation();
-  data.delete_when_done = delete_when_done;
 
   animation_to_view_[data.animation] = view;
 
@@ -66,8 +64,11 @@ void BoundsAnimator::AnimateViewTo(View* view,
 
 void BoundsAnimator::SetAnimationForView(View* view,
                                          SlideAnimation* animation) {
+  DCHECK(animation);
+
   scoped_ptr<SlideAnimation> animation_wrapper(animation);
-  if (data_.find(view) == data_.end())
+
+  if (!IsAnimating(view))
     return;
 
   // We delay deleting the animation until the end so that we don't prematurely
@@ -83,19 +84,20 @@ void BoundsAnimator::SetAnimationForView(View* view,
 }
 
 const SlideAnimation* BoundsAnimator::GetAnimationForView(View* view) {
-  return data_.find(view) == data_.end() ? NULL : data_[view].animation;
+  return !IsAnimating(view) ? NULL : data_[view].animation;
 }
 
 void BoundsAnimator::SetAnimationDelegate(View* view,
                                           AnimationDelegate* delegate,
                                           bool delete_when_done) {
   DCHECK(IsAnimating(view));
+
   data_[view].delegate = delegate;
   data_[view].delete_delegate_when_done = delete_when_done;
 }
 
 void BoundsAnimator::StopAnimatingView(View* view) {
-  if (data_.find(view) == data_.end())
+  if (!IsAnimating(view))
     return;
 
   data_[view].animation->Stop();
@@ -130,6 +132,7 @@ SlideAnimation* BoundsAnimator::CreateAnimation() {
 
 void BoundsAnimator::RemoveFromMaps(View* view) {
   DCHECK(data_.count(view) > 0);
+  DCHECK(animation_to_view_.count(data_[view].animation) > 0);
 
   animation_to_view_.erase(data_[view].animation);
   data_.erase(view);
@@ -144,15 +147,15 @@ void BoundsAnimator::CleanupData(bool send_cancel, Data* data, View* view) {
     data->delegate = NULL;
   }
 
-  if (data->delete_when_done)
-    delete view;
-
-  delete data->animation;
-  data->animation = NULL;
+  if (data->animation) {
+    data->animation->set_delegate(NULL);
+    delete data->animation;
+    data->animation = NULL;
+  }
 }
 
 Animation* BoundsAnimator::ResetAnimationForView(View* view) {
-  if (data_.find(view) == data_.end())
+  if (!IsAnimating(view))
     return NULL;
 
   Animation* old_animation = data_[view].animation;
@@ -164,10 +167,35 @@ Animation* BoundsAnimator::ResetAnimationForView(View* view) {
   return old_animation;
 }
 
+void BoundsAnimator::AnimationEndedOrCanceled(const Animation* animation,
+                                              AnimationEndType type) {
+  DCHECK(animation_to_view_.find(animation) != animation_to_view_.end());
+
+  View* view = animation_to_view_[animation];
+  DCHECK(view);
+
+  // Make a copy of the data as Remove empties out the maps.
+  Data data = data_[view];
+
+  RemoveFromMaps(view);
+
+  if (data.delegate) {
+    if (type == ANIMATION_ENDED) {
+      data.delegate->AnimationEnded(animation);
+    } else {
+      DCHECK_EQ(ANIMATION_CANCELED, type);
+      data.delegate->AnimationCanceled(animation);
+    }
+  }
+
+  CleanupData(false, &data, view);
+}
+
 void BoundsAnimator::AnimationProgressed(const Animation* animation) {
   DCHECK(animation_to_view_.find(animation) != animation_to_view_.end());
 
   View* view = animation_to_view_[animation];
+  DCHECK(view);
   const Data& data = data_[view];
   gfx::Rect new_bounds =
       animation->CurrentValueBetween(data.start_bounds, data.target_bounds);
@@ -184,38 +212,16 @@ void BoundsAnimator::AnimationProgressed(const Animation* animation) {
     view->SetBounds(new_bounds);
   }
 
-  if (data_[view].delegate)
-    data_[view].delegate->AnimationProgressed(animation);
+  if (data.delegate)
+    data.delegate->AnimationProgressed(animation);
 }
 
 void BoundsAnimator::AnimationEnded(const Animation* animation) {
-  View* view = animation_to_view_[animation];
-  AnimationDelegate* delegate = data_[view].delegate;
-
-  // Make a copy of the data as Remove empties out the maps.
-  Data data = data_[view];
-
-  RemoveFromMaps(view);
-
-  if (delegate)
-    delegate->AnimationEnded(animation);
-
-  CleanupData(false, &data, view);
+  AnimationEndedOrCanceled(animation, ANIMATION_ENDED);
 }
 
 void BoundsAnimator::AnimationCanceled(const Animation* animation) {
-  View* view = animation_to_view_[animation];
-  AnimationDelegate* delegate = data_[view].delegate;
-
-  // Make a copy of the data as Remove empties out the maps.
-  Data data = data_[view];
-
-  RemoveFromMaps(view);
-
-  if (delegate)
-    delegate->AnimationCanceled(animation);
-
-  CleanupData(false, &data, view);
+  AnimationEndedOrCanceled(animation, ANIMATION_CANCELED);
 }
 
 void BoundsAnimator::AnimationContainerProgressed(
diff --git a/views/animation/bounds_animator.h b/views/animation/bounds_animator.h
index 606c05f..e63bbaa 100644
--- a/views/animation/bounds_animator.h
+++ b/views/animation/bounds_animator.h
@@ -47,15 +47,11 @@ class BoundsAnimator : public AnimationDelegate,
   explicit BoundsAnimator(View* view);
   ~BoundsAnimator();
 
-  // Starts animating |view| from its current bounds to |target|. If
-  // |delete_when_done| is true the view is deleted when the animation
-  // completes. If there is already an animation running for the view it's
-  // stopped and a new one started. If an AnimationDelegate has been set for
-  // |view| it is removed (after being notified that the animation was
-  // canceled).
-  void AnimateViewTo(View* view,
-                     const gfx::Rect& target,
-                     bool delete_when_done);
+  // Starts animating |view| from its current bounds to |target|. If there is
+  // already an animation running for the view it's stopped and a new one
+  // started. If an AnimationDelegate has been set for |view| it is removed
+  // (after being notified that the animation was canceled).
+  void AnimateViewTo(View* view, const gfx::Rect& target);
 
   // Sets the animation for the specified view. BoundsAnimator takes ownership
   // of the specified animation.
@@ -98,14 +94,10 @@ class BoundsAnimator : public AnimationDelegate,
   // Tracks data about the view being animated.
   struct Data {
     Data()
-        : delete_when_done(false),
-          delete_delegate_when_done(false),
+        : delete_delegate_when_done(false),
           animation(NULL),
           delegate(NULL) {}
 
-    // Should the view be deleted when done?
-    bool delete_when_done;
-
     // If true the delegate is deleted when done.
     bool delete_delegate_when_done;
 
@@ -122,6 +114,12 @@ class BoundsAnimator : public AnimationDelegate,
     AnimationDelegate* delegate;
   };
 
+  // Used by AnimationEndedOrCanceled.
+  enum AnimationEndType {
+    ANIMATION_ENDED,
+    ANIMATION_CANCELED
+  };
+
   typedef std::map<View*, Data> ViewToDataMap;
 
   typedef std::map<const Animation*, View*> AnimationToViewMap;
@@ -139,6 +137,10 @@ class BoundsAnimator : public AnimationDelegate,
   // of the returned animation passes to the caller.
   Animation* ResetAnimationForView(View* view);
 
+  // Invoked from AnimationEnded and AnimationCanceled.
+  void AnimationEndedOrCanceled(const Animation* animation,
+                                AnimationEndType type);
+
   // AnimationDelegate overrides.
   virtual void AnimationProgressed(const Animation* animation);
   virtual void AnimationEnded(const Animation* animation);
diff --git a/views/animation/bounds_animator_unittest.cc b/views/animation/bounds_animator_unittest.cc
index 74ea5f2..d85b185 100644
--- a/views/animation/bounds_animator_unittest.cc
+++ b/views/animation/bounds_animator_unittest.cc
@@ -114,7 +114,7 @@ TEST_F(BoundsAnimatorTest, AnimateViewTo) {
   gfx::Rect initial_bounds(0, 0, 10, 10);
   child()->SetBounds(initial_bounds);
   gfx::Rect target_bounds(10, 10, 20, 20);
-  animator()->AnimateViewTo(child(), target_bounds, false);
+  animator()->AnimateViewTo(child(), target_bounds);
   animator()->SetAnimationDelegate(child(), &delegate, false);
 
   // The animator should be animating now.
@@ -134,7 +134,7 @@ TEST_F(BoundsAnimatorTest, AnimateViewTo) {
 
 // Make sure an AnimationDelegate is deleted when canceled.
 TEST_F(BoundsAnimatorTest, DeleteDelegateOnCancel) {
-  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10), false);
+  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
   animator()->SetAnimationDelegate(child(), new OwnedDelegate(), true);
 
   animator()->Cancel();
@@ -150,10 +150,10 @@ TEST_F(BoundsAnimatorTest, DeleteDelegateOnCancel) {
 // Make sure an AnimationDelegate is deleted when another animation is
 // scheduled.
 TEST_F(BoundsAnimatorTest, DeleteDelegateOnNewAnimate) {
-  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10), false);
+  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
   animator()->SetAnimationDelegate(child(), new OwnedDelegate(), true);
 
-  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10), false);
+  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
 
   // Starting a new animation should both cancel the delegate and delete it.
   EXPECT_TRUE(OwnedDelegate::get_and_clear_deleted());
@@ -164,7 +164,7 @@ TEST_F(BoundsAnimatorTest, DeleteDelegateOnNewAnimate) {
 TEST_F(BoundsAnimatorTest, StopAnimating) {
   scoped_ptr<OwnedDelegate> delegate(new OwnedDelegate());
 
-  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10), false);
+  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
   animator()->SetAnimationDelegate(child(), new OwnedDelegate(), true);
 
   animator()->StopAnimatingView(child());