Cleanup for ViewEventTestBase.

* Replace WidgetDelegate inheritance with composition.
* Make window_ private.
* Null |window_| automatically (via the WidgetDelegate) when it closes.
* Replace manual Layout() override with FillLayout.
* Slightly less random declaration order.

Bug: none
Change-Id: I9179f6d581d6b90d50c324aa81fccdbf200570fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135533Reviewed-by: Thomas Lukaszewicz <tluk@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756311}

chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc

@@ -325,7 +325,7 @@ class BookmarkBarViewEventTestBase : public ViewEventTestBase {
     // clipboard selection from the X server (for the 'paste' item), so mock it
     // out.
     ui::TestClipboard::CreateForCurrentThread();
-    GetWidget()->Activate();
+    window()->Activate();
 #endif
   }
 
@@ -1056,7 +1056,7 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
 
     // Send a down event, which should select the first item.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
+        window()->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest10::Step3)));
   }
 
@@ -1069,7 +1069,7 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
 
     // Send a key down event, which should select the next item.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
+        window()->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest10::Step4)));
   }
 
@@ -1082,7 +1082,7 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
 
     // Send a right arrow to force the menu to open.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_RIGHT, false, false, false, false,
+        window()->GetNativeWindow(), ui::VKEY_RIGHT, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest10::Step5)));
   }
 
@@ -1098,7 +1098,7 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
 
     // Send a left arrow to close the submenu.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_LEFT, false, false, false, false,
+        window()->GetNativeWindow(), ui::VKEY_LEFT, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest10::Step6)));
   }
 
@@ -1113,7 +1113,7 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
 
     // Send a down arrow to go down to f1b.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
+        window()->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest10::Step7)));
   }
 
@@ -1126,7 +1126,7 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
 
     // Send a down arrow to wrap back to f1a.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
+        window()->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest10::Step8)));
   }
 
@@ -1139,8 +1139,8 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
 
     // Send enter, which should select the item.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_RETURN, false, false, false, false,
-        CreateEventTask(this, &BookmarkBarViewTest10::Step9)));
+        window()->GetNativeWindow(), ui::VKEY_RETURN, false, false, false,
+        false, CreateEventTask(this, &BookmarkBarViewTest10::Step9)));
   }
 
   void Step9() {
@@ -1194,8 +1194,8 @@ class BookmarkBarViewTest11 : public BookmarkBarViewEventTestBase {
   void Step3() {
     // Send escape so that the context menu hides.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_ESCAPE, false, false, false, false,
-        CreateEventTask(this, &BookmarkBarViewTest11::Step4)));
+        window()->GetNativeWindow(), ui::VKEY_ESCAPE, false, false, false,
+        false, CreateEventTask(this, &BookmarkBarViewTest11::Step4)));
   }
 
   void Step4() {
@@ -1426,8 +1426,8 @@ class BookmarkBarViewTest14 : public BookmarkBarViewEventTestBase {
 
     // Send escape so that the context menu hides.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_ESCAPE, false, false, false, false,
-        CreateEventTask(this, &BookmarkBarViewTest14::Step3)));
+        window()->GetNativeWindow(), ui::VKEY_ESCAPE, false, false, false,
+        false, CreateEventTask(this, &BookmarkBarViewTest14::Step3)));
   }
 
   void Step3() {
@@ -1542,8 +1542,7 @@ class BookmarkBarViewTest16 : public BookmarkBarViewEventTestBase {
     ASSERT_TRUE(button->state() == views::Button::STATE_PRESSED);
 
     // Close the window.
-    window_->Close();
-    window_ = NULL;
+    window()->Close();
 
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, CreateEventTask(this, &BookmarkBarViewTest16::Done));
@@ -1962,7 +1961,7 @@ class BookmarkBarViewTest22 : public BookmarkBarViewDragTestBase {
   void OnWidgetDragWillStart(views::Widget* widget) override {
     // Watch for main window destruction instead of menu dragging.
     widget_observer()->RemoveAll();
-    widget_observer()->Add(window_);
+    widget_observer()->Add(window());
 
     BookmarkBarViewDragTestBase::OnWidgetDragWillStart(widget);
   }
@@ -1988,8 +1987,7 @@ class BookmarkBarViewTest22 : public BookmarkBarViewDragTestBase {
     // window alone may not exit this message loop.
     BookmarkBarViewDragTestBase::OnDragEntered();
 
-    window_->Close();
-    window_ = nullptr;
+    window()->Close();
   }
 
   const BookmarkNode* GetDroppedNode() const override {
@@ -2031,8 +2029,7 @@ class BookmarkBarViewTest23 : public BookmarkBarViewEventTestBase {
 
     // Navigate down to highlight the first menu item.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        GetWidget()->GetNativeWindow(), ui::VKEY_DOWN, false, false, false,
-        false,  // No modifer keys
+        window()->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest23::Step3)));
   }
 
