[UI Cleanup] Introduce common WidgetVisibleWaiter

Introduce a common views::test::WidgetVisibleWaiter helper class to
enable tests to easily wait for a given widget to become visible, and
use it in place of places that had similar functionality.

Bug: None
Change-Id: I4fda7baeef12ae33f47a47393a58396f917b65e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2151343Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759725}

chrome/browser/ui/views/extensions/extension_installed_bubble_view_browsertest.cc

@@ -20,47 +20,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/views/test/widget_test.h"
 #include "ui/views/widget/widget.h"
-#include "ui/views/widget/widget_observer.h"
-
-namespace {
-
-// Helper class to wait for a views::Widget to become visible.
-// TODO(devlin): Move this to a common place, and hunt down other similar
-// functionality.
-class WidgetVisibleWaiter : public views::WidgetObserver {
- public:
-  explicit WidgetVisibleWaiter(views::Widget* widget) : widget_(widget) {}
-  ~WidgetVisibleWaiter() override = default;
-
-  void Wait() {
-    if (!widget_->IsVisible()) {
-      widget_observer_.Add(widget_);
-      run_loop_.Run();
-    }
-  }
-
-  // WidgetObserver:
-  void OnWidgetVisibilityChanged(views::Widget* widget, bool visible) override {
-    DCHECK_EQ(widget_, widget);
-    if (visible)
-      run_loop_.Quit();
-  }
-
-  void OnWidgetDestroying(views::Widget* widget) override {
-    DCHECK_EQ(widget_, widget);
-    ADD_FAILURE() << "Widget destroying before it became visible!";
-    // Even though the test failed, be polite and remove the observer so we
-    // don't crash with a UAF in the destructor.
-    widget_observer_.Remove(widget);
-  }
-
- private:
-  views::Widget* const widget_;
-  base::RunLoop run_loop_;
-  ScopedObserver<views::Widget, views::WidgetObserver> widget_observer_{this};
-};
-
-}  // namespace
 
 class ExtensionInstalledBubbleViewsBrowserTest
     : public SupportsTestDialog<extensions::ExtensionBrowserTest>,
@@ -135,7 +94,7 @@ void ExtensionInstalledBubbleViewsBrowserTest::ShowUi(const std::string& name) {
   if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
     // With the toolbar menu, the extension slides out of the menu before the
     // bubble shows. Wait for the bubble to become visible.
-    WidgetVisibleWaiter(bubble_widget_).Wait();
+    views::test::WidgetVisibleWaiter(bubble_widget_).Wait();
   }
 }
 

chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc

@@ -21,6 +21,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "ui/gfx/animation/animation_test_api.h"
 #include "ui/views/controls/label.h"
+#include "ui/views/test/widget_test.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
@@ -51,32 +52,6 @@ class WindowDeactivedWaiter : public views::WidgetObserver {
   base::RunLoop run_loop_;
 };
 
-// Helper to wait until the hover card widget is visible.
-class HoverCardVisibleWaiter : public views::WidgetObserver {
- public:
-  explicit HoverCardVisibleWaiter(Widget* hover_card)
-      : hover_card_(hover_card) {
-    hover_card_->AddObserver(this);
-  }
-  ~HoverCardVisibleWaiter() override { hover_card_->RemoveObserver(this); }
-
-  void Wait() {
-    if (hover_card_->IsVisible())
-      return;
-    run_loop_.Run();
-  }
-
-  // WidgetObserver overrides:
-  void OnWidgetVisibilityChanged(Widget* widget, bool visible) override {
-    if (visible)
-      run_loop_.Quit();
-  }
-
- private:
-  Widget* const hover_card_;
-  base::RunLoop run_loop_;
-};
-
 class TabHoverCardBubbleViewBrowserTest : public DialogBrowserTest {
  public:
   TabHoverCardBubbleViewBrowserTest()
@@ -130,8 +105,7 @@ class TabHoverCardBubbleViewBrowserTest : public DialogBrowserTest {
   // DialogBrowserTest:
   void ShowUi(const std::string& name) override {
     HoverMouseOverTabAt(0);
-    HoverCardVisibleWaiter waiter(hover_card()->GetWidget());
-    waiter.Wait();
+    views::test::WidgetVisibleWaiter(hover_card()->GetWidget()).Wait();
   }
 
  private:
@@ -181,9 +155,8 @@ IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
   Tab* const tab = tab_strip()->tab_at(0);
   tab_strip()->GetFocusManager()->SetFocusedView(tab);
   Widget* const widget = hover_card()->GetWidget();
-  HoverCardVisibleWaiter waiter(widget);
-  waiter.Wait();
   ASSERT_NE(nullptr, widget);
+  views::test::WidgetVisibleWaiter(widget).Wait();
   EXPECT_TRUE(widget->IsVisible());
 }
 
@@ -202,13 +175,11 @@ IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
   Tab* const tab = tab_strip()->tab_at(0);
   tab_strip()->GetFocusManager()->SetFocusedView(tab);
   Widget* const widget = hover_card()->GetWidget();
-  HoverCardVisibleWaiter waiter(widget);
-  waiter.Wait();
   ASSERT_NE(nullptr, widget);
+  views::test::WidgetVisibleWaiter(widget).Wait();
   EXPECT_TRUE(widget->IsVisible());
   tab_strip()->GetFocusManager()->SetFocusedView(tab->close_button_);
-  waiter.Wait();
-  ASSERT_NE(nullptr, widget);
+  views::test::WidgetVisibleWaiter(widget).Wait();
   EXPECT_TRUE(widget->IsVisible());
 }
 
