Fix the browser match rules.

chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc

Index: chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
diff --git a/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc b/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
index bcfdd4fd0db929d3f96e1165b6141a43fb8c77a0..f41b302d965a4c874308a12d3a1480328d75a9ed 100644
--- a/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
+++ b/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
@@ -29,51 +29,6 @@ typedef ash::test::AshTestBase WindowSizerAshTest;
 
 namespace {
 
-// A browser window proxy which is able to associate an aura native window with
-// it.
-class TestBrowserWindowAura : public TestBrowserWindow {
- public:
-  // |native_window| will still be owned by the caller after the constructor
-  // was called.
-  explicit TestBrowserWindowAura(aura::Window* native_window)
-      : native_window_(native_window) {
-  }
-  ~TestBrowserWindowAura() override {}
-
-  // TestBrowserWindow overrides:
-  void Show() override {
-    native_window_->Show();
-    Activate();
-  }
-  void Hide() override { native_window_->Hide(); }
-  void Activate() override {
-    aura::client::GetActivationClient(
-        native_window_->GetRootWindow())->ActivateWindow(native_window_.get());
-  }
-  gfx::NativeWindow GetNativeWindow() const override {
-    return native_window_.get();
-  }
-  gfx::Rect GetBounds() const override { return native_window_->bounds(); }
-
-  Browser* browser() { return browser_.get(); }
-
-  void CreateBrowser(const Browser::CreateParams& params) {
-    Browser::CreateParams create_params = params;
-    create_params.window = this;
-    browser_.reset(new Browser(create_params));
-    if (browser_->is_type_tabbed() || browser_->is_app()) {
-      ash::wm::GetWindowState(native_window_.get())->
-          set_window_position_managed(true);
-    }
-  }
-
- private:
-  scoped_ptr<Browser> browser_;
-  scoped_ptr<aura::Window> native_window_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestBrowserWindowAura);
-};
-
 scoped_ptr<TestBrowserWindowAura> CreateTestBrowserWindow(
     aura::Window* window,
     const gfx::Rect& bounds,
@@ -83,6 +38,11 @@ scoped_ptr<TestBrowserWindowAura> CreateTestBrowserWindow(
   scoped_ptr<TestBrowserWindowAura> browser_window(
       new TestBrowserWindowAura(window));
   browser_window->CreateBrowser(params);
+  if (browser_window->browser()->is_type_tabbed() ||
+      browser_window->browser()->is_app()) {
+    ash::wm::GetWindowState(browser_window->GetNativeWindow())
+        ->set_window_position_managed(true);
+  }
   return browser_window.Pass();
 }
 
@@ -476,22 +436,19 @@ TEST_F(WindowSizerAshTest, MAYBE_PlaceNewWindows) {
 
   // Creating a popup handler here to make sure it does not interfere with the
   // existing windows.
-  scoped_ptr<BrowserWindow> browser_window(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(0),
-      gfx::Rect(16, 32, 640, 320),
+  scoped_ptr<TestBrowserWindowAura> browser_window(CreateTestBrowserWindow(
+      CreateTestWindowInShellWithId(0), gfx::Rect(16, 32, 640, 320),
       Browser::CreateParams(profile.get(), chrome::HOST_DESKTOP_TYPE_ASH)));
 
   // Creating a popup to make sure it does not interfere with the positioning.
   scoped_ptr<TestBrowserWindowAura> browser_popup(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(1),
-      gfx::Rect(16, 32, 128, 256),
+      CreateTestWindowInShellWithId(1), gfx::Rect(16, 32, 128, 256),
       Browser::CreateParams(Browser::TYPE_POPUP, profile.get(),
                             chrome::HOST_DESKTOP_TYPE_ASH)));
 
   // Creating a panel to make sure it does not interfere with the positioning.
-  scoped_ptr<BrowserWindow> browser_panel(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(2),
-      gfx::Rect(32, 48, 256, 512),
+  scoped_ptr<TestBrowserWindowAura> browser_panel(CreateTestBrowserWindow(
+      CreateTestWindowInShellWithId(2), gfx::Rect(32, 48, 256, 512),
       Browser::CreateParams(Browser::TYPE_POPUP, profile.get(),
                             chrome::HOST_DESKTOP_TYPE_ASH)));
   browser_window->Show();
@@ -530,6 +487,12 @@ TEST_F(WindowSizerAshTest, MAYBE_PlaceNewWindows) {
             1200 - ash::WindowPositioner::kDesktopBorderSize).ToString(),
         window_bounds.ToString());
   }
+
+  // Explicitly delete browsers since they're not owned by

[msw, 2015/07/08 01:14:00]

Hmm, should TestBrowserWindowAura own its browsers, as the old implementations
did? Regardless, CreateTestBrowserWindow should return a scoped_ptr<Browser>, so
these callers can then manage the Browser lifetimes with local scoped_ptrs.

[xdai1, 2015/07/08 21:34:21]

This is a little tricky here. Since we modified
CreateBrowserWithAuraTestWindowForParams to return a scoped_ptr<Browser> but not
a scoped_ptr<TestBrowserWindowAura>, so the life time of TestBrowserWindowAura
will be managed by TestBrowserWindowOwner (see test_browser_window.cc file),
which is a BrowserListObserver. TestBrowserWindowOwner will listen to
OnBrowserRemoved() event and delete the corresponding TestBrowserWindowAura. So
if TestBrowserWindowAura own its browser, it will cause problems.

As you suggested, I modified CreateBrowserWithTestWindowForParams,
CreateBrowserWithAuraTestWindowForParams to return scoped_ptr<Browser>.

Also introduced a new function
CreateBrowserForAuraTestWindowAndParams(scoped_ptr<aura::Window>,Browser::CreateParams&)
to avoid deleting these browsers explicitly. Do you think it's necessory?

+  // TestBrowserWindowAura.
+  delete browser_window->browser();
+  delete browser_popup->browser();
+  delete browser_panel->browser();
 }
 
 // On desktop linux aura, we currently don't use the ash frame, breaking some
