Fix the browser match rules.

chrome/browser/ui/ash/window_positioner_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/wm/window_positioner.h"
 
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/test/test_shell_delegate.h"
 #include "ash/wm/window_resizer.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/test/base/test_browser_window.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/render_view_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/aura/env.h"
 #include "ui/aura/test/test_windows.h"
 #include "ui/aura/window_event_dispatcher.h"
 #include "ui/gfx/screen.h"
 
 namespace ash {
 namespace test {
 
-namespace {
-
-// A browser window proxy which is able to associate an aura native window with
-// it.
-class TestBrowserWindowAura : public TestBrowserWindow {
- public:
-  explicit TestBrowserWindowAura(aura::Window* native_window);
-  ~TestBrowserWindowAura() override;
-
-  gfx::NativeWindow GetNativeWindow() const override { return native_window_; }
-
- private:
-  gfx::NativeWindow native_window_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestBrowserWindowAura);
-};
-
-TestBrowserWindowAura::TestBrowserWindowAura(aura::Window *native_window)
-    : native_window_(native_window) {
-}
-
-TestBrowserWindowAura::~TestBrowserWindowAura() {}
-
-}  // namespace
-
 // A test class for preparing window positioner tests - it creates a testing
 // base by adding a window and a popup which can be independently
 // positioned to see where the positioner will place the window.
 class WindowPositionerTest : public AshTestBase {
  public:
   WindowPositionerTest();
 
   void SetUp() override;
   void TearDown() override;
 
  protected:
-  aura::Window* window() { return window_.get(); }
-  aura::Window* popup() { return popup_.get(); }
-
-  Browser* window_browser() { return window_owning_browser_.get(); }
-  Browser* popup_browser() { return popup_owning_browser_.get(); }
+  aura::Window* window() { return browser_window_->GetNativeWindow(); }
+  aura::Window* popup() { return browser_popup_->GetNativeWindow(); }
 
   WindowPositioner* window_positioner() { return window_positioner_; }
 
   // The positioner & desktop's used grid alignment size.
   const int grid_size_;
 
  private:
   WindowPositioner* window_positioner_;

[msw, 2015/07/07 18:11:54]

nit: make this a scoped_ptr too, please

[xdai1, 2015/07/08 00:26:21]

Done.

 
   // These two need to be deleted after everything else is gone.
   TestingProfile profile_;
 
-  // These get created for each session.
-  scoped_ptr<aura::Window> window_;
-  scoped_ptr<aura::Window> popup_;
-
   scoped_ptr<BrowserWindow> browser_window_;
   scoped_ptr<BrowserWindow> browser_popup_;
 
-  scoped_ptr<Browser> window_owning_browser_;
-  scoped_ptr<Browser> popup_owning_browser_;
-
   DISALLOW_COPY_AND_ASSIGN(WindowPositionerTest);
 };
 
 WindowPositionerTest::WindowPositionerTest()
     : grid_size_(WindowPositioner::kMinimumWindowOffset),
-      window_positioner_(NULL) {
+      window_positioner_(nullptr) {
 }
 
 void WindowPositionerTest::SetUp() {
   AshTestBase::SetUp();
   // Create some default dummy windows.
-  window_.reset(CreateTestWindowInShellWithId(0));
-  window_->SetBounds(gfx::Rect(16, 32, 640, 320));
-  popup_.reset(CreateTestWindowInShellWithId(1));
-  popup_->SetBounds(gfx::Rect(16, 32, 128, 256));
+  aura::Window* window = CreateTestWindowInShellWithId(0);

[msw, 2015/07/07 18:11:54]

These should still be scoped_ptrs.

[xdai1, 2015/07/08 00:26:21]

It seems |window| is owned by TestBrowserWindowAura class, so the life time of
|window| will be taken care by TestBrowserWindowAura? 

[msw, 2015/07/08 01:13:59]

Hmm, that does seem to be the case. The transfer of ownership would be clearer
here if the constructor took a scoped_ptr<aura::Window>, but maybe it's fine
as-is for now; wdyt? Also, it'd be nice if the ctor had fewer callers and tests
used CreateBrowserWithAuraTestWindowForParams instead.

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

Yes, I agree it would be more clearer: modifying the constructor to take a
scoped_ptr<aura::Window> and transfer ownership here. 

But we can't use CreateBrowserWithAuraTestWindowForParams here because in this
test the aura window is created by CreateTestWindowInShellWithId. Same reason
for WindowSizerAshTest. I think we could create another function accepting a
window as an argument like
CreateBrowserForAuraTestWindowAndParams(scoped_ptr<aura::Window>, const
Browser::CreateParams&) to avoid explicitly calling the ctor, wdyt?

+  window->SetBounds(gfx::Rect(16, 32, 640, 320));
+  aura::Window* popup = CreateTestWindowInShellWithId(1);
+  popup->SetBounds(gfx::Rect(16, 32, 128, 256));
 
   // Create a browser for the window.
-  browser_window_.reset(new TestBrowserWindowAura(window_.get()));
+  browser_window_.reset(new TestBrowserWindowAura(window));
   Browser::CreateParams window_params(&profile_,
                                       chrome::HOST_DESKTOP_TYPE_ASH);
   window_params.window = browser_window_.get();
-  window_owning_browser_.reset(new Browser(window_params));
 
   // Creating a browser for the popup.
-  browser_popup_.reset(new TestBrowserWindowAura(popup_.get()));
+  browser_popup_.reset(new TestBrowserWindowAura(popup));
   Browser::CreateParams popup_params(Browser::TYPE_POPUP, &profile_,
                                      chrome::HOST_DESKTOP_TYPE_ASH);
   popup_params.window = browser_popup_.get();
-  popup_owning_browser_.reset(new Browser(popup_params));
 
   // We hide all windows upon start - each user is required to set it up
   // as he needs it.
-  window()->Hide();
-  popup()->Hide();
+  window->Hide();
+  popup->Hide();
   window_positioner_ = new WindowPositioner();
 }
 
 void WindowPositionerTest::TearDown() {
   // Since the AuraTestBase is needed to create our assets, we have to
   // also delete them before we tear it down.
-  window_owning_browser_.reset(NULL);
-  popup_owning_browser_.reset(NULL);
-
-  browser_window_.reset(NULL);
-  browser_popup_.reset(NULL);
-
-  window_.reset(NULL);
-  popup_.reset(NULL);
-
+  browser_window_.reset(nullptr);
+  browser_popup_.reset(nullptr);
   AshTestBase::TearDown();
   delete window_positioner_;
-  window_positioner_ = NULL;
+  window_positioner_ = nullptr;
 }
 
 int AlignToGridRoundDown(int location, int grid_size) {
   if (grid_size <= 1 || location % grid_size == 0)
     return location;
   return location / grid_size * grid_size;
 }
 
 TEST_F(WindowPositionerTest, cascading) {
   const gfx::Rect work_area =
@@ skipping 119 matching lines @@
 
   // Check that the popup is placed full screen.
   gfx::Rect full = window_positioner()->GetPopupPosition(pop_position);
   EXPECT_EQ(gfx::Rect(work_area.x(), work_area.y(),
                       pop_position.width(), pop_position.height()),
                       full);
 }
 
 }  // namespace test
 }  // namespace ash