@@ -218,9 +189,8 @@ IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
   Tab* const tab = tab_strip()->tab_at(0);
   tab_strip()->GetFocusManager()->SetFocusedView(tab);
   Widget* const widget = hover_card()->GetWidget();
-  HoverCardVisibleWaiter waiter(widget);
-  waiter.Wait();
   ASSERT_NE(nullptr, widget);
+  views::test::WidgetVisibleWaiter(widget).Wait();
   EXPECT_TRUE(widget->IsVisible());
 
   ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, 0);
@@ -235,9 +205,8 @@ IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
   Tab* const tab = tab_strip()->tab_at(0);
   tab_strip()->GetFocusManager()->SetFocusedView(tab);
   Widget* const widget = hover_card()->GetWidget();
-  HoverCardVisibleWaiter waiter(widget);
-  waiter.Wait();
   ASSERT_NE(nullptr, widget);
+  views::test::WidgetVisibleWaiter(widget).Wait();
   EXPECT_TRUE(widget->IsVisible());
 
   ClickMouseOnTab(0);
@@ -267,15 +236,13 @@ IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
     browser()->command_controller()->ExecuteCommand(IDC_FOCUS_NEXT_PANE);
 
   Widget* const widget = hover_card()->GetWidget();
-  HoverCardVisibleWaiter waiter(widget);
-  waiter.Wait();
   ASSERT_NE(nullptr, widget);
+  views::test::WidgetVisibleWaiter(widget).Wait();
   EXPECT_TRUE(widget->IsVisible());
 
   // Move focus forward to the close button or next tab dependent on window
   // size.
   tab_strip()->AcceleratorPressed(ui::Accelerator(ui::VKEY_RIGHT, ui::EF_NONE));
-  ASSERT_NE(nullptr, widget);
   EXPECT_TRUE(widget->IsVisible());
 }
 
@@ -378,16 +345,14 @@ IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
   HoverMouseOverTabAt(0);
 
   Widget* const widget = hover_card()->GetWidget();
-  HoverCardVisibleWaiter waiter(widget);
-  waiter.Wait();
+  ASSERT_NE(nullptr, widget);
+  views::test::WidgetVisibleWaiter(widget).Wait();
 
   EXPECT_EQ(GetHoverCardsSeenCount(), 1);
-  ASSERT_NE(nullptr, widget);
   EXPECT_TRUE(widget->IsVisible());
 
   HoverMouseOverTabAt(1);
   EXPECT_EQ(GetHoverCardsSeenCount(), 2);
-  ASSERT_NE(nullptr, widget);
   EXPECT_TRUE(widget->IsVisible());
 
   ui::ListSelectionModel selection;

chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_interactive_uitest.cc

@@ -14,37 +14,11 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "ui/events/keycodes/keyboard_codes.h"
+#include "ui/views/test/widget_test.h"
 #include "ui/views/widget/widget.h"
-#include "ui/views/widget/widget_observer.h"
 
 using views::Widget;
 