@@ -634,26 +597,25 @@ TEST_F(WindowSizerAshTest, MAYBE_PlaceNewWindowsOnMultipleDisplays) {
   scoped_ptr<TestingProfile> profile(new TestingProfile());
 
   // Create browser windows that are used as reference.
-  scoped_ptr<BrowserWindow> browser_window(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(0),
-      gfx::Rect(10, 10, 200, 200),
+  scoped_ptr<TestBrowserWindowAura> browser_window(CreateTestBrowserWindow(
+      CreateTestWindowInShellWithId(0), gfx::Rect(10, 10, 200, 200),
       Browser::CreateParams(profile.get(), chrome::HOST_DESKTOP_TYPE_ASH)));
+  gfx::NativeWindow native_window = browser_window->GetNativeWindow();
   browser_window->Show();
-  EXPECT_EQ(browser_window->GetNativeWindow()->GetRootWindow(),
-            ash::Shell::GetTargetRootWindow());
-
-  scoped_ptr<BrowserWindow> another_browser_window(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(1),
-      gfx::Rect(400, 10, 300, 300),
-      Browser::CreateParams(profile.get(), chrome::HOST_DESKTOP_TYPE_ASH)));
+  EXPECT_EQ(native_window->GetRootWindow(), ash::Shell::GetTargetRootWindow());
+
+  scoped_ptr<TestBrowserWindowAura> another_browser_window(
+      CreateTestBrowserWindow(
+          CreateTestWindowInShellWithId(1), gfx::Rect(400, 10, 300, 300),
+          Browser::CreateParams(profile.get(), chrome::HOST_DESKTOP_TYPE_ASH)));
+  gfx::NativeWindow another_native_window =
+      another_browser_window->GetNativeWindow();
   another_browser_window->Show();
 
   // Creating a new window to verify the new placement.
   scoped_ptr<TestBrowserWindowAura> new_browser_window(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(0),
-      gfx::Rect(),
-      Browser::CreateParams(profile.get(),
-                            chrome::HOST_DESKTOP_TYPE_ASH)));
+      CreateTestWindowInShellWithId(0), gfx::Rect(),
+      Browser::CreateParams(profile.get(), chrome::HOST_DESKTOP_TYPE_ASH)));
 
   // Make sure the primary root is active.
   ASSERT_EQ(ash::Shell::GetPrimaryRootWindow(),
@@ -680,7 +642,8 @@ TEST_F(WindowSizerAshTest, MAYBE_PlaceNewWindowsOnMultipleDisplays) {
     browser_window->GetNativeWindow()->SetBoundsInScreen(
         gfx::Rect(secondary_bounds.CenterPoint().x() - 100, 10, 200, 200),
         second_display);
-    browser_window->Activate();
+    aura::client::GetActivationClient(native_window->GetRootWindow())
+        ->ActivateWindow(native_window);
     EXPECT_NE(ash::Shell::GetPrimaryRootWindow(),
               ash::Shell::GetTargetRootWindow());
     gfx::Rect window_bounds;
@@ -697,7 +660,8 @@ TEST_F(WindowSizerAshTest, MAYBE_PlaceNewWindowsOnMultipleDisplays) {
   // Activate another window in the primary display and create a new window.
   // It should be created in the primary display.
   {
-    another_browser_window->Activate();
+    aura::client::GetActivationClient(another_native_window->GetRootWindow())
+        ->ActivateWindow(another_native_window);
     EXPECT_EQ(ash::Shell::GetPrimaryRootWindow(),
               ash::Shell::GetTargetRootWindow());
 
@@ -711,6 +675,11 @@ TEST_F(WindowSizerAshTest, MAYBE_PlaceNewWindowsOnMultipleDisplays) {
     // RearrangeVisibleWindowOnShow.
     EXPECT_TRUE(primary_bounds.Contains(window_bounds));
   }
+
+  // Explicitly delete browsers.
+  delete browser_window->browser();
+  delete another_browser_window->browser();
+  delete new_browser_window->browser();
 }
 
 // On desktop linux aura, we currently don't use the ash frame, breaking some
@@ -727,15 +696,13 @@ TEST_F(WindowSizerAshTest, MAYBE_TestShowState) {
 
   // Creating a browser & window to play with.
   scoped_ptr<TestBrowserWindowAura> browser_window(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(0),
-      gfx::Rect(16, 32, 640, 320),
+      CreateTestWindowInShellWithId(0), gfx::Rect(16, 32, 640, 320),
       Browser::CreateParams(Browser::TYPE_TABBED, profile.get(),
                             chrome::HOST_DESKTOP_TYPE_ASH)));
 
   // Create also a popup browser since that behaves different.
   scoped_ptr<TestBrowserWindowAura> browser_popup(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(1),
-      gfx::Rect(16, 32, 640, 320),
+      CreateTestWindowInShellWithId(1), gfx::Rect(16, 32, 640, 320),
       Browser::CreateParams(Browser::TYPE_POPUP, profile.get(),
                             chrome::HOST_DESKTOP_TYPE_ASH)));
 
@@ -785,8 +752,7 @@ TEST_F(WindowSizerAshTest, MAYBE_TestShowState) {
   // window should follow the top level window's state.
   // Creating a browser & window to play with.
   scoped_ptr<TestBrowserWindowAura> browser_window2(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(3),
-      gfx::Rect(16, 32, 640, 320),
+      CreateTestWindowInShellWithId(3), gfx::Rect(16, 32, 640, 320),
       Browser::CreateParams(Browser::TYPE_TABBED, profile.get(),
                             chrome::HOST_DESKTOP_TYPE_ASH)));
 
@@ -829,6 +795,11 @@ TEST_F(WindowSizerAshTest, MAYBE_TestShowState) {
                                  tiny_screen));
 
   }
