Always stop observing holding space bubble widget

HoldingSpaceTray observes holding space bubble widget, but it was not
removing itself from the widget observer list if the bubble got closed
without going through HoldingpaceTray::CloseBubble.

Without this, holding space tray unittests crash on teardown unless the
tray bubble is closed.

Bug: None
Change-Id: If01f7b23e0268734ba178c566f961e645525438f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2504214
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#823058}

ash/system/holding_space/holding_space_tray.cc

@@ -98,11 +98,7 @@ void HoldingSpaceTray::CloseBubble() {
   if (!bubble_)
     return;
 
-  // If the call to `CloseBubble()` originated from `OnWidgetDestroying()`, as
-  // would be the case when closing due to ESC key press, the bubble widget will
-  // have already been destroyed.
-  if (bubble_->GetBubbleWidget())
-    bubble_->GetBubbleWidget()->RemoveObserver(this);
+  widget_observer_.RemoveAll();
 
   bubble_.reset();
   SetIsActive(false);
@@ -120,7 +116,7 @@ void HoldingSpaceTray::ShowBubble(bool show_by_click) {
   // being destroyed. If destruction is due to a call to `CloseBubble()` we will
   // have already cleaned up state but there are cases where the bubble widget
   // is destroyed independent of a call to `CloseBubble()`, e.g. ESC key press.
-  bubble_->GetBubbleWidget()->AddObserver(this);
+  widget_observer_.Add(bubble_->GetBubbleWidget());
 
   SetIsActive(true);
 }
@@ -202,7 +198,6 @@ void HoldingSpaceTray::OnWidgetDragWillStart(views::Widget* widget) {
 }
 
 void HoldingSpaceTray::OnWidgetDestroying(views::Widget* widget) {
-  widget->RemoveObserver(this);
   CloseBubble();
 }
 

ash/system/holding_space/holding_space_tray.h

@@ -16,6 +16,7 @@
 #include "ash/system/tray/tray_background_view.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
+#include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
 namespace ash {
@@ -76,6 +77,7 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
       controller_observer_{this};
   ScopedObserver<HoldingSpaceModel, HoldingSpaceModelObserver> model_observer_{
       this};
+  ScopedObserver<views::Widget, views::WidgetObserver> widget_observer_{this};
 
   base::WeakPtrFactory<HoldingSpaceTray> weak_factory_{this};
 };

ash/system/holding_space/holding_space_tray_unittest.cc

@@ -310,8 +310,6 @@ TEST_F(HoldingSpaceTrayTest, DownloadsContainer) {
 
   // Pinned container is showing "educational" info, and it should remain shown.
   EXPECT_TRUE(test_api()->PinnedFilesContainerShown());
-
-  test_api()->Close();
 }
 
 // Verifies the downloads container is shown and orders items as expected when
@@ -395,8 +393,6 @@ TEST_F(HoldingSpaceTrayTest, FinalizingDownloadItemThatShouldBeInvisible) {
             HoldingSpaceItemView::Cast(download_chips[0])->item()->id());
   EXPECT_EQ(item_2->id(),
             HoldingSpaceItemView::Cast(download_chips[1])->item()->id());
-
-  test_api()->Close();
 }
 
 // Tests that a partially initialized download item does not get shown if a full
@@ -436,8 +432,6 @@ TEST_F(HoldingSpaceTrayTest, PartialItemNowShownOnRemovingADownloadItem) {
   ASSERT_EQ(1u, download_chips.size());
   EXPECT_EQ(item_3->id(),
             HoldingSpaceItemView::Cast(download_chips[0])->item()->id());
-
-  test_api()->Close();
 }
 
 // Tests how screen capture list is updated during item addition, removal and
@@ -538,8 +532,6 @@ TEST_F(HoldingSpaceTrayTest, ScreenCaptureContainer) {
 
   // Pinned container is showing "educational" info, and it should remain shown.
   EXPECT_TRUE(test_api()->PinnedFilesContainerShown());
-
-  test_api()->Close();
 }
 
 // Verifies the screen captures container is shown and orders items as expected
@@ -792,8 +784,6 @@ TEST_F(HoldingSpaceTrayTest, PinnedFilesContainer) {
 
   EXPECT_FALSE(test_api()->RecentFilesContainerShown());
   EXPECT_FALSE(test_api()->PinnedFilesContainerShown());
-
-  test_api()->Close();
 }
 
 // Verifies the pinned items container is not shown if it only contains
@@ -836,8 +826,6 @@ TEST_F(HoldingSpaceTrayTest,
   ASSERT_EQ(1u, pinned_files.size());
   EXPECT_EQ(item_3->id(),
             HoldingSpaceItemView::Cast(pinned_files[0])->item()->id());
-
-  test_api()->Close();
 }
 
 // Verifies the pinned items container is shown and orders items as expected