Removed browser dependency in browser_view_layout

Factored out the browser dependency present in browser_view_layout by
moving the required functionality to its delegate, allowing for
simplified testing of the layout component.

Bug: 871419
Change-Id: If4d738677ad1e6666cda245e64aa62a2e2d103ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1877613
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710046}

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

@@ -56,6 +56,8 @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window_state.h"
+#include "chrome/browser/ui/find_bar/find_bar.h"
+#include "chrome/browser/ui/find_bar/find_bar_controller.h"
 #include "chrome/browser/ui/in_product_help/reopen_tab_in_product_help.h"
 #include "chrome/browser/ui/in_product_help/reopen_tab_in_product_help_factory.h"
 #include "chrome/browser/ui/layout_constants.h"
@@ -376,6 +378,27 @@ class BrowserViewLayoutDelegateImpl : public BrowserViewLayoutDelegate {
     return browser_view_->GetTopControlsSlideBehaviorShownRatio();
   }
 
+  bool SupportsWindowFeature(Browser::WindowFeature feature) const override {
+    return browser_view_->browser()->SupportsWindowFeature(feature);
+  }
+
+  gfx::NativeView GetHostView() const override {
+    return browser_view_->GetWidget()->GetNativeView();
+  }
+
+  bool BrowserIsTypeNormal() const override {
+    return browser_view_->browser()->is_type_normal();
+  }
+
+  bool HasFindBarController() const override {
+    return browser_view_->browser()->HasFindBarController();
+  }
+
+  void MoveWindowForFindBarIfNecessary() const override {
+    auto* const controller = browser_view_->browser()->GetFindBarController();
+    return controller->find_bar()->MoveWindowIfNecessary();
+  }
+
  private:
   BrowserView* browser_view_;
 
@@ -2597,11 +2620,11 @@ void BrowserView::InitViews() {
   }
 
   auto browser_view_layout = std::make_unique<BrowserViewLayout>(
-      std::make_unique<BrowserViewLayoutDelegateImpl>(this), browser(), this,
-      top_container_, tab_strip_region_view_, tabstrip_, webui_tab_strip_view,
-      toolbar_, infobar_container_, contents_container_,
-      immersive_mode_controller_.get(), web_footer_experiment,
-      contents_separator_);
+      std::make_unique<BrowserViewLayoutDelegateImpl>(this),
+      GetWidget()->GetNativeView(), this, top_container_,
+      tab_strip_region_view_, tabstrip_, webui_tab_strip_view, toolbar_,
+      infobar_container_, contents_container_, immersive_mode_controller_.get(),
+      web_footer_experiment, contents_separator_);
   SetLayoutManager(std::move(browser_view_layout));
 
   EnsureFocusOrder();

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

@@ -19,7 +19,6 @@
 #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
 #include "chrome/browser/ui/views/download/download_shelf_view.h"
 #include "chrome/browser/ui/views/exclusive_access_bubble_views.h"
-#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/browser_view_layout_delegate.h"
 #include "chrome/browser/ui/views/frame/contents_layout_manager.h"
 #include "chrome/browser/ui/views/frame/immersive_mode_controller.h"
@@ -91,10 +90,6 @@ class BrowserViewLayout::WebContentsModalDialogHostViews
     return gfx::Point(middle_x - size.width() / 2, top);
   }
 
