Unit test for BrowserViewLayout that does not depend on BrowserView

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

Index: chrome/browser/ui/views/frame/browser_view_layout.h
diff --git a/chrome/browser/ui/views/frame/browser_view_layout.h b/chrome/browser/ui/views/frame/browser_view_layout.h
index c9af58a9036033b3cca3d22b1f044a1dbe0de3e2..22cfcff51ec1e8c115196e87936059a261affb13 100644
--- a/chrome/browser/ui/views/frame/browser_view_layout.h
+++ b/chrome/browser/ui/views/frame/browser_view_layout.h
@@ -16,12 +16,12 @@ class BookmarkBarView;
 class Browser;
 class BrowserView;
 class ContentsContainer;
-class DownloadShelfView;
+class ImmersiveModeController;
 class InfoBarContainerView;
 class OverlayContainer;
 class TabContentsContainer;
 class TabStrip;
-class ToolbarView;
+class TopContainerView;
 class WebContentsModalDialogHost;
 
 namespace gfx {
@@ -33,6 +33,19 @@ namespace views {
 class SingleSplitView;
 }
 
+// Delegate class to allow BrowserViewLayout to be decoupled from BrowserView
+// for testing. Not a nested class so it can be forward declared.
+class BrowserViewLayoutDelegate {

[sky, 2013/05/15 00:37:22]

put in its own header.

[James Cook, 2013/05/15 16:45:39]

Done.

+ public:
+  virtual ~BrowserViewLayoutDelegate() {}

[sky, 2013/05/15 00:37:22]

nit: make protected as BrowserViewLayout doesn't own the delegate. Or make
BrowserViewLayout own the delegate since the delegate isn't implemented on an
existing object.

[James Cook, 2013/05/15 16:45:39]

I don't understand this suggestion. BrowserView owns an implementation of this
delegate via a scoped_ptr<>. It does that so I can use the same function names
in the delegate as exist in BrowserVIew (via BrowserWindow).  Is there a
particular advantage to having BrowserViewLayout own its delegate?

[sky, 2013/05/15 17:24:52]

Makes ownership simpler. BrowserView has and owns the delegate, but it never
uses it. Your test also now has to deal with ownership of the delegate as well.
If you instead make BrowserViewLayout own the delegate then none of the other
sites need to track ownership.

The typical pattern is to implement the delegate on an existing object, in which
case the consumer of the delegate BrowserViewLayout here) can't own the
delegate. That isn't the case with your patch since BrowserView can't directly
implement the delegate.

I think making BrowserViewLayout own the delegate simplifies things slightly,
but it isn't that typical. I'm fine with either of the following:
. make destructor protected.
. make BrowserViewLayout own the delegate.

+
+  virtual bool IsTabStripVisible() const = 0;
+  virtual gfx::Rect GetBoundsForTabStrip(views::View* tab_strip) const = 0;
+  virtual bool IsToolbarVisible() const = 0;
+  virtual bool IsBookmarkBarVisible() const = 0;
+  virtual bool DownloadShelfNeedsLayout() const = 0;
+};
+
 // The layout manager used in chrome browser.
 class BrowserViewLayout : public views::LayoutManager {
  public:
@@ -43,18 +56,30 @@ class BrowserViewLayout : public views::LayoutManager {
   virtual ~BrowserViewLayout();
 
   // Sets all the views to be managed. Tests may inject stubs or NULL.
+  // Does not take ownership.
   void Init(Browser* browser,
+            BrowserViewLayoutDelegate* delegate,
             BrowserView* browser_view,
+            views::View* top_container,
+            TabStrip* tab_strip,
+            views::View* toolbar,
             InfoBarContainerView* infobar_container,
-            views::SingleSplitView* contents_split,
+            views::View* contents_split,
             ContentsContainer* contents_container,
-            OverlayContainer* overlay_container);
+            OverlayContainer* overlay_container,
+            ImmersiveModeController* immersive_mode_controller);
 
   // Sets or updates views that are not available when |this| is initialized.
+  void set_window_switcher_button(views::View* button) {
+    window_switcher_button_ = button;
+  }
+  void set_tab_strip(TabStrip* tab_strip) {
+    tab_strip_ = tab_strip;
+  }
   void set_bookmark_bar(BookmarkBarView* bookmark_bar) {
     bookmark_bar_ = bookmark_bar;
   }
-  void set_download_shelf(DownloadShelfView* download_shelf) {
+  void set_download_shelf(views::View* download_shelf) {
     download_shelf_ = download_shelf;
   }
 
@@ -83,6 +108,7 @@ class BrowserViewLayout : public views::LayoutManager {
  private:
   FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, BrowserViewLayout);
   FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, Layout);
+  FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, LayoutDownloadShelf);
   class WebContentsModalDialogHostViews;
 
   enum InstantUIState {
@@ -99,7 +125,7 @@ class BrowserViewLayout : public views::LayoutManager {
 
   // Layout the tab strip region, returns the coordinate of the bottom of the
   // TabStrip, for laying out subsequent controls.
-  int LayoutTabStripRegion();
+  int LayoutTabStripRegion(views::View* browser_view);
 
   // Layout the following controls, starting at |top|, returns the coordinate
   // of the bottom of the control, for laying out the next control.
@@ -143,16 +169,28 @@ class BrowserViewLayout : public views::LayoutManager {
   // The browser from the owning BrowserView.
   Browser* browser_;
 
+  // The delegate interface. May be a mock in tests.
+  BrowserViewLayoutDelegate* delegate_;
+
   // The owning BrowserView. May be NULL in tests.
+  // TODO(jamescook): Remove this, use the views::View passed in to Layout().
   BrowserView* browser_view_;
 
   // Child views that the layout manager manages.
+  // NOTE: If you add a view, try to add it as a views::View, which makes
+  // testing much easier.
+  views::View* top_container_;
+  TabStrip* tab_strip_;
+  views::View* toolbar_;
   BookmarkBarView* bookmark_bar_;
   InfoBarContainerView* infobar_container_;
-  views::SingleSplitView* contents_split_;
+  views::View* contents_split_;
   ContentsContainer* contents_container_;
   OverlayContainer* overlay_container_;
-  DownloadShelfView* download_shelf_;
+  views::View* window_switcher_button_;
+  views::View* download_shelf_;
+
+  ImmersiveModeController* immersive_mode_controller_;
 
   // The bounds within which the vertically-stacked contents of the BrowserView
   // should be laid out within. This is just the local bounds of the