CR23: Fix background behind toolbar rounded corners.

When the toolbar draws its rounded corners, it leaves behind some pixels
that are unpainted. This CL adds two child Views to ToolbarView that sit
behind the top-left and top-right corners. These views are not affected
by the clip path. The views draw the same background as the tabstrip.

This CL makes two small supporting changes:
* It moves some logic in TopContainerBackground to a standalone method
  to avoid code duplication for painting image-themed backgrounds.
* It adds a listener to update the background views on window active
  state change.

Note that the toolbar already repaints on theme change so no additional
listener is needed for that case.

Change-Id: Id06fd5121e975b9268b89de7a74eb8ce5ba82e4e
Bug: 4559201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4559201
Reviewed-by: David Pennington <dpenning@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1150000}

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

@@ -22,33 +22,47 @@ void TopContainerBackground::Paint(gfx::Canvas* canvas,
                   /*translate_view_coordinates=*/false);
 }
 
+bool TopContainerBackground::PaintThemeCustomImage(
+    gfx::Canvas* canvas,
+    const views::View* view,
+    const BrowserView* browser_view,
+    bool translate_view_coordinates) {
+  const ui::ThemeProvider* const theme_provider = view->GetThemeProvider();
+  if (!theme_provider->HasCustomImage(IDR_THEME_TOOLBAR)) {
+    return false;
+  }
+
+  // This is a recapitulation of the logic originally used to place the
+  // background image in the bookmarks bar. It is used to ensure backwards-
+  // compatibility with existing themes, even though it is not technically
+  // correct in all cases.
+  gfx::Point view_offset = view->GetMirroredPosition();
+  // TODO(pbos): See if we can figure out how to translate correctly
+  // unconditionally from this bool.
+  if (translate_view_coordinates) {
+    views::View::ConvertPointToTarget(view, browser_view, &view_offset);
+  }
+  gfx::Point pos =
+      view_offset + browser_view->GetMirroredPosition().OffsetFromOrigin();
+  pos.Offset(browser_view->frame()->GetThemeBackgroundXInset(),
+             -browser_view->tabstrip()->GetStrokeThickness() -
+                 browser_view->frame()->GetTopInset());
+  const gfx::Rect bounds = view->GetLocalBounds();
+
+  canvas->TileImageInt(*theme_provider->GetImageSkiaNamed(IDR_THEME_TOOLBAR),
+                       pos.x(), pos.y(), bounds.x(), bounds.y(), bounds.width(),
+                       bounds.height(), 1.0f, SkTileMode::kRepeat,
+                       SkTileMode::kMirror);
+  return true;
+}
+
 void TopContainerBackground::PaintBackground(gfx::Canvas* canvas,
                                              const views::View* view,
                                              const BrowserView* browser_view,
                                              bool translate_view_coordinates) {
-  const ui::ThemeProvider* const theme_provider = view->GetThemeProvider();
-  if (theme_provider->HasCustomImage(IDR_THEME_TOOLBAR)) {
-    // This is a recapitulation of the logic originally used to place the
-    // background image in the bookmarks bar. It is used to ensure backwards-
-    // compatibility with existing themes, even though it is not technically
-    // correct in all cases.
-    gfx::Point view_offset = view->GetMirroredPosition();
-    // TODO(pbos): See if we can figure out how to translate correctly
-    // unconditionally from this bool.
-    if (translate_view_coordinates)
-      views::View::ConvertPointToTarget(view, browser_view, &view_offset);
-    gfx::Point pos =
-        view_offset + browser_view->GetMirroredPosition().OffsetFromOrigin();
-    pos.Offset(browser_view->frame()->GetThemeBackgroundXInset(),
-               -browser_view->tabstrip()->GetStrokeThickness() -
-                   browser_view->frame()->GetTopInset());
-    const gfx::Rect bounds = view->GetLocalBounds();
-
-    canvas->TileImageInt(*theme_provider->GetImageSkiaNamed(IDR_THEME_TOOLBAR),
-                         pos.x(), pos.y(), bounds.x(), bounds.y(),
-                         bounds.width(), bounds.height(), 1.0f,
-                         SkTileMode::kRepeat, SkTileMode::kMirror);
-  } else {
+  bool painted = PaintThemeCustomImage(canvas, view, browser_view,
+                                       translate_view_coordinates);
+  if (!painted) {
     canvas->DrawColor(view->GetColorProvider()->GetColor(kColorToolbar));
   }
 }

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

@@ -22,6 +22,13 @@ class TopContainerBackground : public views::Background {
   TopContainerBackground& operator=(const TopContainerBackground& other) =
       delete;
 
+  // Paints the theme's custom image if one exists. Returns whether or not any
+  // painting occurred.
+  static bool PaintThemeCustomImage(gfx::Canvas* canvas,
+                                    const views::View* view,
+                                    const BrowserView* browser_view,
+                                    bool translate_view_coordinates);
+
   // Static version for painting this background, used by the SidePanel
   // background to paint this background as a part of its background.
   // TODO(pbos): See if we can get rid of `translate_view_coordinates` by

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -170,6 +170,28 @@ constexpr int kToolbarDividerSpacing = 9;
 constexpr int kBrowserAppMenuRefreshExpandedMargin = 5;
 constexpr int kBrowserAppMenuRefreshCollapsedMargin = 2;
 
+// Draws background akin to the tabstrip.
+class TabstripLikeBackground : public views::Background {
+ public:
+  explicit TabstripLikeBackground(BrowserView* browser_view)
+      : browser_view_(browser_view) {}
+
+ private:
+  // views::Background:
+  void Paint(gfx::Canvas* canvas, views::View* view) const override {
+    bool painted = TopContainerBackground::PaintThemeCustomImage(
+        canvas, view, browser_view_, /*translate_view_coordinates=*/false);
+    if (!painted) {
+      SkColor frame_color =
+          browser_view_->frame()->GetFrameView()->GetFrameColor(
+              BrowserFrameActiveState::kUseCurrent);
+      canvas->DrawColor(frame_color);
+    }
+  }
+
+  const raw_ptr<BrowserView> browser_view_;
+};
+
 }  // namespace
 
 class ToolbarView::ContainerView : public views::View {
@@ -223,6 +245,20 @@ void ToolbarView::Init() {
   aura::WindowOcclusionTracker::ScopedPause pause_occlusion;
 #endif
 
+  // The background views must be behind container_view_.
+  if (features::IsChromeRefresh2023()) {
+    background_view_left_ = AddChildViewAt(std::make_unique<View>(), 0);
+    background_view_left_->SetBackground(
+        std::make_unique<TabstripLikeBackground>(browser_view_));
+    background_view_right_ = AddChildViewAt(std::make_unique<View>(), 0);
+    background_view_right_->SetBackground(
+        std::make_unique<TabstripLikeBackground>(browser_view_));
+
+    active_state_subscription_ =
+        GetWidget()->RegisterPaintAsActiveChangedCallback(base::BindRepeating(
+            &ToolbarView::ActiveStateChanged, base::Unretained(this)));
+  }
+
   auto location_bar = std::make_unique<LocationBarView>(
       browser_, browser_->profile(), browser_->command_controller(), this,
       display_mode_ != DisplayMode::NORMAL);
@@ -707,6 +743,15 @@ void ToolbarView::Layout() {
   // The container view should be the exact same size/position as ToolbarView.
   container_view_->SetSize(size());
 
+  if (features::IsChromeRefresh2023()) {
+    // The background views should be behind the top-left and top-right corners
+    // of the container_view_.
+    const int corner_radius = GetLayoutConstant(TOOLBAR_CORNER_RADIUS);
+    background_view_left_->SetBounds(0, 0, corner_radius, corner_radius);
+    background_view_right_->SetBounds(width() - corner_radius, 0, corner_radius,
+                                      corner_radius);
+  }
+
   if (display_mode_ == DisplayMode::CUSTOM_TAB) {
     custom_tab_bar_->SetBounds(0, 0, width(),
                                custom_tab_bar_->GetPreferredSize().height());
@@ -754,6 +799,11 @@ void ToolbarView::UpdateClipPath() {
   container_view_->SetClipPath(path);
 }
 
+void ToolbarView::ActiveStateChanged() {
+  background_view_left_->SchedulePaint();
+  background_view_right_->SchedulePaint();
+}
+
 bool ToolbarView::AcceleratorPressed(const ui::Accelerator& accelerator) {
   const views::View* focused_view = focus_manager()->GetFocusedView();
   if (focused_view && (focused_view->GetID() == VIEW_ID_OMNIBOX))

chrome/browser/ui/views/toolbar/toolbar_view.h

@@ -258,6 +258,9 @@ class ToolbarView : public views::AccessiblePaneView,
 
   void UpdateClipPath();
 
+  // Called when active state for the window changes.
+  void ActiveStateChanged();
+
   gfx::SlideAnimation size_animation_{this};
 
   // Controls. Most of these can be null, e.g. in popup windows. Only
@@ -312,9 +315,21 @@ class ToolbarView : public views::AccessiblePaneView,
   // All children are added to container_view_ and layout_manager_ applies to
   // container_view_. The reason for this layer of indiretion is because
   // container_view_ has a clip path set in UpdateClipPath() which adds rounded
-  // corners. This leaves some unpainted pixels, which this class will paint in
+  // corners. This leaves some unpainted pixels, which are painted by
+  // background_view_left_ and background_view_right_.
   // the future.
   raw_ptr<ContainerView> container_view_ = nullptr;
+
+  // There are two situations where background_view_left_ and
+  // background_view_right_ need be repainted: window active state change and
+  // theme change. active_state_subscription_ handles the former, and the latter
+  // causes the whole toolbar to be repainted so not special logic is necessary.
+  raw_ptr<View> background_view_left_ = nullptr;
+  raw_ptr<View> background_view_right_ = nullptr;
+
+  // Listens to changes to active state to update background_view_right_ and
+  // background_view_left_, as their background depends on active state.
+  base::CallbackListSubscription active_state_subscription_;
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_TOOLBAR_VIEW_H_