-  bool ShouldActivateDialog() const override {
-    return chrome::FindLastActive() == browser_view_layout_->browser_;
-  }
-
   gfx::Size GetMaximumDialogSize() override {
     views::View* view = browser_view_layout_->contents_container_;
     gfx::Rect content_area = view->ConvertRectToWidget(view->GetLocalBounds());
@@ -104,9 +99,7 @@ class BrowserViewLayout::WebContentsModalDialogHostViews
 
  private:
   gfx::NativeView GetHostView() const override {
-    gfx::NativeWindow window =
-        browser_view_layout_->browser()->window()->GetNativeWindow();
-    return views::Widget::GetWidgetForNativeWindow(window)->GetNativeView();
+    return browser_view_layout_->host_view_;
   }
 
   // Add/remove observer.
@@ -129,8 +122,8 @@ class BrowserViewLayout::WebContentsModalDialogHostViews
 
 BrowserViewLayout::BrowserViewLayout(
     std::unique_ptr<BrowserViewLayoutDelegate> delegate,
-    Browser* browser,
-    views::ClientView* browser_view,
+    gfx::NativeView host_view,
+    BrowserView* browser_view,
     views::View* top_container,
     views::View* tab_strip_region_view,
     TabStrip* tab_strip,
@@ -142,7 +135,7 @@ BrowserViewLayout::BrowserViewLayout(
     views::View* web_footer_experiment,
     views::View* contents_separator)
     : delegate_(std::move(delegate)),
-      browser_(browser),
+      host_view_(host_view),
       browser_view_(browser_view),
       top_container_(top_container),
       tab_strip_region_view_(tab_strip_region_view),
@@ -175,14 +168,14 @@ gfx::Size BrowserViewLayout::GetMinimumSize(const views::View* host) const {
   constexpr int kMainBrowserContentsMinimumHeight = 1;
 
   const bool has_tabstrip =
-      browser()->SupportsWindowFeature(Browser::FEATURE_TABSTRIP);
+      delegate_->SupportsWindowFeature(Browser::FEATURE_TABSTRIP);
   const bool has_toolbar =
-      browser()->SupportsWindowFeature(Browser::FEATURE_TOOLBAR);
+      delegate_->SupportsWindowFeature(Browser::FEATURE_TOOLBAR);
   const bool has_location_bar =
-      browser()->SupportsWindowFeature(Browser::FEATURE_LOCATIONBAR);
+      delegate_->SupportsWindowFeature(Browser::FEATURE_LOCATIONBAR);
   const bool has_bookmarks_bar =
       bookmark_bar_ && bookmark_bar_->GetVisible() &&
-      browser()->SupportsWindowFeature(Browser::FEATURE_BOOKMARKBAR);
+      delegate_->SupportsWindowFeature(Browser::FEATURE_BOOKMARKBAR);
 
   gfx::Size tabstrip_size(has_tabstrip ? tab_strip_->GetMinimumSize()
                                        : gfx::Size());
@@ -196,7 +189,7 @@ gfx::Size BrowserViewLayout::GetMinimumSize(const views::View* host) const {
   // TODO(pkotwicz): Adjust the minimum height for the find bar.
 
   gfx::Size contents_size(contents_container_->GetMinimumSize());
-  contents_size.SetToMax(browser()->is_type_normal()
+  contents_size.SetToMax(delegate_->BrowserIsTypeNormal()
                              ? gfx::Size(kMainBrowserContentsMinimumWidth,
                                          kMainBrowserContentsMinimumHeight)
                              : kContentsMinimumSize);
@@ -224,14 +217,11 @@ gfx::Rect BrowserViewLayout::GetFindBarBoundingBox() const {
   // The "user-perceived content area" excludes the detached bookmark bar (in
   // the New Tab case) and any infobars since they are not _visually_ connected
   // to the Toolbar.
-
-  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_);
-
   gfx::Rect bounding_box;
   if (!immersive_mode_controller_->IsEnabled() ||
       immersive_mode_controller_->IsRevealed()) {
     bounding_box =
-        browser_view->toolbar_button_provider()->GetFindBarBoundingBox(
+        browser_view_->toolbar_button_provider()->GetFindBarBoundingBox(
             contents_container_->height());
   }
   if (!bounding_box.IsEmpty())
@@ -244,9 +234,9 @@ gfx::Rect BrowserViewLayout::GetFindBarBoundingBox() const {
   // Under ChromeOS, the top_container_ may include the title bar for hosted
   // apps. Just make sure something of consequence is visible before it's height
   // is used.
-  const int top_container_height = (browser_view->tabstrip()->GetVisible() ||
-                                    browser_view->toolbar()->GetVisible() ||
-                                    browser_view->IsBookmarkBarVisible())
+  const int top_container_height = (browser_view_->tabstrip()->GetVisible() ||
+                                    browser_view_->toolbar()->GetVisible() ||
+                                    browser_view_->IsBookmarkBarVisible())
                                        ? top_container_->height()
                                        : 0;
   if (base::i18n::IsRTL())
@@ -256,6 +246,10 @@ gfx::Rect BrowserViewLayout::GetFindBarBoundingBox() const {
   return bounding_box;
 }
 
+gfx::NativeView BrowserViewLayout::GetHostView() {
+  return delegate_->GetHostView();
+}
+
 int BrowserViewLayout::NonClientHitTest(const gfx::Point& point) {
   // Since the TabStrip only renders in some parts of the top of the window,
   // the un-obscured area is considered to be part of the non-client caption
@@ -359,8 +353,8 @@ void BrowserViewLayout::Layout(views::View* browser_view) {
   // code calls back into us to find the bounding box the find bar
   // must be laid out within, and that code depends on the
   // TabContentsContainer's bounds being up to date.
-  if (browser()->HasFindBarController())
-    browser()->GetFindBarController()->find_bar()->MoveWindowIfNecessary();
+  if (delegate_->HasFindBarController())
+    delegate_->MoveWindowForFindBarIfNecessary();
 
   // Adjust the fullscreen exit bubble bounds for |top_container_|'s new bounds.
   // This makes the fullscreen exit bubble look like it animates with
@@ -533,8 +527,8 @@ void BrowserViewLayout::UpdateTopContainerBounds() {
 
 int BrowserViewLayout::LayoutDownloadShelf(int bottom) {
   if (delegate_->DownloadShelfNeedsLayout()) {
-    bool visible = browser()->SupportsWindowFeature(
-        Browser::FEATURE_DOWNLOADSHELF);
+    bool visible =
+        delegate_->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF);
     DCHECK(download_shelf_);
     int height = visible ? download_shelf_->GetPreferredSize().height() : 0;
     download_shelf_->SetVisible(visible);
@@ -567,6 +561,6 @@ bool BrowserViewLayout::IsInfobarVisible() const {
   // Cast to a views::View to access GetPreferredSize().
   views::View* infobar_container = infobar_container_;
   // NOTE: Can't check if the size IsEmpty() since it's always 0-width.
-  return browser_->SupportsWindowFeature(Browser::FEATURE_INFOBAR) &&
+  return delegate_->SupportsWindowFeature(Browser::FEATURE_INFOBAR) &&
          (infobar_container->GetPreferredSize().height() != 0);
 }

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

@@ -12,10 +12,11 @@
 #include "base/macros.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
+#include "ui/gfx/native_widget_types.h"
 #include "ui/views/layout/layout_manager.h"
 
 class BookmarkBarView;
-class Browser;
+class BrowserView;
 class BrowserViewLayoutDelegate;
 class ImmersiveModeController;
 class InfoBarContainerView;
@@ -26,7 +27,6 @@ class Point;
 }  // namespace gfx
 
 namespace views {
-class ClientView;
 class View;
 class Widget;
 }  // namespace views
@@ -49,8 +49,8 @@ class BrowserViewLayout : public views::LayoutManager {
 
   // |browser_view| may be null in tests.
   BrowserViewLayout(std::unique_ptr<BrowserViewLayoutDelegate> delegate,
-                    Browser* browser,
-                    views::ClientView* browser_view,
+                    gfx::NativeView host_view,
+                    BrowserView* browser_view,
                     views::View* top_container,
                     views::View* tab_strip_region_view,
                     TabStrip* tab_strip,
@@ -81,6 +81,9 @@ class BrowserViewLayout : public views::LayoutManager {
   // Returns the bounding box, in widget coordinates,  for the find bar.
   gfx::Rect GetFindBarBoundingBox() const;
 
+  // Returns the view against which the dialog is positioned and parented.
+  gfx::NativeView GetHostView();
+
   // 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.
@@ -101,9 +104,6 @@ class BrowserViewLayout : public views::LayoutManager {
   FRIEND_TEST_ALL_PREFIXES(BrowserViewLayoutTest, LayoutDownloadShelf);
   class WebContentsModalDialogHostViews;
 
-  Browser* browser() { return browser_; }
-  const Browser* browser() const { return browser_; }
-
   // Layout the following controls, starting at |top|, returns the coordinate
   // of the bottom of the control, for laying out the next control.
   int LayoutTabStripRegion(int top);
@@ -136,11 +136,11 @@ class BrowserViewLayout : public views::LayoutManager {
   // The delegate interface. May be a mock in tests.
   const std::unique_ptr<BrowserViewLayoutDelegate> delegate_;
 
-  // The browser from the owning BrowserView.
-  Browser* const browser_;
+  // The view against which the web dialog is positioned and parented.
+  gfx::NativeView const host_view_;
 
   // The owning browser view.
-  views::ClientView* const browser_view_;
+  BrowserView* const 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

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

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_VIEW_LAYOUT_DELEGATE_H_
 #define CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_VIEW_LAYOUT_DELEGATE_H_
 
+#include "ui/gfx/native_widget_types.h"
+
 class ExclusiveAccessBubbleViews;
 
 namespace gfx {
@@ -27,6 +29,11 @@ class BrowserViewLayoutDelegate {
   virtual ExclusiveAccessBubbleViews* GetExclusiveAccessBubble() const = 0;
   virtual bool IsTopControlsSlideBehaviorEnabled() const = 0;
   virtual float GetTopControlsSlideBehaviorShownRatio() const = 0;
+  virtual bool SupportsWindowFeature(Browser::WindowFeature feature) const = 0;
+  virtual gfx::NativeView GetHostView() const = 0;
+  virtual bool BrowserIsTypeNormal() const = 0;
+  virtual bool HasFindBarController() const = 0;
+  virtual void MoveWindowForFindBarIfNecessary() const = 0;
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_VIEW_LAYOUT_DELEGATE_H_

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

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/views/frame/browser_view_layout.h"
 
+#include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/browser_view_layout_delegate.h"
@@ -14,13 +15,13 @@
 #include "chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "chrome/browser/ui/views/tabs/tab_strip_controller.h"
-#include "chrome/test/base/browser_with_test_window_test.h"
+#include "chrome/test/views/chrome_views_test_base.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/views/controls/separator.h"
 
 namespace {
 
-static const int kToolbarHeight = 30 - views::Separator::kThickness;
+constexpr int kToolbarHeight = 30 - views::Separator::kThickness;
 
 class MockBrowserViewLayoutDelegate : public BrowserViewLayoutDelegate {
  public:
@@ -68,6 +69,22 @@ class MockBrowserViewLayoutDelegate : public BrowserViewLayoutDelegate {
   float GetTopControlsSlideBehaviorShownRatio() const override {
     return top_controls_shown_ratio_;
   }
+  bool SupportsWindowFeature(
+      const Browser::WindowFeature feature) const override {
+    static const base::NoDestructor<base::flat_set<Browser::WindowFeature>>
+        supported_features({
+            Browser::FEATURE_TABSTRIP,
+            Browser::FEATURE_TOOLBAR,
+            Browser::FEATURE_LOCATIONBAR,
+            Browser::FEATURE_BOOKMARKBAR,
+            Browser::FEATURE_DOWNLOADSHELF,
+        });
+    return base::Contains(*supported_features, feature);
+  }
+  gfx::NativeView GetHostView() const override { return nullptr; }
+  bool BrowserIsTypeNormal() const override { return true; }
+  bool HasFindBarController() const override { return false; }
+  void MoveWindowForFindBarIfNecessary() const override {}
 
  private:
   bool tab_strip_visible_ = true;
@@ -117,7 +134,7 @@ class MockImmersiveModeController : public ImmersiveModeController {
 
 ///////////////////////////////////////////////////////////////////////////////
 // Tests of BrowserViewLayout. Runs tests without constructing a BrowserView.
-class BrowserViewLayoutTest : public BrowserWithTestWindowTest {
+class BrowserViewLayoutTest : public ChromeViewsTestBase {
  public:
   BrowserViewLayoutTest()
       : delegate_(nullptr),
@@ -140,9 +157,8 @@ class BrowserViewLayoutTest : public BrowserWithTestWindowTest {
   InfoBarContainerView* infobar_container() { return infobar_container_; }
   views::View* contents_container() { return contents_container_; }
 
-  // BrowserWithTestWindowTest overrides:
   void SetUp() override {
-    BrowserWithTestWindowTest::SetUp();
+    ChromeViewsTestBase::SetUp();
 
     root_view_.reset(CreateFixedSizeView(gfx::Size(800, 600)));
 
@@ -158,7 +174,6 @@ class BrowserViewLayoutTest : public BrowserWithTestWindowTest {
     toolbar_ =
         CreateFixedSizeView(gfx::Size(800, 30 - views::Separator::kThickness));
     top_container_->AddChildView(toolbar_);
-    top_container_->AddChildView(toolbar_);
     separator_ =
         top_container_->AddChildView(std::make_unique<views::Separator>());
     root_view_->AddChildView(top_container_);
@@ -182,7 +197,8 @@ class BrowserViewLayoutTest : public BrowserWithTestWindowTest {
     // TODO(jamescook): Attach |layout_| to |root_view_|?
     delegate_ = new MockBrowserViewLayoutDelegate();
     layout_ = std::make_unique<BrowserViewLayout>(
-        std::unique_ptr<BrowserViewLayoutDelegate>(delegate_), browser(),
+        std::unique_ptr<BrowserViewLayoutDelegate>(delegate_),
+        nullptr,  // NativeView.
         nullptr,  // BrowserView.
         top_container_, tab_strip_region_view, tab_strip_, nullptr, toolbar_,
         infobar_container_, contents_container_,
@@ -211,7 +227,6 @@ class BrowserViewLayoutTest : public BrowserWithTestWindowTest {
 
 // Test basic construction and initialization.
 TEST_F(BrowserViewLayoutTest, BrowserViewLayout) {
-  EXPECT_TRUE(layout()->browser());
   EXPECT_TRUE(layout()->GetWebContentsModalDialogHost());
   EXPECT_FALSE(layout()->IsInfobarVisible());
 }

ui/views/controls/separator.cc

@@ -11,9 +11,6 @@
 
 namespace views {
 
-// static
-const int Separator::kThickness = 1;
-
 Separator::Separator() = default;
 
 Separator::~Separator() = default;

ui/views/controls/separator.h

@@ -20,7 +20,7 @@ class VIEWS_EXPORT Separator : public View {
   METADATA_HEADER(Separator);
 
   // The separator's thickness in dip.
-  static const int kThickness;
+  static constexpr int kThickness = 1;
 
   Separator();
   ~Separator() override;