Move infobar separator drawing to InfoBarView.

This leaves InfoBarBackground drawing only the solid-color fill; a subsequent
patch will remove InfoBarBackground entirely.

This also changes how the separator size is computed.  Previously, all infobars
reported their preferred size including a separator, and InfoBarContainerView
trimmed 1 DIP off the bottom infobar.  However, this didn't actually prevent
InfoBarBackground from drawing the separator on that infobar (I think).

Now, separators are drawn at the top rather than the bottom, so it's the top
infobar which may need to omit the separator.  InfoBarView handles this omission
directly when computing its desired height.  InfoBarContainerView has to notify
a newly-top infobar to get it to recompute the height after animation finishes.

Additionally, since separators are 1 px, we only reserve 1 DIP when below DSF 2;
otherwise we just use some of the space within the infobar, since the top and
bottom are always padding anyway.

I could have left separators drawn at the bottom of infobars rather than the
top; at the time I thought through all this it seemed like doing things at the
top made some parts easier, but in retrospect it'd probably be about the same
either way.

BUG=none
TEST=none

Change-Id: I682851f16cc4a6a764c13f3983f22c829e9b10bd
Reviewed-on: https://chromium-review.googlesource.com/1000419Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549399}

chrome/browser/ui/BUILD.gn

@@ -1380,8 +1380,6 @@ split_static_library("ui") {
       "global_error/global_error_service_factory.h",
       "hung_renderer/hung_renderer_core.cc",
       "hung_renderer/hung_renderer_core.h",
-      "infobar_container_delegate.cc",
-      "infobar_container_delegate.h",
       "javascript_dialogs/javascript_dialog_views.cc",
       "javascript_dialogs/javascript_dialog_views.h",
       "layout_constants.cc",

chrome/browser/ui/infobar_container_delegate.cc

-// Copyright 2014 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/infobar_container_delegate.h"
-
-#if defined(TOOLKIT_VIEWS)
-#include "ui/views/window/non_client_view.h"
-#endif
-
-// static
-#if defined(OS_MACOSX)
-const int InfoBarContainerDelegate::kSeparatorLineHeight = 1;
-#else
-const int InfoBarContainerDelegate::kSeparatorLineHeight =
-    views::NonClientFrameView::kClientEdgeThickness;
-#endif

chrome/browser/ui/infobar_container_delegate.h

-// Copyright 2014 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_INFOBAR_CONTAINER_DELEGATE_H_
-#define CHROME_BROWSER_UI_INFOBAR_CONTAINER_DELEGATE_H_
-
-#include "components/infobars/core/infobar_container.h"
-
-class InfoBarContainerDelegate : public infobars::InfoBarContainer::Delegate {
- public:
-  static const int kSeparatorLineHeight;
-};
-
-#endif  // CHROME_BROWSER_UI_INFOBAR_CONTAINER_DELEGATE_H_

chrome/browser/ui/views/infobars/infobar_background.cc

@@ -41,19 +41,4 @@ void InfoBarBackground::Paint(gfx::Canvas* canvas, views::View* view) const {
   fill.setStyle(cc::PaintFlags::kFill_Style);
   fill.setColor(get_color());
   canvas->DrawPath(fill_path, fill);
-
-  // Bottom separator.
-  cc::PaintFlags stroke;
-  stroke.setStyle(cc::PaintFlags::kStroke_Style);
-  const int kSeparatorThicknessPx = 1;
-  stroke.setStrokeWidth(SkIntToScalar(kSeparatorThicknessPx));
-  SkColor separator_color = view->GetThemeProvider()->GetColor(
-      ThemeProperties::COLOR_DETACHED_BOOKMARK_BAR_SEPARATOR);
-  stroke.setColor(separator_color);
-  stroke.setAntiAlias(false);
-  gfx::SizeF view_size_px = gfx::ScaleSize(gfx::SizeF(view->size()), dsf);
-  SkScalar y = SkIntToScalar(view_size_px.height() - kSeparatorThicknessPx) +
-               SK_ScalarHalf;
-  SkScalar w = SkFloatToScalar(view_size_px.width());
-  canvas->sk_canvas()->drawLine(0, y, w, y, stroke);
 }

chrome/browser/ui/views/infobars/infobar_container_view.cc

@@ -6,7 +6,6 @@
 
 #include "cc/paint/paint_flags.h"
 #include "cc/paint/paint_shader.h"
-#include "chrome/browser/ui/infobar_container_delegate.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/infobars/infobar_view.h"
 #include "chrome/grit/generated_resources.h"
@@ -69,14 +68,7 @@ void InfoBarContainerView::Layout() {
   // Iterate over all infobars; the last child is the content shadow.
   for (int i = 0; i < child_count() - 1; ++i) {
     InfoBarView* child = static_cast<InfoBarView*>(child_at(i));
-    int child_height = child->computed_height();
-
-    // Trim off the bottom bar's separator; the shadow is good enough.
-    // The last infobar is the second to last child overall (followed by
-    // |content_shadow_|).
-    if (i == child_count() - 2)
-      child_height -= InfoBarContainerDelegate::kSeparatorLineHeight;
-    child->SetBounds(0, top, width(), child_height);
+    child->SetBounds(0, top, width(), child->computed_height());
     top = child->bounds().bottom();
   }
 
@@ -107,10 +99,6 @@ gfx::Size InfoBarContainerView::CalculatePreferredSize() const {
     size.SetToMax(child_size);  // Only affects our width.
   }
 
-  // No need to reserve space for the bottom bar's separator; the shadow is good
-  // enough.
-  size.Enlarge(0, -InfoBarContainerDelegate::kSeparatorLineHeight);
-
   // Don't reserve space for the bottom shadow here.  Because the shadow paints
   // to its own layer and this class doesn't, it can paint outside the size
   // computed here.  Not including the shadow bounds means the browser will
@@ -134,3 +122,22 @@ void InfoBarContainerView::PlatformSpecificRemoveInfoBar(
     infobars::InfoBar* infobar) {
   RemoveChildView(static_cast<InfoBarView*>(infobar));
 }
+
+void InfoBarContainerView::PlatformSpecificInfoBarStateChanged(
+    bool is_animating) {
+  // If we just finished animating the removal of the previous top infobar, the
+  // new top infobar should now stop drawing a top separator.  In this case the
+  // previous top infobar is zero-sized but has not yet been removed from the
+  // container, so we'll have at least three children (two infobars and a
+  // shadow), and the new top infobar is child 1.  The conditional below
+  // won't exclude cases where we're adding rather than removing an infobar, but
+  // doing unnecessary work on the second infobar in those cases is harmless.
+  if (!is_animating && child_count() > 2) {
+    // Dropping the separator may change the height.
+    auto* infobar = static_cast<InfoBarView*>(child_at(1));
+    infobar->RecalculateHeight();
+
+    // We need to force a paint whether or not the height actually changed.
+    infobar->SchedulePaint();
+  }
+}

chrome/browser/ui/views/infobars/infobar_container_view.h

@@ -31,6 +31,7 @@ class InfoBarContainerView : public views::AccessiblePaneView,
   void PlatformSpecificAddInfoBar(infobars::InfoBar* infobar,
                                   size_t position) override;
   void PlatformSpecificRemoveInfoBar(infobars::InfoBar* infobar) override;
+  void PlatformSpecificInfoBarStateChanged(bool is_animating) override;
 
  private:
   // This view draws the shadow over the web contents below the

chrome/browser/ui/views/infobars/infobar_view.cc

@@ -10,7 +10,7 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/themes/theme_properties.h"
-#include "chrome/browser/ui/infobar_container_delegate.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/harmony/chrome_typography.h"
 #include "chrome/browser/ui/views/infobars/infobar_background.h"
@@ -126,6 +126,13 @@ InfoBarView::InfoBarView(std::unique_ptr<infobars::InfoBarDelegate> delegate)
   AddChildView(close_button_);
 }
 
+InfoBarView::~InfoBarView() {
+  // We should have closed any open menus in PlatformSpecificHide(), then
+  // subclasses' RunMenu() functions should have prevented opening any new ones
+  // once we became unowned.
+  DCHECK(!menu_runner_.get());
+}
+
 void InfoBarView::RecalculateHeight() {
   // Ensure the infobar is tall enough to display its contents.
   int height = 0;
@@ -135,14 +142,7 @@ void InfoBarView::RecalculateHeight() {
     const int margin_height = margins ? margins->height() : 0;
     height = std::max(height, child->height() + margin_height);
   }
-  SetTargetHeight(height + InfoBarContainerDelegate::kSeparatorLineHeight);
-}
-
-InfoBarView::~InfoBarView() {
-  // We should have closed any open menus in PlatformSpecificHide(), then
-  // subclasses' RunMenu() functions should have prevented opening any new ones
-  // once we became unowned.
-  DCHECK(!menu_runner_.get());
+  SetTargetHeight(height + GetSeparatorHeightDip());
 }
 
 views::Label* InfoBarView::CreateLabel(const base::string16& text) const {
@@ -203,6 +203,16 @@ void InfoBarView::ViewHierarchyChanged(
   }
 }
 
+void InfoBarView::OnPaint(gfx::Canvas* canvas) {
+  views::View::OnPaint(canvas);
+
+  if (ShouldDrawSeparator()) {
+    const SkColor color =
+        GetColor(ThemeProperties::COLOR_DETACHED_BOOKMARK_BAR_SEPARATOR);
+    BrowserView::Paint1pxHorizontalLine(canvas, color, GetLocalBounds(), false);
+  }
+}
+
 void InfoBarView::OnThemeChanged() {
   const SkColor background_color = GetColor(kBackgroundColor);
   background()->SetNativeControlColor(background_color);
@@ -260,7 +270,8 @@ int InfoBarView::EndX() const {
 }
 
 int InfoBarView::OffsetY(views::View* view) const {
-  return std::max((target_height() - view->height()) / 2, 0) -
+  return GetSeparatorHeightDip() +
+         std::max((target_height() - view->height()) / 2, 0) -
          (target_height() - height());
 }
 
@@ -347,6 +358,30 @@ void InfoBarView::OnWillChangeFocus(View* focused_before, View* focused_now) {
   }
 }
 
+bool InfoBarView::ShouldDrawSeparator() const {
+  return parent()->GetIndexOf(this) != 0;
+}
+
+int InfoBarView::GetSeparatorHeightDip() const {
+  // We only need a separator for infobars after the first; the topmost infobar
+  // uses the toolbar as its top separator.
+  //
+  // Ideally the separator would take out 1 px in layout, but since we lay out
+  // in DIPs, we reserve 1 DIP below scale factor 2x, and 0 DIPs at 2 or above.
+  // This way the padding above the infobar content will never be more than 1 px
+  // from its ideal value.
+  //
+  // This only works because all infobars have padding at the top; if we
+  // actually draw all the way to the top, we'd risk drawing a separator atop
+  // some infobar content.
+  auto scale_factor = [this]() {
+    auto* widget = GetWidget();
+    // There may be no widget in tests.
+    return widget ? widget->GetCompositor()->device_scale_factor() : 1;
+  };
+  return (ShouldDrawSeparator() && (scale_factor() < 2)) ? 1 : 0;
+}
+
 SkColor InfoBarView::GetColor(int id) const {
   const auto* theme_provider = GetThemeProvider();
   // When there's no theme provider, this color will never be used; it will be

chrome/browser/ui/views/infobars/infobar_view.h

@@ -29,6 +29,7 @@ class InfoBarView : public infobars::InfoBar,
                     public views::ExternalFocusTracker {
  public:
   explicit InfoBarView(std::unique_ptr<infobars::InfoBarDelegate> delegate);
+  ~InfoBarView() override;
 
   // Requests that the infobar recompute its target height.
   void RecalculateHeight();
@@ -36,8 +37,6 @@ class InfoBarView : public infobars::InfoBar,
  protected:
   using Labels = std::vector<views::Label*>;
 
-  ~InfoBarView() override;
-
   // Creates a label with the appropriate font and color for an infobar.
   views::Label* CreateLabel(const base::string16& text) const;
 
@@ -56,6 +55,7 @@ class InfoBarView : public infobars::InfoBar,
   void Layout() override;
   void ViewHierarchyChanged(
       const ViewHierarchyChangedDetails& details) override;
+  void OnPaint(gfx::Canvas* canvas) override;
   void OnThemeChanged() override;
   void OnNativeThemeChanged(const ui::NativeTheme* theme) override;
 
@@ -74,12 +74,14 @@ class InfoBarView : public infobars::InfoBar,
   int StartX() const;
   int EndX() const;
 
-  // Given a |view|, returns the centered y position within |child_container_|,
-  // taking into account animation so the control "slides in" (or out) as we
-  // animate open and closed.
+  // Given a |view|, returns the centered y position, taking into account
+  // animation so the control "slides in" (or out) as we animate open and
+  // closed.
   int OffsetY(views::View* view) const;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(InfoBarViewTest, ShouldDrawSeparator);
+
   // Does the actual work for AssignWidths().  Assumes |labels| is sorted by
   // decreasing preferred width.
   static void AssignWidthsSorted(Labels* labels, int available_width);
@@ -96,6 +98,12 @@ class InfoBarView : public infobars::InfoBar,
   // views::ExternalFocusTracker:
   void OnWillChangeFocus(View* focused_before, View* focused_now) override;
 
+  // Returns whether this infobar should draw a 1 px separator at its top.
+  bool ShouldDrawSeparator() const;
+
+  // Returns how many DIPs the container should reserve for a separator between
+  // infobars, in addition to the height of the infobars themselves.
+  int GetSeparatorHeightDip() const;
 
   // Returns the current color for the theme property |id|.  Will return the
   // wrong value if no theme provider is available.

chrome/browser/ui/views/infobars/infobar_view_unittest.cc

+// Copyright 2018 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/infobars/infobar_view.h"
+
+#include "chrome/browser/infobars/infobar_service.h"
+#include "chrome/browser/ui/views/infobars/infobar_container_view.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/views/chrome_views_test_base.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_web_contents_factory.h"
+
+class TestInfoBarDelegate : public infobars::InfoBarDelegate {
+ public:
+  static bool Create(InfoBarService* infobar_service) {
+    return !!infobar_service->AddInfoBar(
+        std::make_unique<InfoBarView>(std::make_unique<TestInfoBarDelegate>()));
+  }
+
+  InfoBarIdentifier GetIdentifier() const override { return TEST_INFOBAR; }
+};
+
+class InfoBarViewTest : public ChromeViewsTestBase {
+ public:
+  InfoBarViewTest()
+      : web_contents_(web_contents_factory_.CreateWebContents(&profile_)),
+        infobar_container_view_(nullptr) {
+    InfoBarService::CreateForWebContents(web_contents_);
+    infobar_container_view_.ChangeInfoBarManager(infobar_service());
+  }
+
+  InfoBarService* infobar_service() {
+    return InfoBarService::FromWebContents(web_contents_);
+  }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  content::TestWebContentsFactory web_contents_factory_;
+  content::WebContents* web_contents_;
+  InfoBarContainerView infobar_container_view_;
+};
+
+TEST_F(InfoBarViewTest, ShouldDrawSeparator) {
+  // Add multiple infobars.
+  for (int i = 0; i < 3; ++i)
+    EXPECT_TRUE(TestInfoBarDelegate::Create(infobar_service()));
+
+  // The top infobar should not draw a separator; the others should.
+  auto infobar_at = [this](size_t index) {
+    return static_cast<InfoBarView*>(infobar_service()->infobar_at(index));
+  };
+  EXPECT_FALSE(infobar_at(0)->ShouldDrawSeparator());
+  EXPECT_TRUE(infobar_at(1)->ShouldDrawSeparator());
+  EXPECT_TRUE(infobar_at(2)->ShouldDrawSeparator());
+}

chrome/test/BUILD.gn

@@ -4255,6 +4255,7 @@ test("unit_tests") {
         "../browser/ui/views/frame/test_with_browser_view.cc",
         "../browser/ui/views/frame/test_with_browser_view.h",
         "../browser/ui/views/frame/web_contents_close_handler_unittest.cc",
+        "../browser/ui/views/infobars/infobar_view_unittest.cc",
         "../browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc",
         "../browser/ui/views/media_router/web_contents_display_observer_view_unittest.cc",
         "../browser/ui/views/omnibox/omnibox_result_view_unittest.cc",