Add a unit test for BrowserViewLayout

This is the first of several steps.
* Decouple Browser and BrowserView initialization
* Dependency inject Browser into BrowserViewLayout
* Modify BrowserWithTestWindowTest to allow it to use a BrowserView as
  its BrowserWindow
* Add some trivial assertions

BUG=233374
TEST=added unit_tests BrowserViewLayout.*

Review URL: https://codereview.chromium.org/14348011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@195301 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/views/frame/browser_view.cc

@@ -380,12 +380,11 @@ void BookmarkExtensionBackground::Paint(gfx::Canvas* canvas,
 ///////////////////////////////////////////////////////////////////////////////
 // BrowserView, public:
 
-BrowserView::BrowserView(Browser* browser)
+BrowserView::BrowserView()
     : views::ClientView(NULL, NULL),
       last_focused_view_storage_id_(
           views::ViewStorage::GetInstance()->CreateStorageID()),
       frame_(NULL),
-      browser_(browser),
       top_container_(NULL),
       tabstrip_(NULL),
       toolbar_(NULL),
@@ -407,7 +406,6 @@ BrowserView::BrowserView(Browser* browser)
       immersive_mode_controller_(chrome::CreateImmersiveModeController()),
       ALLOW_THIS_IN_INITIALIZER_LIST(color_change_listener_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(activate_modal_dialog_factory_(this)) {
-  browser_->tab_strip_model()->AddObserver(this);
 }
 
 BrowserView::~BrowserView() {
@@ -462,6 +460,11 @@ BrowserView::~BrowserView() {
   browser_.reset();
 }
 
+void BrowserView::Init(Browser* browser) {
+  browser_.reset(browser);
+  browser_->tab_strip_model()->AddObserver(this);
+}
+
 // static
 BrowserView* BrowserView::GetBrowserViewForNativeWindow(
     gfx::NativeWindow window) {
@@ -1843,7 +1846,7 @@ void BrowserView::ViewHierarchyChanged(bool is_add,
                                        views::View* parent,
                                        views::View* child) {
   if (!initialized_ && is_add && child == this && GetWidget()) {
-    Init();
+    InitViews();
     initialized_ = true;
   }
 }
@@ -1935,10 +1938,10 @@ void BrowserView::OnSysColorChange() {
   chrome::MaybeShowInvertBubbleView(browser_.get(), contents_container_);
 }
 
-void BrowserView::Init() {
+void BrowserView::InitViews() {
   GetWidget()->AddObserver(this);
 
-  SetLayoutManager(new BrowserViewLayout);
+  SetLayoutManager(new BrowserViewLayout(browser()));
   // Stow a pointer to this object onto the window handle so that we can get at
   // it later when all we have is a native view.
   GetWidget()->SetNativeWindowProperty(kBrowserViewKey, this);
@@ -2527,7 +2530,8 @@ void BrowserView::CreateLauncherIcon() {
 BrowserWindow* BrowserWindow::CreateBrowserWindow(Browser* browser) {
   // Create the view and the frame. The frame will attach itself via the view
   // so we don't need to do anything with the pointer.
-  BrowserView* view = new BrowserView(browser);
+  BrowserView* view = new BrowserView();
+  view->Init(browser);
   (new BrowserFrame(view))->InitBrowserFrame();
   view->GetWidget()->non_client_view()->SetAccessibleName(
       l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));

chrome/browser/ui/views/frame/browser_view.h

@@ -103,9 +103,12 @@ class BrowserView : public BrowserWindow,
   // The browser view's class name.
   static const char kViewClassName[];
 
-  explicit BrowserView(Browser* browser);
+  BrowserView();
   virtual ~BrowserView();
 
+  // Takes ownership of |browser|.
+  void Init(Browser* browser);
+
   void set_frame(BrowserFrame* frame) { frame_ = frame; }
   BrowserFrame* frame() const { return frame_; }
 
@@ -473,8 +476,8 @@ class BrowserView : public BrowserWindow,
   // can be traversed using F6, in the order they should be traversed.
   void GetAccessiblePanes(std::vector<views::AccessiblePaneView*>* panes);
 
-  // Browser window related initializations.
-  void Init();
+  // Constructs and initializes the child views.
+  void InitViews();
 
   // Callback for the loading animation(s) associated with this view.
   void LoadingAnimationCallback();

chrome/browser/ui/views/frame/browser_view_layout.cc

@@ -106,8 +106,9 @@ const int BrowserViewLayout::kToolbarTabStripVerticalOverlap = 3;
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserViewLayout, public:
 
-BrowserViewLayout::BrowserViewLayout()
-    : contents_split_(NULL),
+BrowserViewLayout::BrowserViewLayout(Browser* browser)
+    : browser_(browser),
+      contents_split_(NULL),
       contents_container_(NULL),
       download_shelf_(NULL),
       browser_view_(NULL),
@@ -391,14 +392,6 @@ gfx::Size BrowserViewLayout::GetPreferredSize(views::View* host) {
 //////////////////////////////////////////////////////////////////////////////
 // BrowserViewLayout, private:
 
-Browser* BrowserViewLayout::browser() {
-  return browser_view_->browser();
-}
-
-const Browser* BrowserViewLayout::browser() const {
-  return browser_view_->browser();
-}
-
 int BrowserViewLayout::LayoutTabStripRegion() {
   TabStrip* tabstrip = browser_view_->tabstrip_;
   if (!browser_view_->IsTabStripVisible()) {
@@ -634,6 +627,6 @@ int BrowserViewLayout::LayoutDownloadShelf(int bottom) {
 bool BrowserViewLayout::InfobarVisible() const {
   views::View* infobar_container = browser_view_->infobar_container_;
   // NOTE: Can't check if the size IsEmpty() since it's always 0-width.
-  return browser()->SupportsWindowFeature(Browser::FEATURE_INFOBAR) &&
+  return browser_->SupportsWindowFeature(Browser::FEATURE_INFOBAR) &&
       (infobar_container->GetPreferredSize().height() != 0);
 }

chrome/browser/ui/views/frame/browser_view_layout.h

@@ -7,6 +7,7 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "ui/gfx/rect.h"
 #include "ui/views/layout/layout_manager.h"
@@ -36,7 +37,7 @@ class BrowserViewLayout : public views::LayoutManager {
   // The vertical overlap between the TabStrip and the Toolbar.
   static const int kToolbarTabStripVerticalOverlap;
 
-  BrowserViewLayout();
+  explicit BrowserViewLayout(Browser* browser);
   virtual ~BrowserViewLayout();
 
   WebContentsModalDialogHost* GetWebContentsModalDialogHost();
@@ -65,6 +66,8 @@ class BrowserViewLayout : public views::LayoutManager {
   virtual gfx::Size GetPreferredSize(views::View* host) OVERRIDE;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, BrowserViewLayout);
+  FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, Layout);
   class WebContentsModalDialogHostViews;
 
   enum InstantUIState {
@@ -77,8 +80,7 @@ class BrowserViewLayout : public views::LayoutManager {
     kInstantUIFullPageResults,
   };
 
-  Browser* browser();
-  const Browser* browser() const;
+  Browser* browser() { return browser_; }
 
   // Layout the tab strip region, returns the coordinate of the bottom of the
   // TabStrip, for laying out subsequent controls.
@@ -125,6 +127,9 @@ class BrowserViewLayout : public views::LayoutManager {
   // Returns true if an infobar is showing.
   bool InfobarVisible() const;
 
+  // The browser from the owning BrowserView.
+  Browser* browser_;
+
   // Child views that the layout manager manages.
   views::SingleSplitView* contents_split_;
   ContentsContainer* contents_container_;

chrome/browser/ui/views/frame/browser_view_layout_unittest.cc

+// Copyright 2013 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 "chrome/browser/ui/views/frame/browser_view_layout.h"
+
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// Tests of the LayoutManger for BrowserView.
+class BrowserViewLayoutTest : public BrowserWithTestWindowTest {
+ public:
+  BrowserViewLayoutTest() : layout_(NULL) {}
+  virtual ~BrowserViewLayoutTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    // BrowserWithTestWindowTest takes ownership of BrowserView via |window_|.
+    BrowserView* browser_view = new BrowserView;
+    set_window(browser_view);
+
+    // Creates a Browser using |browser_view| as its BrowserWindow.
+    BrowserWithTestWindowTest::SetUp();
+
+    // Memory ownership is tricky here. BrowserView needs ownership of
+    // |browser|, so BrowserWithTestWindowTest cannot continue to own it.
+    Browser* browser = release_browser();
+    browser_view->Init(browser);
+
+    // BrowserView also takes ownership of |layout_|.
+    layout_ = new BrowserViewLayout(browser);
+    browser_view->SetLayoutManager(layout_);
+  }
+
+  BrowserViewLayout* layout() { return layout_; }
+
+ private:
+  BrowserViewLayout* layout_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserViewLayoutTest);
+};
+
+// Test basic construction and initialization.
+TEST_F(BrowserViewLayoutTest, BrowserViewLayout) {
+  EXPECT_TRUE(layout()->browser());
+  EXPECT_TRUE(layout()->GetWebContentsModalDialogHost());
+  EXPECT_EQ(BrowserViewLayout::kInstantUINone, layout()->GetInstantUIState());
+  // TODO(jamescook): Add more as we break dependencies.
+}
+
+// Test the core layout functions.
+TEST_F(BrowserViewLayoutTest, Layout) {
+  // We don't have a bookmark bar yet, so no contents offset is required.
+  EXPECT_EQ(0, layout()->GetContentsOffsetForBookmarkBar());
+  EXPECT_EQ(0, layout()->GetTopMarginForActiveContent());
+  EXPECT_EQ(0, layout()->GetTopMarginForOverlayContent());
+  // TODO(jamescook): Add more as we break dependencies.
+}

chrome/chrome_tests_unit.gypi

@@ -1449,6 +1449,7 @@
         'browser/ui/views/crypto_module_password_dialog_view_unittest.cc',
         'browser/ui/views/extensions/browser_action_drag_data_unittest.cc',
         'browser/ui/views/first_run_bubble_unittest.cc',
+        'browser/ui/views/frame/browser_view_layout_unittest.cc',
         'browser/ui/views/reload_button_unittest.cc',
         'browser/ui/views/select_file_dialog_extension_unittest.cc',
         'browser/ui/views/status_icons/status_tray_win_unittest.cc',

chrome/test/base/browser_with_test_window_test.h

@@ -72,8 +72,8 @@ class BrowserWithTestWindowTest : public testing::Test {
   virtual void TearDown() OVERRIDE;
 
  protected:
-  TestBrowserWindow* window() const { return window_.get(); }
-  void set_window(TestBrowserWindow* window) {
+  BrowserWindow* window() const { return window_.get(); }
+  void set_window(BrowserWindow* window) {
     window_.reset(window);
   }
 
@@ -81,6 +81,9 @@ class BrowserWithTestWindowTest : public testing::Test {
   void set_browser(Browser* browser) {
     browser_.reset(browser);
   }
+  Browser* release_browser() WARN_UNUSED_RESULT {
+    return browser_.release();
+  }
 
   TestingProfile* profile() const { return profile_.get(); }
   void set_profile(TestingProfile* profile);
@@ -129,7 +132,7 @@ class BrowserWithTestWindowTest : public testing::Test {
   content::TestBrowserThread file_user_blocking_thread_;
 
   scoped_ptr<TestingProfile> profile_;
-  scoped_ptr<TestBrowserWindow> window_;
+  scoped_ptr<BrowserWindow> window_;  // Usually a TestBrowserWindow.
   scoped_ptr<Browser> browser_;
 
   // The existence of this object enables tests via