-// Helper to wait until the hover card widget is visible.
-class HoverCardVisibleWaiter : public views::WidgetObserver {
- public:
-  explicit HoverCardVisibleWaiter(Widget* hover_card)
-      : hover_card_(hover_card) {
-    hover_card_->AddObserver(this);
-  }
-  ~HoverCardVisibleWaiter() override { hover_card_->RemoveObserver(this); }
-
-  void Wait() {
-    if (hover_card_->IsVisible())
-      return;
-    run_loop_.Run();
-  }
-
-  // WidgetObserver overrides:
-  void OnWidgetVisibilityChanged(Widget* widget, bool visible) override {
-    if (visible)
-      run_loop_.Quit();
-  }
-
- private:
-  Widget* const hover_card_;
-  base::RunLoop run_loop_;
-};
-
 class TabHoverCardBubbleViewInteractiveUiTest : public InProcessBrowserTest {
  public:
   TabHoverCardBubbleViewInteractiveUiTest() {
@@ -74,10 +48,8 @@ IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewInteractiveUiTest,
   tab_strip->UpdateHoverCard(tab);
   TabHoverCardBubbleView* hover_card = GetHoverCard(tab_strip);
   Widget* widget = hover_card->GetWidget();
-  HoverCardVisibleWaiter waiter(widget);
-  waiter.Wait();
-
   EXPECT_NE(nullptr, widget);
+  views::test::WidgetVisibleWaiter(widget).Wait();
   EXPECT_TRUE(widget->IsVisible());
 
   EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_DOWN, false,

ui/views/test/widget_test.cc

@@ -5,6 +5,7 @@
 #include "ui/views/test/widget_test.h"
 
 #include "build/build_config.h"
+#include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/views/test/native_widget_factory.h"
 #include "ui/views/widget/root_view.h"
@@ -223,5 +224,32 @@ void WidgetDestroyedWaiter::OnWidgetDestroyed(Widget* widget) {
   run_loop_.Quit();
 }
 
+WidgetVisibleWaiter::WidgetVisibleWaiter(Widget* widget) : widget_(widget) {}
+WidgetVisibleWaiter::~WidgetVisibleWaiter() = default;
+
+void WidgetVisibleWaiter::Wait() {
+  if (!widget_->IsVisible()) {
+    widget_observer_.Add(widget_);
+    run_loop_.Run();
+  }
+}
+
+void WidgetVisibleWaiter::OnWidgetVisibilityChanged(Widget* widget,
+                                                    bool visible) {
+  DCHECK_EQ(widget_, widget);
+  if (visible) {
+    widget_observer_.Remove(widget);
+    run_loop_.Quit();
+  }
+}
+
+void WidgetVisibleWaiter::OnWidgetDestroying(Widget* widget) {
+  DCHECK_EQ(widget_, widget);
+  ADD_FAILURE() << "Widget destroying before it became visible!";
+  // Even though the test failed, be polite and remove the observer so we
+  // don't crash with a UAF in the destructor.
+  widget_observer_.Remove(widget);
+}
+
 }  // namespace test
 }  // namespace views

ui/views/test/widget_test.h

@@ -11,6 +11,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/scoped_observer.h"
 #include "build/build_config.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/views/test/views_test_base.h"
@@ -273,6 +274,29 @@ class WidgetDestroyedWaiter : public WidgetObserver {
   DISALLOW_COPY_AND_ASSIGN(WidgetDestroyedWaiter);
 };
 
+// Helper class to wait for a Widget to become visible. This will add a failure
+// to the currently-running test if the widget is destroyed before becoming
+// visible.
+class WidgetVisibleWaiter : public WidgetObserver {
+ public:
+  explicit WidgetVisibleWaiter(Widget* widget);
+  WidgetVisibleWaiter(const WidgetVisibleWaiter&) = delete;
+  WidgetVisibleWaiter& operator=(const WidgetVisibleWaiter&) = delete;
+  ~WidgetVisibleWaiter() override;
+
+  // Waits for the widget to become visible.
+  void Wait();
+
+ private:
+  // WidgetObserver:
+  void OnWidgetVisibilityChanged(Widget* widget, bool visible) override;
+  void OnWidgetDestroying(Widget* widget) override;
+
+  Widget* const widget_;
+  base::RunLoop run_loop_;
+  ScopedObserver<Widget, WidgetObserver> widget_observer_{this};
+};
+
 }  // namespace test
 }  // namespace views