@@ -2043,10 +2040,9 @@ class BookmarkBarViewTest23 : public BookmarkBarViewEventTestBase {
     ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
 
     // Open the context menu via the keyboard.
-    ASSERT_TRUE(ui_controls::SendKeyPress(GetWidget()->GetNativeWindow(),
+    ASSERT_TRUE(ui_controls::SendKeyPress(window()->GetNativeWindow(),
                                           ui::VKEY_APPS, false, false, false,
-                                          false  // No modifer keys
-                                          ));
+                                          false));
     // The BookmarkContextMenuNotificationObserver triggers Step4.
   }
 
@@ -2102,8 +2098,7 @@ class BookmarkBarViewTest24 : public BookmarkBarViewEventTestBase {
 
     // Navigate down to highlight the first menu item.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        GetWidget()->GetNativeWindow(), ui::VKEY_DOWN, false, false, false,
-        false,  // No modifer keys
+        window()->GetNativeWindow(), ui::VKEY_DOWN, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest24::Step3)));
   }
 
@@ -2114,10 +2109,9 @@ class BookmarkBarViewTest24 : public BookmarkBarViewEventTestBase {
     ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
 
     // Open the context menu via the keyboard.
-    ASSERT_TRUE(ui_controls::SendKeyPress(GetWidget()->GetNativeWindow(),
+    ASSERT_TRUE(ui_controls::SendKeyPress(window()->GetNativeWindow(),
                                           ui::VKEY_APPS, false, false, false,
-                                          false  // No modifer keys
-                                          ));
+                                          false));
     // The BookmarkContextMenuNotificationObserver triggers Step4.
   }
 
@@ -2130,8 +2124,8 @@ class BookmarkBarViewTest24 : public BookmarkBarViewEventTestBase {
 
     // Send escape to close the context menu.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_ESCAPE, false, false, false, false,
-        CreateEventTask(this, &BookmarkBarViewTest24::Step5)));
+        window()->GetNativeWindow(), ui::VKEY_ESCAPE, false, false, false,
+        false, CreateEventTask(this, &BookmarkBarViewTest24::Step5)));
   }
 
   void Step5() {
@@ -2145,8 +2139,8 @@ class BookmarkBarViewTest24 : public BookmarkBarViewEventTestBase {
 
     // Send escape to close the main menu.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), ui::VKEY_ESCAPE, false, false, false, false,
-        CreateEventTask(this, &BookmarkBarViewTest24::Done)));
+        window()->GetNativeWindow(), ui::VKEY_ESCAPE, false, false, false,
+        false, CreateEventTask(this, &BookmarkBarViewTest24::Done)));
   }
 
   BookmarkContextMenuNotificationObserver observer_;
@@ -2177,7 +2171,7 @@ class BookmarkBarViewTest25 : public BookmarkBarViewEventTestBase {
 
     // Send KEYCODE key event, which should close the menu.
     ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
-        window_->GetNativeWindow(), KEYCODE, false, false, false, false,
+        window()->GetNativeWindow(), KEYCODE, false, false, false, false,
         CreateEventTask(this, &BookmarkBarViewTest25::Step3)));
   }
 
