Fix alignment for IDR_THEME_TOOLBAR and IDR_THEME_FRAME.

This CL makes two changes:

(1) It standardizes the logic from different views for painting the
toolbar theme, tab theme, and frame theme. This ensures that the themes
are painted consistently. The new logic is x-platform. Although it must
be called in several platform-specific classes, the base calculations
can now be shared.

(2) It aligns the origin point for the toolbar theme, tab theme, and
frame theme. This fixes some themes (which relies on this behavior) and
breaks some themes (which rely on the behavior that the toolbar theme is
aligned with the top painted pixel of the active tab). I made this
choice for two reasons:

(2a) This matches the pre-existing code level documentation. (2b) The
themes that this breaks are generally relying on implementation details
of chrome. For example, some themes rely on the shape of the tabs: flat
upper side. Other themes rely on the exact DIP height of the current tab
strip.

Note that most themes are not affected by (2) since most themes use a
repeating pixel for IDR_THEME_TOOLBAR.

Theme with a gradient. Before CL on left, after CL on right:
https://screenshot.googleplex.com/9yb84TXEK8YsdJH

Theme with an image. Before CL on left, after CL on right:
https://screenshot.googleplex.com/LViTJPTYLSfqhZe

Change-Id: I9754769c078cbb606f15a1adfe844a634ce67a12
Bug: 1473538
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4809121
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: David Pennington <dpenning@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188116}

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

@@ -220,7 +220,7 @@ void BrowserFrameHeaderChromeOS::PaintFrameImages(gfx::Canvas* canvas) {
 
   PaintFrameImagesInRoundRect(canvas, frame_image, frame_overlay_image,
                               appearance_provider_->GetFrameHeaderColor(active),
-                              GetPaintedBounds(), GetThemeBackgroundXInset(),
+                              GetPaintedBounds(), /*image_inset_x=*/0,
                               appearance_provider_->GetFrameHeaderImageYInset(),
                               header_corner_radius());
 }

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

@@ -600,7 +600,7 @@ gfx::ImageSkia BrowserNonClientFrameViewChromeOS::GetFrameHeaderImage(
 }
 
 int BrowserNonClientFrameViewChromeOS::GetFrameHeaderImageYInset() {
-  return ThemeProperties::kFrameHeightAboveTabs - GetTopInset(false);
+  return browser_view()->GetThemeOffsetFromBrowserView().y();
 }
 
 gfx::ImageSkia BrowserNonClientFrameViewChromeOS::GetFrameHeaderOverlayImage(

chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm

@@ -507,9 +507,17 @@ FullscreenToolbarStyle GetUserPreferredToolbarStyle(bool always_show) {
 }
 
 void BrowserNonClientFrameViewMac::PaintThemedFrame(gfx::Canvas* canvas) {
+  // On macOS the origin of the BrowserNonClientFrameViewMac is (0,0) so no
+  // further modification is necessary. See
+  // TopContainerBackground::PaintThemeCustomImage for details.
+  gfx::Point theme_image_offset =
+      browser_view()->GetThemeOffsetFromBrowserView();
+
   gfx::ImageSkia image = GetFrameImage();
-  canvas->TileImageInt(image, 0, TopUIFullscreenYOffset(), width(),
-                       image.height());
+  canvas->TileImageInt(image, theme_image_offset.x(), theme_image_offset.y(), 0,
+                       TopUIFullscreenYOffset(), width(), image.height(),
+                       /*tile_scale=*/1.0f, SkTileMode::kRepeat,
+                       SkTileMode::kMirror);
   gfx::ImageSkia overlay = GetFrameOverlayImage();
   canvas->DrawImageInt(overlay, 0, 0);
 }

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

@@ -1544,6 +1544,18 @@ void BrowserView::SetLoadingAnimationStateChangeClosureForTesting(
   loading_animation_state_change_closure_ = std::move(closure);
 }
 
+gfx::Point BrowserView::GetThemeOffsetFromBrowserView() const {
+  gfx::Point browser_view_origin;
+  const views::View* root_view = this;
+  while (root_view->parent()) {
+    root_view = root_view->parent();
+  }
+  views::View::ConvertPointToTarget(this, root_view, &browser_view_origin);
+  return gfx::Point(
+      -browser_view_origin.x(),
+      ThemeProperties::kFrameHeightAboveTabs - browser_view_origin.y());
+}
+
 bool BrowserView::IsLoadingAnimationRunningForTesting() const {
   return loading_animation_timer_.IsRunning();
 }

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

@@ -813,6 +813,12 @@ class BrowserView : public BrowserWindow,
     return web_app_frame_toolbar();
   }
 