+
+  // Explictly delete browsers.
+  delete browser_window->browser();
+  delete browser_popup->browser();
+  delete browser_window2->browser();
 }
 
 // Test that the default show state override behavior is properly handled.
@@ -837,16 +808,14 @@ TEST_F(WindowSizerAshTest, TestShowStateDefaults) {
   scoped_ptr<TestingProfile> profile(new TestingProfile());
 
   scoped_ptr<TestBrowserWindowAura> browser_window(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(0),
-      gfx::Rect(16, 32, 640, 320),
+      CreateTestWindowInShellWithId(0), gfx::Rect(16, 32, 640, 320),
       Browser::CreateParams(Browser::TYPE_TABBED, profile.get(),
                             chrome::HOST_DESKTOP_TYPE_ASH)));
 
   // Create also a popup browser since that behaves slightly different for
   // defaults.
   scoped_ptr<TestBrowserWindowAura> browser_popup(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(1),
-      gfx::Rect(16, 32, 128, 256),
+      CreateTestWindowInShellWithId(1), gfx::Rect(16, 32, 128, 256),
       Browser::CreateParams(Browser::TYPE_POPUP, profile.get(),
                             chrome::HOST_DESKTOP_TYPE_ASH)));
 
@@ -907,6 +876,10 @@ TEST_F(WindowSizerAshTest, TestShowStateDefaults) {
                                browser_popup->browser(),
                                p1600x1200,
                                p1600x1200), ui::SHOW_STATE_NORMAL);
+
+  // Explicitly delete browsers.
+  delete browser_window->browser();
+  delete browser_popup->browser();
 }
 
 TEST_F(WindowSizerAshTest, DefaultStateBecomesMaximized) {
@@ -987,8 +960,7 @@ TEST_F(WindowSizerAshTest, TrustedPopupBehavior) {
   trusted_popup_create_params.trusted_source = true;
 
   scoped_ptr<TestBrowserWindowAura> trusted_popup(CreateTestBrowserWindow(
-      CreateTestWindowInShellWithId(1),
-      gfx::Rect(16, 32, 640, 320),
+      CreateTestWindowInShellWithId(1), gfx::Rect(16, 32, 640, 320),
       trusted_popup_create_params));
   // Trusted popup windows should follow the saved show state and ignore the
   // last show state.
@@ -1007,4 +979,7 @@ TEST_F(WindowSizerAshTest, TrustedPopupBehavior) {
                                trusted_popup->browser(),
                                p1600x1200,
                                p1600x1200));
+
+  // Explicitly delete browser.
+  delete trusted_popup->browser();
 }