Stop the animation associated to the removed icon

The regression issue was introduced by https://crrev.com/c/1925783
by destructing the removed view at the end of ShelfView::ShelfItemRe-
moved. However, on some edge cases (like switching between accounts),
the removed item may be destructed before animation on item ends.
This CL fixes the issue by stopping the animation associated to
the removed icon before destruction.

Bug: 1032002
Change-Id: I98aea59c9f4d0310c4c170edbb23c581816fdbe6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1975084Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726461}

ash/shelf/shelf_view.cc

@@ -259,6 +259,9 @@ class ShelfView::FadeOutAnimationDelegate : public gfx::AnimationDelegate {
     view_->layer()->ScheduleDraw();
   }
   void AnimationEnded(const Animation* animation) override {
+    // Ensures that |view| is not used after destruction.
+    shelf_view_->StopAnimatingViewIfAny(view_.get());
+
     // Remove the view which has been faded away.
     view_.reset();
 
@@ -462,6 +465,11 @@ ShelfAppButton* ShelfView::GetShelfAppButton(const ShelfID& id) {
   return static_cast<ShelfAppButton*>(view);
 }
 
+void ShelfView::StopAnimatingViewIfAny(views::View* view) {
+  if (bounds_animator_->IsAnimating(view))
+    bounds_animator_->StopAnimatingView(view);
+}
+
 bool ShelfView::ShouldHideTooltip(const gfx::Point& cursor_location) const {
   // There are thin gaps between launcher buttons but the tooltip shouldn't hide
   // in the gaps, but the tooltip should hide if the mouse moved totally outside
@@ -2281,10 +2289,17 @@ void ShelfView::ShelfItemRemoved(int model_index, const ShelfItem& old_item) {
         view.get(), std::unique_ptr<gfx::AnimationDelegate>(
                         new FadeOutAnimationDelegate(this, std::move(view))));
   } else {
+    // Ensures that |view| is not used after destruction.
+    StopAnimatingViewIfAny(view.get());
+
+    // Removes |view| to trigger ViewHierarchyChanged function in the parent
+    // view if any.
+    view.reset();
+
     // If there is no fade out animation, notify the parent view of the
     // changed size before bounds animations start.
     if (chromeos::switches::ShouldShowScrollableShelf())
-      HandleInvisibleViewRemovedInScrollableShelf(std::move(view));
+      PreferredSizeChanged();
 
     // We don't need to show a fade out animation for invisible |view|. When an
     // item is ripped out from the shelf, its |view| is already invisible.
@@ -2633,20 +2648,4 @@ bool ShelfView::UpdateVisibleIndices() {
          (last_visible_index_ != previous_last_visible_index);
 }
 
-void ShelfView::HandleInvisibleViewRemovedInScrollableShelf(
-    std::unique_ptr<views::View> view) {
-  DCHECK(chromeos::switches::ShouldShowScrollableShelf());
-  DCHECK(!view->GetVisible());
-
-  // Ensures that |view| is not used after destruction.
-  if (bounds_animator_->IsAnimating(view.get()))
-    bounds_animator_->StopAnimatingView(view.get());
-
-  // Remove |view| so that the visible index is updated before triggering
-  // PreferredSizeChanged().
-  view.reset();
-
-  PreferredSizeChanged();
-}
-
 }  // namespace ash

ash/shelf/shelf_view.h

@@ -296,6 +296,10 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   // scrollable shelf is enabled. Returns whether those two indices are changed.
   bool UpdateVisibleIndices();
 
+  // If there is animation associated with |view| in |bounds_animator_|,
+  // stops the animation.
+  void StopAnimatingViewIfAny(views::View* view);
+
   // Return the view model for test purposes.
   const views::ViewModel* view_model_for_test() const {
     return view_model_.get();
@@ -567,10 +571,6 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   // Different from GetTitleForView, |view| here must be a child view.
   base::string16 GetTitleForChildView(const views::View* view) const;
 
-  // |view| is the unique pointer to the shelf icon to be removed.
-  void HandleInvisibleViewRemovedInScrollableShelf(
-      std::unique_ptr<views::View> view);
-
   // The model; owned by Launcher.
   ShelfModel* model_;