+  // This value is used in a common calculation in NonClientFrameView
+  // subclasses. This must be added to the origin of the first painted pixel of
+  // NonClientFrameView to get the correct offset. See
+  // TopContainerBackground::PaintThemeCustomImage for details.
+  gfx::Point GetThemeOffsetFromBrowserView() const;
+
  private:
   // Do not friend BrowserViewLayout. Use the BrowserViewLayoutDelegate
   // interface to keep these two classes decoupled and testable.

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

@@ -625,11 +625,8 @@ void OpaqueBrowserFrameView::OnPaint(gfx::Canvas* canvas) {
   frame_background_->set_use_custom_frame(frame()->UseCustomFrame());
   frame_background_->set_is_active(active);
   frame_background_->set_theme_image(GetFrameImage());
-  const int y_inset =
-      browser_view()->GetTabStripVisible()
-          ? (ThemeProperties::kFrameHeightAboveTabs - GetTopInset(false))
-          : 0;
-  frame_background_->set_theme_image_y_inset(y_inset);
+  frame_background_->set_theme_image_inset(
+      browser_view()->GetThemeOffsetFromBrowserView());
   frame_background_->set_theme_overlay_image(GetFrameOverlayImage());
   frame_background_->set_top_area_height(GetTopAreaHeight());
 

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

@@ -884,8 +884,9 @@ void PictureInPictureBrowserFrameView::OnPaint(gfx::Canvas* canvas) {
     frame_background_->set_use_custom_frame(frame()->UseCustomFrame());
     frame_background_->set_is_active(ShouldPaintAsActive());
     frame_background_->set_theme_image(GetFrameImage());
-    frame_background_->set_theme_image_y_inset(
-        ThemeProperties::kFrameHeightAboveTabs - GetTopAreaHeight());
+
+    frame_background_->set_theme_image_inset(
+        browser_view()->GetThemeOffsetFromBrowserView());
     frame_background_->set_theme_overlay_image(GetFrameOverlayImage());
     frame_background_->set_top_area_height(GetTopAreaHeight());
     PaintRestoredFrameBorderLinux(

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

@@ -18,50 +18,47 @@ TopContainerBackground::TopContainerBackground(BrowserView* browser_view)
 
 void TopContainerBackground::Paint(gfx::Canvas* canvas,
                                    views::View* view) const {
-  PaintBackground(canvas, view, browser_view_,
-                  /*translate_view_coordinates=*/false);
+  PaintBackground(canvas, view, browser_view_);
 }
 
 bool TopContainerBackground::PaintThemeCustomImage(
     gfx::Canvas* canvas,
     const views::View* view,
-    const BrowserView* browser_view,
-    bool translate_view_coordinates) {
+    const BrowserView* browser_view) {
   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);
+  PaintThemeAlignedImage(canvas, view, browser_view,
+                         theme_provider->GetImageSkiaNamed(IDR_THEME_TOOLBAR));
   return true;
 }
 
+void TopContainerBackground::PaintThemeAlignedImage(
+    gfx::Canvas* canvas,
+    const views::View* view,
+    const BrowserView* browser_view,
+    gfx::ImageSkia* image) {
+  // Get the origin of this view and translate it to coordinate system of the
+  // BrowserView.
+  gfx::Point pos;
+  views::View::ConvertPointToTarget(view, browser_view, &pos);
+
+  // Add in the translation to account for positioning of the theme image
+  // relative of the origin of BrowserView.
+  pos.Offset(0, ThemeProperties::kFrameHeightAboveTabs);
+
+  const gfx::Rect bounds = view->GetLocalBounds();
+  canvas->TileImageInt(*image, pos.x(), pos.y(), bounds.x(), bounds.y(),
+                       bounds.width(), bounds.height(), 1.0f,
+                       SkTileMode::kRepeat, SkTileMode::kMirror);
+}
+
 void TopContainerBackground::PaintBackground(gfx::Canvas* canvas,
                                              const views::View* view,
-                                             const BrowserView* browser_view,
-                                             bool translate_view_coordinates) {
-  bool painted = PaintThemeCustomImage(canvas, view, browser_view,
-                                       translate_view_coordinates);
+                                             const BrowserView* browser_view) {
+  bool painted = PaintThemeCustomImage(canvas, view, browser_view);
   if (!painted) {
     canvas->DrawColor(view->GetColorProvider()->GetColor(kColorToolbar));
   }

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

@@ -22,22 +22,51 @@ 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.
+  // We need a mechanism to consistently paint theme custom images across
+  // multiple views. Specifically, IDR_THEME_TOOLBAR and IDR_THEME_FRAME*, and
+  // IDR_TAB* are
+  // expected to be aligned. To do this we need:
+  //   (1) A fixed reference point + coordinate system that all views can align
+  //   on.
+  //   (2) An agreement on the position of the theme custom image in this
+  //   coordinate system.
+  // This is complicated by the fact that for most platforms, Chrome also draws
+  // the frame/border/shadow of the window itself, which we do not want themed.
+  //
+  // (1) The fixed reference point that we will use for all calculations is the
+  // origin of the BrowserView in the coordinate system of the root view
+  // (BrowserRootView). To provide an example: at the time of this writing, on a
+  // standard tabbed window on Linux, this point is (16, 13).
+  // (2) The origin of the theme custom image is set to 16 DIPs above the fixed
+  // reference point. See kFrameHeightAboveTabs. This is an implementation
+  // detail that theme authors have been relying on for many years. Continuing
+  // our example, this point is (16, -3).
+  //
+  // Most views will be a child of BrowserView. This method handles painting the
+  // theme custom image for these views. The math is straight forward. However,
+  // the portion of the tab strip that is behind the tabs is painted by
+  // views::FrameBackground. The logic there must be kept in sync with the logic
+  // here. Continuing our example, the origin that FrameBackground starts
+  // drawing at is (16, 10). Notice that this is 3 DIPS above the BrowserView!
+  //
+  // This method paints IDR_THEME_TOOLBAR if its 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);
+                                    const BrowserView* browser_view);
+
+  // Similar to PaintThemeCustomImage but the image is supplied.
+  static void PaintThemeAlignedImage(gfx::Canvas* canvas,
+                                     const views::View* view,
+                                     const BrowserView* browser_view,
+                                     gfx::ImageSkia* image);
 
   // 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
-  // figuring out a way to translate the offset correctly regardless of `view`.
-  // Also figure out if tab painting could reuse this logic.
+  // TODO(pbos): Figure out if tab painting could reuse this logic.
   static void PaintBackground(gfx::Canvas* canvas,
                               const views::View* view,
-                              const BrowserView* browser_view,
-                              bool translate_view_coordinates);
+                              const BrowserView* browser_view);
 
  private:
   // views::Background:

chrome/browser/ui/views/side_panel/side_panel.cc

@@ -130,9 +130,7 @@ class SidePanelBorder : public views::Border {
       gfx::ScopedCanvas scoped_rescale(canvas);
       canvas->Scale(dsf, dsf);
 
-      TopContainerBackground::PaintBackground(
-          canvas, &view, browser_view_,
-          /*translate_view_coordinates=*/true);
+      TopContainerBackground::PaintBackground(canvas, &view, browser_view_);
     }
 
     // Paint the inner border around SidePanel content.

chrome/browser/ui/views/tabs/tab_style_views.cc

@@ -19,11 +19,14 @@
 #include "chrome/browser/ui/tabs/tab_types.h"
 #include "chrome/browser/ui/ui_features.h"
 #include "chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/frame/top_container_background.h"
 #include "chrome/browser/ui/views/tabs/glow_hover_controller.h"
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/browser/ui/views/tabs/tab_close_button.h"
 #include "chrome/browser/ui/views/tabs/tab_group_underline.h"
 #include "chrome/browser/ui/views/tabs/tab_slot_controller.h"
+#include "chrome/browser/ui/views/tabs/tab_slot_view.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/tab_groups/tab_group_visual_data.h"
 #include "third_party/skia/include/core/SkRRect.h"
@@ -937,10 +940,12 @@ void GM2TabStyleViews::PaintTabBackgroundFill(
   if (fill_id.has_value()) {
     gfx::ScopedCanvas scale_scoper(canvas);
     canvas->sk_canvas()->scale(scale, scale);
-    canvas->TileImageInt(
-        *tab_->GetThemeProvider()->GetImageSkiaNamed(fill_id.value()),
-        tab_->GetMirroredX() + tab_->controller()->GetBackgroundOffset(), 0, 0,
-        y_inset, tab_->width(), tab_->height());
+    gfx::ImageSkia* image =
+        tab_->GetThemeProvider()->GetImageSkiaNamed(fill_id.value());
+    TopContainerBackground::PaintThemeAlignedImage(
+        canvas, tab_,
+        BrowserView::GetBrowserViewForBrowser(tab_->controller()->GetBrowser()),
+        image);
   }
 
   if (hovered) {

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

@@ -188,8 +188,8 @@ class TabstripLikeBackground : public views::Background {
  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);
+    bool painted = TopContainerBackground::PaintThemeCustomImage(canvas, view,
+                                                                 browser_view_);
     if (!painted) {
       SkColor frame_color =
           browser_view_->frame()->GetFrameView()->GetFrameColor(

ui/views/window/frame_background.cc

@@ -125,8 +125,12 @@ void FrameBackground::PaintMaximized(gfx::Canvas* canvas,
 #endif
 
   // Draw the theme frame and overlay, if available.
+  // See TopContainerBackground::PaintThemeCustomImage for details on the
+  // positioning logic.
   if (!theme_image_.isNull()) {
-    canvas->TileImageInt(theme_image_, 0, theme_image_y_inset_, x, y, width,
+    int x_offset = theme_image_inset_.x() + x;
+    int y_offset = theme_image_inset_.y() + y;
+    canvas->TileImageInt(theme_image_, x_offset, y_offset, x, y, width,
                          top_area_height_, 1.0f, SkTileMode::kRepeat,
                          SkTileMode::kMirror);
   }

ui/views/window/frame_background.h

@@ -7,6 +7,7 @@
 
 #include "base/memory/raw_ptr.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/geometry/point.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/views/views_export.h"
 
@@ -49,8 +50,11 @@ class VIEWS_EXPORT FrameBackground {
   // Memory is owned by the caller.
   void set_theme_image(const gfx::ImageSkia& image) { theme_image_ = image; }
 
-  // Sets an inset into the theme image to begin painting at.
-  void set_theme_image_y_inset(int y_inset) { theme_image_y_inset_ = y_inset; }
+  // Sets an inset into the theme image to begin painting at. This must be
+  // further modified by the origin of the frame.
+  void set_theme_image_inset(gfx::Point theme_image_inset) {
+    theme_image_inset_ = theme_image_inset;
+  }
 
   // Sets an image that overlays the top window image.  Usually used to add
   // edge highlighting to provide the illusion of depth.  May be null (empty).
@@ -108,7 +112,7 @@ class VIEWS_EXPORT FrameBackground {
   bool use_custom_frame_ = true;
   bool is_active_ = true;
   gfx::ImageSkia theme_image_;
-  int theme_image_y_inset_ = 0;
+  gfx::Point theme_image_inset_;
   gfx::ImageSkia theme_overlay_image_;
   int top_area_height_ = 0;