@@ -2220,9 +2214,8 @@ class BookmarkBarViewTest26 : public BookmarkBarViewEventTestBase {
     // Send WM_CANCELMODE, which should close the menu. The message is sent
     // synchronously, however, we post a task to make sure that the message is
     // processed completely before finishing the test.
-    ::SendMessage(
-        GetWidget()->GetNativeView()->GetHost()->GetAcceleratedWidget(),
-        WM_CANCELMODE, 0, 0);
+    ::SendMessage(window()->GetNativeView()->GetHost()->GetAcceleratedWidget(),
+                  WM_CANCELMODE, 0, 0);
 
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,

chrome/browser/ui/views/menu_test_base.cc

@@ -28,8 +28,8 @@ void MenuTestBase::Click(views::View* view, base::OnceClosure next) {
 }
 
 void MenuTestBase::KeyPress(ui::KeyboardCode keycode, base::OnceClosure next) {
-  ui_controls::SendKeyPressNotifyWhenDone(GetWidget()->GetNativeWindow(),
-                                          keycode, false, false, false, false,
+  ui_controls::SendKeyPressNotifyWhenDone(window()->GetNativeWindow(), keycode,
+                                          false, false, false, false,
                                           std::move(next));
 }
 

chrome/browser/ui/views/test/view_event_test_base.cc

@@ -17,36 +17,63 @@
 #include "ui/base/clipboard/clipboard.h"
 #include "ui/base/ime/init/input_method_initializer.h"
 #include "ui/compositor/test/test_context_factories.h"
+#include "ui/views/layout/fill_layout.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_delegate.h"
 
 namespace {
 
-// View subclass that allows you to specify the preferred size.
+// View that keeps its preferred size in sync with what |harness| requests.
 class TestView : public views::View {
  public:
-  explicit TestView(ViewEventTestBase* harness) : harness_(harness) {}
+  explicit TestView(ViewEventTestBase* harness) : harness_(harness) {
+    SetLayoutManager(std::make_unique<views::FillLayout>());
+    AddChildView(harness_->CreateContentsView());
+  }
+  TestView(const TestView&) = delete;
+  TestView& operator=(const TestView&) = delete;
+  ~TestView() override = default;
 
   gfx::Size CalculatePreferredSize() const override {
     return harness_->GetPreferredSizeForContents();
   }
 
-  void Layout() override {
-    // Permit a test to remove the view being tested from the hierarchy, then
-    // still handle a _NET_WM_STATE event on Linux during teardown that triggers
-    // layout.
-    if (!children().empty())
-      children().front()->SetBoundsRect(GetLocalBounds());
-  }
-
  private:
   ViewEventTestBase* harness_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestView);
 };
 
 }  // namespace
 
+class TestBaseWidgetDelegate : public views::WidgetDelegate {
+ public:
+  explicit TestBaseWidgetDelegate(ViewEventTestBase* harness)
+      : harness_(harness) {}
+  TestBaseWidgetDelegate(const TestBaseWidgetDelegate&) = delete;
+  TestBaseWidgetDelegate& operator=(const TestBaseWidgetDelegate&) = delete;
+  ~TestBaseWidgetDelegate() override = default;
+
+  // views::WidgetDelegate:
+  bool CanResize() const override { return true; }
+  void WindowClosing() override { harness_->window_ = nullptr; }
+  void DeleteDelegate() override { delete this; }
+  views::Widget* GetWidget() override { return contents_->GetWidget(); }
+  const views::Widget* GetWidget() const override {
+    return contents_->GetWidget();
+  }
+  views::View* GetContentsView() override {
+    // This will first be called by Widget::Init(), which passes the returned
+    // View* to SetContentsView(), which takes ownership.
+    if (!contents_)
+      contents_ = new TestView(harness_);
+    return contents_;
+  }
+
+ private:
+  ViewEventTestBase* harness_;
+  views::View* contents_ = nullptr;
+};
+
 ViewEventTestBase::ViewEventTestBase() {
   // The TestingBrowserProcess must be created in the constructor because there
   // are tests that require it before SetUp() is called.
@@ -58,9 +85,8 @@ ViewEventTestBase::ViewEventTestBase() {
   mojo::core::Init();
 }
 
-void ViewEventTestBase::Done() {
-  drag_event_thread_.reset();
-  run_loop_.Quit();
+ViewEventTestBase::~ViewEventTestBase() {
+  TestingBrowserProcess::DeleteInstance();
 }
 
 void ViewEventTestBase::SetUpTestCase() {
@@ -72,9 +98,7 @@ void ViewEventTestBase::SetUp() {
   ui::InitializeInputMethodForTesting();
 
   // The ContextFactory must exist before any Compositors are created.
-  const bool enable_pixel_output = false;
-  context_factories_ =
-      std::make_unique<ui::TestContextFactories>(enable_pixel_output);
+  context_factories_ = std::make_unique<ui::TestContextFactories>(false);
 
 #if defined(OS_MACOSX)
   views_delegate_.set_context_factory(context_factories_->GetContextFactory());
@@ -83,8 +107,10 @@ void ViewEventTestBase::SetUp() {
 
   platform_part_.reset(ViewEventTestPlatformPart::Create(
       context_factories_->GetContextFactory()));
+
   window_ = views::Widget::CreateWindowWithContext(
-      this, platform_part_->GetContext());
+      new TestBaseWidgetDelegate(this),  // Owns itself.
+      platform_part_->GetContext());
   window_->Show();
 }
 
@@ -92,7 +118,6 @@ void ViewEventTestBase::TearDown() {
   if (window_) {
     window_->Close();
     base::RunLoop().RunUntilIdle();
-    window_ = NULL;
   }
 
   ui::Clipboard::DestroyClipboardForCurrentThread();
@@ -107,31 +132,9 @@ gfx::Size ViewEventTestBase::GetPreferredSizeForContents() const {
   return gfx::Size();
 }
 
-bool ViewEventTestBase::CanResize() const {
-  return true;
-}
-
-views::View* ViewEventTestBase::GetContentsView() {
-  if (!content_view_) {
-    // Wrap the real view (as returned by CreateContentsView) in a View so
-    // that we can customize the preferred size.
-    TestView* test_view = new TestView(this);
-    test_view->AddChildView(CreateContentsView());
-    content_view_ = test_view;
-  }
-  return content_view_;
-}
-
-const views::Widget* ViewEventTestBase::GetWidget() const {
-  return content_view_->GetWidget();
-}
-
-views::Widget* ViewEventTestBase::GetWidget() {
-  return content_view_->GetWidget();
-}
-
-ViewEventTestBase::~ViewEventTestBase() {
-  TestingBrowserProcess::DeleteInstance();
+void ViewEventTestBase::Done() {
+  drag_event_thread_.reset();
+  run_loop_.Quit();
 }
 
 void ViewEventTestBase::StartMessageLoopAndRunTest() {

chrome/browser/ui/views/test/view_event_test_base.h

@@ -15,14 +15,12 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
-#include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
 #include "chrome/test/views/chrome_test_views_delegate.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "ui/views/widget/widget_delegate.h"
 
 #if defined(OS_WIN)
 #include "ui/base/win/scoped_ole_initializer.h"
@@ -37,6 +35,7 @@ class TestContextFactories;
 }
 
 class ViewEventTestPlatformPart;
+class TestBaseWidgetDelegate;
 
 // Base class for Views based tests that dispatch events.
 //
@@ -70,39 +69,34 @@ class ViewEventTestPlatformPart;
 // driven from observer callbacks and posted on the task runner returned by
 // GetDragTaskRunner().
 
-class ViewEventTestBase : public views::WidgetDelegate, public testing::Test {
+class ViewEventTestBase : public testing::Test {
  public:
   ViewEventTestBase();
-
-  // Invoke when done either because of failure or success. Quits the message
-  // loop.
-  void Done();
+  ViewEventTestBase(const ViewEventTestBase&) = delete;
+  ViewEventTestBase& operator=(const ViewEventTestBase&) = delete;
+  ~ViewEventTestBase() override;
 
   static void SetUpTestCase();
 
-  // Creates a window.
+  // testing::Test:
   void SetUp() override;
-
-  // Destroys the window.
   void TearDown() override;
 
+  // Returns the view that is added to the window.
+  virtual views::View* CreateContentsView() = 0;
+
   // Returns an empty Size. Subclasses that want a preferred size other than
   // that of the View returned by CreateContentsView should override this
   // appropriately.
   virtual gfx::Size GetPreferredSizeForContents() const;
 
-  // views::WidgetDelegate:
-  bool CanResize() const override;
-  views::View* GetContentsView() override;
-  const views::Widget* GetWidget() const override;
-  views::Widget* GetWidget() override;
+  // Invoke when done either because of failure or success. Quits the message
+  // loop.
+  void Done();
 
- protected:
-  ~ViewEventTestBase() override;
-
-  // Returns the view that is added to the window.
-  virtual views::View* CreateContentsView() = 0;
+  views::Widget* window() { return window_; }
 
+ protected:
   // Called once the message loop is running.
   virtual void DoTestOnMessageLoop() = 0;
 
@@ -123,34 +117,25 @@ class ViewEventTestBase : public views::WidgetDelegate, public testing::Test {
   // Returns a task runner to use for drag-related mouse events.
   scoped_refptr<base::SingleThreadTaskRunner> GetDragTaskRunner();
 
-  views::Widget* window_ = nullptr;
-
  private:
+  friend class TestBaseWidgetDelegate;
+
   // Callback from CreateEventTask. Runs the supplied task and if there are
   // failures invokes Done.
   void RunTestMethod(base::OnceClosure task);
 
-  // The content of the Window.
-  views::View* content_view_ = nullptr;
-
   // Thread for posting background drag events.
   std::unique_ptr<base::Thread> drag_event_thread_;
 
   content::BrowserTaskEnvironment task_environment_;
-
 #if defined(OS_WIN)
   ui::ScopedOleInitializer ole_initializer_;
 #endif
-
   std::unique_ptr<ui::TestContextFactories> context_factories_;
-
   std::unique_ptr<ViewEventTestPlatformPart> platform_part_;
-
   ChromeTestViewsDelegate<> views_delegate_;
-
   base::RunLoop run_loop_;
-
-  DISALLOW_COPY_AND_ASSIGN(ViewEventTestBase);
+  views::Widget* window_ = nullptr;
 };
 
 // Convenience macro for defining a ViewEventTestBase. See class description

chrome/browser/ui/views/touch_events_interactive_uitest_win.cc

@@ -163,7 +163,7 @@ class TouchEventsViewTest : public ViewEventTestBase {
 
     const int touch_pointer_count = 3;
     TouchEventHandler touch_event_handler;
-    GetWidget()->GetNativeWindow()->GetHost()->window()->AddPreTargetHandler(
+    window()->GetNativeWindow()->GetHost()->window()->AddPreTargetHandler(
         &touch_event_handler);
     gfx::Point in_content(touch_view_->width() / 2, touch_view_->height() / 2);
     views::View::ConvertPointToScreen(touch_view_, &in_content);
@@ -181,7 +181,7 @@ class TouchEventsViewTest : public ViewEventTestBase {
     EXPECT_EQ(touch_pointer_count,
               gesture_recognizer_->num_touch_release_events());
 
-    GetWidget()->GetNativeWindow()->GetHost()->window()->RemovePreTargetHandler(
+    window()->GetNativeWindow()->GetHost()->window()->RemovePreTargetHandler(
         &touch_event_handler);
     Done();
   }
@@ -210,7 +210,7 @@ class TouchEventsRecursiveViewTest : public TouchEventsViewTest {
 
     const int touch_pointer_count = 1;
     TouchEventHandler touch_event_handler;
-    GetWidget()->GetNativeWindow()->GetHost()->window()->AddPreTargetHandler(
+    window()->GetNativeWindow()->GetHost()->window()->AddPreTargetHandler(
         &touch_event_handler);
     gfx::Point in_content(touch_view_->width() / 2, touch_view_->height() / 2);
     views::View::ConvertPointToScreen(touch_view_, &in_content);
@@ -224,7 +224,7 @@ class TouchEventsRecursiveViewTest : public TouchEventsViewTest {
     EXPECT_EQ(touch_pointer_count + 1, touch_event_handler.num_touch_presses());
     EXPECT_EQ(0, touch_event_handler.num_pointers_down());
     EXPECT_EQ(2, touch_event_handler.max_call_depth());
-    GetWidget()->GetNativeWindow()->GetHost()->window()->RemovePreTargetHandler(
+    window()->GetNativeWindow()->GetHost()->window()->RemovePreTargetHandler(
         &touch_event_handler);
     Done();
   }