Unit test for BrowserViewLayout that does not depend on BrowserView

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

 // 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.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_VIEW_LAYOUT_H_
 #define CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_VIEW_LAYOUT_H_
 
 #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"
 
 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 {
 class Point;
 class Size;
 }
 
 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:
   // The vertical overlap between the TabStrip and the Toolbar.
   static const int kToolbarTabStripVerticalOverlap;
 
   BrowserViewLayout();
   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;
   }
 
   WebContentsModalDialogHost* GetWebContentsModalDialogHost();
 
   // Returns the minimum size of the browser view.
   gfx::Size GetMinimumSize();
 
   // Returns the bounding box, in widget coordinates,  for the find bar.
   gfx::Rect GetFindBarBoundingBox() const;
 
   // Returns true if the specified point(BrowserView coordinates) is in
   // in the window caption area of the browser window.
   bool IsPositionInWindowCaption(const gfx::Point& point);
 
   // Tests to see if the specified |point| (in nonclient view's coordinates)
   // is within the views managed by the laymanager. Returns one of
   // HitTestCompat enum defined in ui/base/hit_test.h.
   // See also ClientView::NonClientHitTest.
   int NonClientHitTest(const gfx::Point& point);
 
   // views::LayoutManager overrides:
   virtual void Layout(views::View* host) OVERRIDE;
   virtual gfx::Size GetPreferredSize(views::View* host) OVERRIDE;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, BrowserViewLayout);
   FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, Layout);
+  FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, LayoutDownloadShelf);
   class WebContentsModalDialogHostViews;
 
   enum InstantUIState {
     // No instant suggestions are being shown.
     kInstantUINone,
     // Instant suggestions are displayed in a overlay overlapping the tab
     // contents.
     kInstantUIOverlay,
     // Instant suggestions are displayed in the main tab contents.
     kInstantUIFullPageResults,
   };
 
   Browser* browser() { return browser_; }
 
   // 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.
   int LayoutToolbar(int top);
   int LayoutBookmarkAndInfoBars(int top);
   int LayoutBookmarkBar(int top);
   int LayoutInfoBar(int top);
 
   // Layout the |contents_split_| view between the coordinates |top| and
   // |bottom|. See browser_view.h for details of the relationship between
@@ skipping 23 matching lines @@
   // Layout the Download Shelf, returns the coordinate of the top of the
   // control, for laying out the previous control.
   int LayoutDownloadShelf(int bottom);
 
   // Returns true if an infobar is showing.
   bool InfobarVisible() const;
 
   // 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
   // BrowserView.
   // TODO(jamescook): Remove this and just use browser_view_->GetLocalBounds().
   gfx::Rect vertical_layout_rect_;
 
   // The host for use in positioning the web contents modal dialog.
   scoped_ptr<WebContentsModalDialogHostViews> dialog_host_;
 
   // The distance the web contents modal dialog is from the top of the window,
   // in pixels.
   int web_contents_modal_dialog_top_y_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowserViewLayout);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_VIEW_LAYOUT_H_