More download shelf cleanup:

* Remove DownloadShelfNeedsLayout().  The BrowserView implementation of
  this turned out to be equivalent to checking if the download shelf
  GetVisible() returns true.  BrowserViewLayout can do this directly.
  The test implementation of this was easy to replace with setting the
  visibility directly.

* Remove the SetViewVisibility() call in LayoutDownloadShelf().  The
  download shelf updates its own visibility (correctly), so this was a
  no-op.

* Remove the Layout() call in LayoutDownloadShelf().  If the View size
  has changed, SetBoundsRect() will Layout() automatically.  If not,
  layout is unnecessary.

* Better use of unique_ptr to convey ownership in unittest code.

Bug: none
Change-Id: Ia79970b1fd386a080623ff80c57eb29bc50c2d2a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2227435
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774638}

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

@@ -399,15 +399,6 @@ class BrowserViewLayoutDelegateImpl : public BrowserViewLayoutDelegate {
       : browser_view_(browser_view) {}
   ~BrowserViewLayoutDelegateImpl() override {}
 
-  // BrowserViewLayoutDelegate overrides:
-  bool DownloadShelfNeedsLayout() const override {
-    DownloadShelfView* download_shelf = browser_view_->download_shelf_.get();
-    // Re-layout the shelf either if it is visible or if its close animation
-    // is currently running.
-    return download_shelf &&
-           (download_shelf->IsShowing() || download_shelf->IsClosing());
-  }
-
   bool IsTabStripVisible() const override {
     return browser_view_->IsTabStripVisible();
   }

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

@@ -529,13 +529,10 @@ void BrowserViewLayout::UpdateTopContainerBounds() {
 
 int BrowserViewLayout::LayoutDownloadShelf(int bottom) {
   TRACE_EVENT0("ui", "BrowserViewLayout::LayoutDownloadShelf");
-  if (delegate_->DownloadShelfNeedsLayout()) {
-    DCHECK(download_shelf_);
+  if (download_shelf_ && download_shelf_->GetVisible()) {
     const int height = download_shelf_->GetPreferredSize().height();
-    SetViewVisibility(download_shelf_, true);
     download_shelf_->SetBounds(vertical_layout_rect_.x(), bottom - height,
                                vertical_layout_rect_.width(), height);
-    download_shelf_->Layout();
     bottom -= height;
   }
   return bottom;

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

@@ -26,7 +26,6 @@ class BrowserViewLayoutDelegate {
   virtual bool IsToolbarVisible() const = 0;
   virtual bool IsBookmarkBarVisible() const = 0;
   virtual bool IsContentsSeparatorEnabled() const = 0;
-  virtual bool DownloadShelfNeedsLayout() const = 0;
   virtual ExclusiveAccessBubbleViews* GetExclusiveAccessBubble() const = 0;
   virtual bool IsTopControlsSlideBehaviorEnabled() const = 0;
   virtual float GetTopControlsSlideBehaviorShownRatio() const = 0;

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

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/views/frame/browser_view_layout.h"
 
+#include <memory>
+
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
@@ -29,9 +31,6 @@ class MockBrowserViewLayoutDelegate : public BrowserViewLayoutDelegate {
   MockBrowserViewLayoutDelegate() = default;
   ~MockBrowserViewLayoutDelegate() override = default;
 
-  void set_download_shelf_needs_layout(bool layout) {
-    download_shelf_needs_layout_ = layout;
-  }
   void set_tab_strip_visible(bool visible) {
     tab_strip_visible_ = visible;
   }
@@ -63,10 +62,6 @@ class MockBrowserViewLayoutDelegate : public BrowserViewLayoutDelegate {
   bool IsContentsSeparatorEnabled() const override {
     return content_separator_enabled_;
   }
-  bool DownloadShelfNeedsLayout() const override {
-    return download_shelf_needs_layout_;
-  }
-
   ExclusiveAccessBubbleViews* GetExclusiveAccessBubble() const override {
     return nullptr;
   }
@@ -97,7 +92,6 @@ class MockBrowserViewLayoutDelegate : public BrowserViewLayoutDelegate {
   bool toolbar_visible_ = true;
   bool bookmark_bar_visible_ = true;
   bool content_separator_enabled_ = true;
-  bool download_shelf_needs_layout_ = false;
   bool top_controls_slide_enabled_ = false;
   float top_controls_shown_ratio_ = 1.f;
 
@@ -106,8 +100,8 @@ class MockBrowserViewLayoutDelegate : public BrowserViewLayoutDelegate {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-views::View* CreateFixedSizeView(const gfx::Size& size) {
-  auto* view = new views::View;
+std::unique_ptr<views::View> CreateFixedSizeView(const gfx::Size& size) {
+  auto view = std::make_unique<views::View>();
   view->SetPreferredSize(size);
   view->SizeToPreferredSize();
   return view;
@@ -168,45 +162,44 @@ class BrowserViewLayoutTest : public ChromeViewsTestBase {
   void SetUp() override {
     ChromeViewsTestBase::SetUp();
 
-    root_view_.reset(CreateFixedSizeView(gfx::Size(800, 600)));
+    root_view_ = CreateFixedSizeView(gfx::Size(800, 600));
 
     immersive_mode_controller_ =
         std::make_unique<MockImmersiveModeController>();
 
-    top_container_ = CreateFixedSizeView(gfx::Size(800, 60));
-    views::View* tab_strip_region_view = new TabStripRegionView();
-    tab_strip_ = new TabStrip(std::make_unique<FakeBaseTabStripController>());
-    top_container_->AddChildView(tab_strip_region_view);
-    tab_strip_region_view->AddChildView(tab_strip_);
-    webui_tab_strip_ = CreateFixedSizeView(gfx::Size(800, 200));
+    top_container_ =
+        root_view_->AddChildView(CreateFixedSizeView(gfx::Size(800, 60)));
+    views::View* tab_strip_region_view =
+        top_container_->AddChildView(std::make_unique<TabStripRegionView>());
+    tab_strip_ = tab_strip_region_view->AddChildView(std::make_unique<TabStrip>(
+        std::make_unique<FakeBaseTabStripController>()));
+    webui_tab_strip_ =
+        top_container_->AddChildView(CreateFixedSizeView(gfx::Size(800, 200)));
     webui_tab_strip_->SetVisible(false);
-    top_container_->AddChildView(webui_tab_strip_);
-    toolbar_ = CreateFixedSizeView(gfx::Size(800, kToolbarHeight));
-    top_container_->AddChildView(toolbar_);
+    toolbar_ = top_container_->AddChildView(
+        CreateFixedSizeView(gfx::Size(800, kToolbarHeight)));
     separator_ =
         top_container_->AddChildView(std::make_unique<views::Separator>());
-    root_view_->AddChildView(top_container_);
 
-    infobar_container_ = new InfoBarContainerView(nullptr);
-    root_view_->AddChildView(infobar_container_);
+    infobar_container_ = root_view_->AddChildView(
+        std::make_unique<InfoBarContainerView>(nullptr));
 
-    contents_web_view_ = CreateFixedSizeView(gfx::Size(800, 600));
-    devtools_web_view_ = CreateFixedSizeView(gfx::Size(800, 600));
+    contents_container_ =
+        root_view_->AddChildView(CreateFixedSizeView(gfx::Size(800, 600)));
+    devtools_web_view_ = contents_container_->AddChildView(
+        CreateFixedSizeView(gfx::Size(800, 600)));
     devtools_web_view_->SetVisible(false);
-
-    contents_container_ = CreateFixedSizeView(gfx::Size(800, 600));
-    contents_container_->AddChildView(devtools_web_view_);
-    contents_container_->AddChildView(contents_web_view_);
+    contents_web_view_ = contents_container_->AddChildView(
+        CreateFixedSizeView(gfx::Size(800, 600)));
     contents_container_->SetLayoutManager(
         std::make_unique<ContentsLayoutManager>(devtools_web_view_,
                                                 contents_web_view_));
 
-    root_view_->AddChildView(contents_container_);
-
     // TODO(jamescook): Attach |layout_| to |root_view_|?
-    delegate_ = new MockBrowserViewLayoutDelegate();
+    auto delegate = std::make_unique<MockBrowserViewLayoutDelegate>();
+    delegate_ = delegate.get();
     layout_ = std::make_unique<BrowserViewLayout>(
-        std::unique_ptr<BrowserViewLayoutDelegate>(delegate_),
+        std::move(delegate),
         nullptr,  // NativeView.
         nullptr,  // BrowserView.
         top_container_, tab_strip_region_view, tab_strip_, toolbar_,
@@ -284,21 +277,21 @@ TEST_F(BrowserViewLayoutTest, Layout) {
 }
 
 TEST_F(BrowserViewLayoutTest, LayoutDownloadShelf) {
-  std::unique_ptr<views::View> download_shelf(
-      CreateFixedSizeView(gfx::Size(800, 50)));
+  constexpr int kHeight = 50;
+  std::unique_ptr<views::View> download_shelf =
+      CreateFixedSizeView(gfx::Size(800, kHeight));
   layout()->set_download_shelf(download_shelf.get());
 
-  // If download shelf doesn't need layout, it doesn't move the bottom edge.
-  delegate()->set_download_shelf_needs_layout(false);
-  const int kBottom = 500;
+  // If the download shelf isn't visible, it doesn't move the bottom edge.
+  download_shelf->SetVisible(false);
+  constexpr int kBottom = 500;
   EXPECT_EQ(kBottom, layout()->LayoutDownloadShelf(kBottom));
 
-  // Download shelf layout moves up the bottom edge and sets visibility.
-  delegate()->set_download_shelf_needs_layout(true);
-  download_shelf->SetVisible(false);
-  EXPECT_EQ(450, layout()->LayoutDownloadShelf(kBottom));
-  EXPECT_TRUE(download_shelf->GetVisible());
-  EXPECT_EQ(gfx::Rect(0, 450, 0, 50), download_shelf->bounds());
+  // A visible download shelf moves the bottom edge up.
+  download_shelf->SetVisible(true);
+  constexpr int kTop = kBottom - kHeight;
+  EXPECT_EQ(kTop, layout()->LayoutDownloadShelf(kBottom));
+  EXPECT_EQ(gfx::Rect(0, kTop, 0, kHeight), download_shelf->bounds());
 }
 
 TEST_F(BrowserViewLayoutTest, LayoutContentsWithTopControlsSlideBehavior) {