Fix the browser match rules.

chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/scoped_target_root_window.h"
 #include "ash/screen_util.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/test/test_shell_delegate.h"
 #include "ash/wm/window_positioner.h"
@@ skipping 11 matching lines @@
 #include "ui/aura/env.h"
 #include "ui/aura/test/test_windows.h"
 #include "ui/aura/window_event_dispatcher.h"
 #include "ui/gfx/screen.h"
 #include "ui/wm/public/activation_client.h"
 
 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,
     const Browser::CreateParams& params) {
   if (!bounds.IsEmpty())
     window->SetBounds(bounds);
   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();
 }
 
 }  // namespace
 
 // On desktop linux aura, we currently don't use the ash frame, breaking some
 // tests which expect ash sizes: http://crbug.com/303862
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
 #define MAYBE_DefaultSizeCase DISABLED_DefaultSizeCase
 #else
@@ skipping 373 matching lines @@
   scoped_ptr<TestingProfile> profile(new TestingProfile());
   // Creating a popup handler here to make sure it does not interfere with the
   // existing windows.
   Browser::CreateParams native_params(profile.get(),
                                       chrome::HOST_DESKTOP_TYPE_ASH);
   scoped_ptr<Browser> browser(
       chrome::CreateBrowserWithTestWindowForParams(&native_params));
 
   // 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();
   { // Make sure that popups do not get changed.
     gfx::Rect window_bounds;
     GetWindowBounds(p1600x1200, p1600x1200, gfx::Rect(),
                     gfx::Rect(50, 100, 300, 150), bottom_s1600x1200,
                     PERSISTED, browser_popup->browser(),
                     gfx::Rect(), &window_bounds);
     EXPECT_EQ("50,100 300x150", window_bounds.ToString());
@@ skipping 18 matching lines @@
     EXPECT_EQ(
         gfx::Rect(
             std::max(ash::WindowPositioner::kDesktopBorderSize,
                      (1600 - ash::WindowPositioner::kMaximumWindowWidth) / 2),
             ash::WindowPositioner::kDesktopBorderSize,
             std::min(ash::WindowPositioner::kMaximumWindowWidth,
                      1600 - 2 * ash::WindowPositioner::kDesktopBorderSize),
             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
 // tests which expect ash sizes: http://crbug.com/303862
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
 #define MAYBE_PlaceNewBrowserWindowOnEmptyDesktop DISABLED_PlaceNewBrowserWindowOnEmptyDesktop
 #else
 #define MAYBE_PlaceNewBrowserWindowOnEmptyDesktop PlaceNewBrowserWindowOnEmptyDesktop
 #endif
 
@@ skipping 84 matching lines @@
   gfx::Rect primary_bounds = ash::Shell::GetInstance()->GetScreen()->
       GetPrimaryDisplay().bounds();
   gfx::Rect secondary_bounds = ash::ScreenUtil::GetSecondaryDisplay().bounds();
 
   ash::Shell::GetInstance()->set_target_root_window(
       ash::Shell::GetPrimaryRootWindow());
 
   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());
+  EXPECT_EQ(native_window->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)));
+  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(),
             ash::Shell::GetTargetRootWindow());
 
   // First new window should be in the primary.
   {
     gfx::Rect window_bounds;
     GetWindowBounds(p1600x1200, p1600x1200, secondary_bounds,
                     gfx::Rect(), secondary_bounds,
                     PERSISTED, new_browser_window->browser(),
                     gfx::Rect(), &window_bounds);
     // TODO(oshima): Use exact bounds when the window_sizer_ash is
     // moved to ash and changed to include the result from
     // RearrangeVisibleWindowOnShow.
     EXPECT_TRUE(primary_bounds.Contains(window_bounds));
   }
 
   // Move the window to the right side of the secondary display and create a new
   // window. It should be opened then on the secondary display.
   {
     gfx::Display second_display = ash::Shell::GetScreen()->
         GetDisplayNearestPoint(gfx::Point(1600 + 100,10));
     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;
     GetWindowBounds(p1600x1200, p1600x1200, secondary_bounds,
                     gfx::Rect(), secondary_bounds,
                     PERSISTED, new_browser_window->browser(),
                     gfx::Rect(), &window_bounds);
     // TODO(oshima): Use exact bounds when the window_sizer_ash is
     // moved to ash and changed to include the result from
     // RearrangeVisibleWindowOnShow.
     EXPECT_TRUE(secondary_bounds.Contains(window_bounds));
   }
 
   // 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());
 
     gfx::Rect window_bounds;
     GetWindowBounds(p1600x1200, p1600x1200, secondary_bounds,
                     gfx::Rect(), secondary_bounds,
                     PERSISTED, new_browser_window->browser(),
                     gfx::Rect(), &window_bounds);
     // TODO(oshima): Use exact bounds when the window_sizer_ash is
     // moved to ash and changed to include the result from
     // 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
 // tests which expect ash sizes: http://crbug.com/303862
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
 #define MAYBE_TestShowState DISABLED_TestShowState
 #else
 #define MAYBE_TestShowState TestShowState
 #endif
 
 // Test that the show state is properly returned for non default cases.
 TEST_F(WindowSizerAshTest, MAYBE_TestShowState) {
   scoped_ptr<TestingProfile> profile(new TestingProfile());
 
   // 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)));
 
   // Tabbed windows should retrieve the saved window state - since there is a
   // top window.
   EXPECT_EQ(ui::SHOW_STATE_MAXIMIZED,
             GetWindowShowState(ui::SHOW_STATE_MAXIMIZED,
                                ui::SHOW_STATE_NORMAL,
                                BOTH,
                                browser_window->browser(),
@@ skipping 29 matching lines @@
                                ui::SHOW_STATE_NORMAL,
                                BOTH,
                                browser_popup->browser(),
                                p1600x1200,
                                p1600x1200));
 
   // Now create a top level window and check again for both. Only the tabbed
   // 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)));
 
   // A tabbed window should now take the top level window state.
   EXPECT_EQ(ui::SHOW_STATE_DEFAULT,
             GetWindowShowState(ui::SHOW_STATE_MAXIMIZED,
                                ui::SHOW_STATE_DEFAULT,
                                BOTH,
                                browser_window->browser(),
                                p1600x1200,
@@ skipping 22 matching lines @@
     browser_window->Hide();
     EXPECT_EQ(ui::SHOW_STATE_MAXIMIZED,
               GetWindowShowState(ui::SHOW_STATE_MAXIMIZED,
                                  ui::SHOW_STATE_DEFAULT,
                                  BOTH,
                                  browser_window2->browser(),
                                  tiny_screen,
                                  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.
 TEST_F(WindowSizerAshTest, TestShowStateDefaults) {
   // Creating a browser & window to play with.
   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)));
 
   // Check that a browser creation state always get used if not given as
   // SHOW_STATE_DEFAULT. On Windows ASH it should be SHOW_STATE_MAXIMIZED.
   ui::WindowShowState window_show_state =
       GetWindowShowState(ui::SHOW_STATE_MAXIMIZED,
                          ui::SHOW_STATE_MAXIMIZED,
                          DEFAULT,
                          browser_window->browser(),
@@ skipping 40 matching lines @@
                                p1600x1200,
                                p1600x1200), ui::SHOW_STATE_MAXIMIZED);
 
   // The popup should favor the initial show state over the command line.
   EXPECT_EQ(GetWindowShowState(ui::SHOW_STATE_NORMAL,
                                ui::SHOW_STATE_NORMAL,
                                BOTH,
                                browser_popup->browser(),
                                p1600x1200,
                                p1600x1200), ui::SHOW_STATE_NORMAL);
+
+  // Explicitly delete browsers.
+  delete browser_window->browser();
+  delete browser_popup->browser();
 }
 
 TEST_F(WindowSizerAshTest, DefaultStateBecomesMaximized) {
   // Create a browser to pass into the GetWindowBounds function.
   scoped_ptr<TestingProfile> profile(new TestingProfile());
   Browser::CreateParams native_params(profile.get(),
                                       chrome::HOST_DESKTOP_TYPE_ASH);
   scoped_ptr<Browser> browser(
       chrome::CreateBrowserWithTestWindowForParams(&native_params));
 
@@ skipping 60 matching lines @@
   }
 }
 
 TEST_F(WindowSizerAshTest, TrustedPopupBehavior) {
   scoped_ptr<TestingProfile> profile(new TestingProfile());
   Browser::CreateParams trusted_popup_create_params(
       Browser::TYPE_POPUP, profile.get(), chrome::HOST_DESKTOP_TYPE_ASH);
   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.
   EXPECT_EQ(ui::SHOW_STATE_DEFAULT,
             GetWindowShowState(ui::SHOW_STATE_DEFAULT,
                                ui::SHOW_STATE_NORMAL,
                                BOTH,
                                trusted_popup->browser(),
                                p1280x1024,
                                p1600x1200));
   // A popup that is sized to occupy the whole work area has default state.
   EXPECT_EQ(ui::SHOW_STATE_DEFAULT,
             GetWindowShowState(ui::SHOW_STATE_DEFAULT,
                                ui::SHOW_STATE_NORMAL,
                                BOTH,
                                trusted_popup->browser(),
                                p1600x1200,
                                p1600x1200));
+
+  // Explicitly delete browser.
+  delete trusted_popup->browser();
 }