Make PaintTabBackgroundUsingFillId() non-method

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

Index: chrome/browser/ui/views/tabs/tab.cc
diff --git a/chrome/browser/ui/views/tabs/tab.cc b/chrome/browser/ui/views/tabs/tab.cc
index 13209f1bb0e482b956f1a648b0d673e6b5963a6a..1ae84a430c391882f8773a5846e53640f13c4684 100644
--- a/chrome/browser/ui/views/tabs/tab.cc
+++ b/chrome/browser/ui/views/tabs/tab.cc
@@ -116,6 +116,34 @@ const int kImmersiveLoadingStepCount = 32;
 
 const char kTabCloseButtonName[] = "TabCloseButton";
 
+// Parameters for PaintTabBackgroundUsingParams().
+struct PaintBackgroundParams {
+  PaintBackgroundParams(bool is_active,
+                        gfx::ImageSkia* fill_image_ptr,
+                        bool has_custom_image,
+                        gfx::Rect offset_rect,

[Peter Kasting, 2016/08/09 02:52:31]

|offset_rect| feels like a misleading name here.  A point is an offset; the
origin of this rect is an offset.  But that ignores the purpose of the rect's
size.  It seems like this isn't just about giving an offset, but defining the
whole region to paint in.  Probably a more general name like just |rect| would
be less misleading.  (multiple places)

Note that a shorter name would have the side effect of reducing the
line-wrapping you noted was added.

[Greg Levin, 2016/08/09 17:28:18]

Done.

+                        SkColor stroke_color,
+                        SkColor toolbar_color,
+                        SkColor background_color)
+      : is_active(is_active),
+        has_custom_image(has_custom_image),
+        offset_rect(offset_rect),
+        stroke_color(stroke_color),
+        toolbar_color(toolbar_color),
+        background_color(background_color) {
+    if (fill_image_ptr)
+      fill_image = *fill_image_ptr;
+  }
+
+  bool is_active;
+  gfx::ImageSkia fill_image;
+  bool has_custom_image;
+  gfx::Rect offset_rect;
+  SkColor stroke_color;
+  SkColor toolbar_color;
+  SkColor background_color;

[Peter Kasting, 2016/08/09 02:52:31]

Nit: All of these but |fill_image| could be const.  That could be const too with
some clever code in the initializer list.

[Greg Levin, 2016/08/09 17:28:17]

Done.

+};
+
 ////////////////////////////////////////////////////////////////////////////////
 // ImageCacheEntryMetadata
 //
@@ -351,10 +379,8 @@ void GetBorderPath(float scale,
 }
 
 void PaintTabFill(gfx::Canvas* canvas,
-                  gfx::ImageSkia* fill_image,
-                  int x_offset,
-                  int y_offset,
-                  const gfx::Size& size,
+                  const gfx::ImageSkia& fill_image,
+                  gfx::Rect offset_rect,
                   bool is_active) {
   const gfx::Insets tab_insets(GetLayoutInsets(TAB));
   // If this isn't the foreground tab, don't draw over the toolbar, but do
@@ -363,7 +389,8 @@ void PaintTabFill(gfx::Canvas* canvas,
 
   // Draw left edge.
   gfx::ImageSkia tab_l = gfx::ImageSkiaOperations::CreateTiledImage(
-      *fill_image, x_offset, y_offset, g_mask_images.l_width, size.height());
+      fill_image, offset_rect.x(), offset_rect.y(), g_mask_images.l_width,
+      offset_rect.height());
   gfx::ImageSkia theme_l = gfx::ImageSkiaOperations::CreateMaskedImage(
       tab_l, *g_mask_images.image_l);
   canvas->DrawImageInt(
@@ -372,22 +399,115 @@ void PaintTabFill(gfx::Canvas* canvas,
 
   // Draw right edge.
   gfx::ImageSkia tab_r = gfx::ImageSkiaOperations::CreateTiledImage(
-      *fill_image, x_offset + size.width() - g_mask_images.r_width, y_offset,
-      g_mask_images.r_width, size.height());
+      fill_image, offset_rect.x() + offset_rect.width() - g_mask_images.r_width,

[Peter Kasting, 2016/08/09 02:52:31]

Nit: Use right() in place of x() + width()

[Greg Levin, 2016/08/09 17:28:17]

Done.

+      offset_rect.y(), g_mask_images.r_width, offset_rect.height());
   gfx::ImageSkia theme_r = gfx::ImageSkiaOperations::CreateMaskedImage(
       tab_r, *g_mask_images.image_r);
-  canvas->DrawImageInt(theme_r, 0, 0, theme_r.width(),
-                       theme_r.height() - toolbar_overlap,
-                       size.width() - theme_r.width(), 0, theme_r.width(),
-                       theme_r.height() - toolbar_overlap, false);
+  canvas->DrawImageInt(
+      theme_r, 0, 0, theme_r.width(), theme_r.height() - toolbar_overlap,
+      offset_rect.width() - theme_r.width(), 0, theme_r.width(),
+      theme_r.height() - toolbar_overlap, false);
 
   // Draw center.  Instead of masking out the top portion we simply skip over it
   // by incrementing by the top padding, since it's a simple rectangle.
   canvas->TileImageInt(
-      *fill_image, x_offset + g_mask_images.l_width,
-      y_offset + tab_insets.top(), g_mask_images.l_width, tab_insets.top(),
-      size.width() - g_mask_images.l_width - g_mask_images.r_width,
-      size.height() - tab_insets.top() - toolbar_overlap);
+      fill_image, offset_rect.x() + g_mask_images.l_width,
+      offset_rect.y() + tab_insets.top(), g_mask_images.l_width,
+      tab_insets.top(),
+      offset_rect.width() - g_mask_images.l_width - g_mask_images.r_width,
+      offset_rect.height() - tab_insets.top() - toolbar_overlap);

[Peter Kasting, 2016/08/09 02:52:31]

Nit: Shorter:

  const int left = g_mask_images.l_width;
  const int top = tab_insets.top();
  offset_rect.Inset(left, top, g_mask_images.r_width, toolbar_overlap);
  canvas->TileImageInt(fill_image, offset_rect.x(), offset_rect.y(), left, top,
                       offset_rect.width(), offset_rect.height());

(This is all making me think for the millionth time that we should really change
TileImageInt()'s signature, and those of the related functions, to take Points
and Sizes instead of ints.  Then we could just do this:)

  const gfx::Point dest(g_mask_images.l_width, tab_insets.top());
  canvas->TileImageInt(fill_image, offset_rect.origin(), dest,
                       offset_rect.size());

[Greg Levin, 2016/08/09 17:28:17]

The offset_rect -> rect change had already shortened this to 5 lines, so I went
with a 4 line version w/o new left & top variables.

+}
+
+void PaintTabBackgroundUsingParams(gfx::Canvas* canvas,
+                                   views::GlowHoverController* hc,
+                                   const PaintBackgroundParams& params) {
+  const SkScalar kMinHoverRadius = 16;
+  const SkScalar radius = std::max(
+      SkFloatToScalar(params.offset_rect.width() / 4.f), kMinHoverRadius);
+  const bool draw_hover = !params.is_active && hc;
+  SkPoint hover_location(
+      PointToSkPoint(draw_hover ? hc->location() : gfx::Point()));
+  const SkColor hover_color =
+      SkColorSetA(params.toolbar_color, draw_hover ? hc->GetAlpha() : 255);
+
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    gfx::ScopedCanvas scoped_canvas(canvas);
+    const float scale = canvas->UndoDeviceScaleFactor();
+
+    // Draw the fill.
+    SkPath fill;
+    GetFillPath(scale, params.offset_rect.size(), &fill);
+    SkPaint paint;
+    paint.setAntiAlias(true);
+    {
+      gfx::ScopedCanvas clip_scoper(canvas);
+      canvas->ClipPath(fill, true);
+      if (!params.fill_image.isNull()) {
+        gfx::ScopedCanvas scale_scoper(canvas);
+        canvas->sk_canvas()->scale(scale, scale);
+        canvas->TileImageInt(
+            params.fill_image, params.offset_rect.x(), params.offset_rect.y(),
+            0, 0, params.offset_rect.width(), params.offset_rect.height());
+      } else {
+        paint.setColor(params.is_active ? params.toolbar_color
+                                        : params.background_color);
+        canvas->DrawRect(gfx::ScaleToEnclosingRect(
+                             gfx::Rect(params.offset_rect.size()), scale),
+                         paint);
+      }
+      if (draw_hover) {
+        hover_location.scale(SkFloatToScalar(scale));
+        DrawHighlight(canvas, hover_location, radius * scale, hover_color);
+      }
+    }
+
+    // Draw the stroke.
+    SkPath stroke;
+    GetBorderPath(scale, params.offset_rect.size(), false, &stroke);
+    Op(stroke, fill, kDifference_SkPathOp, &stroke);
+    if (!params.is_active) {
+      // Clip out the bottom line; this will be drawn for us by
+      // TabStrip::PaintChildren().
+      canvas->sk_canvas()->clipRect(
+          SkRect::MakeWH(params.offset_rect.width() * scale,
+                         params.offset_rect.height() * scale - 1));
+    }
+    paint.setColor(params.stroke_color);
+    canvas->DrawPath(stroke, paint);
+  } else {
+    if (draw_hover) {
+      // Draw everything to a temporary canvas so we can extract an image for
+      // use in masking the hover glow.
+      gfx::Canvas background_canvas(params.offset_rect.size(),
+                                    canvas->image_scale(), false);
+      PaintTabFill(&background_canvas, params.fill_image, params.offset_rect,
+                   params.is_active);
+      gfx::ImageSkia background_image(background_canvas.ExtractImageRep());
+      canvas->DrawImageInt(background_image, 0, 0);
+
+      gfx::Canvas hover_canvas(params.offset_rect.size(), canvas->image_scale(),
+                               false);
+      DrawHighlight(&hover_canvas, hover_location, radius, hover_color);
+      gfx::ImageSkia result = gfx::ImageSkiaOperations::CreateMaskedImage(
+          gfx::ImageSkia(hover_canvas.ExtractImageRep()), background_image);
+      canvas->DrawImageInt(result, 0, 0);
+    } else {
+      PaintTabFill(canvas, params.fill_image, params.offset_rect,
+                   params.is_active);
+    }
+
+    // Now draw the stroke, highlights, and shadows around the tab edge.
+    TabImages* stroke_images =
+        params.is_active ? &g_active_images : &g_inactive_images;
+    canvas->DrawImageInt(*stroke_images->image_l, 0, 0);
+    canvas->TileImageInt(*stroke_images->image_c, stroke_images->l_width, 0,
+                         params.offset_rect.width() - stroke_images->l_width -
+                             stroke_images->r_width,
+                         params.offset_rect.height());
+    canvas->DrawImageInt(*stroke_images->image_r,
+                         params.offset_rect.width() - stroke_images->r_width,
+                         0);
+  }
 }
 
 }  // namespace
@@ -1476,93 +1596,24 @@ void Tab::PaintTabBackgroundUsingFillId(gfx::Canvas* canvas,
                                         int fill_id,
                                         bool has_custom_image,
                                         int y_offset) {
-  const ui::ThemeProvider* tp = GetThemeProvider();
-  const SkColor toolbar_color = tp->GetColor(ThemeProperties::COLOR_TOOLBAR);
-  gfx::ImageSkia* fill_image = tp->GetImageSkiaNamed(fill_id);
+  views::GlowHoverController* hc =
+      hover_controller_.ShouldDraw() ? &hover_controller_ : nullptr;
+  gfx::ImageSkia* fill_image =
+      has_custom_image || !ui::MaterialDesignController::IsModeMaterial()
+          ? GetThemeProvider()->GetImageSkiaNamed(fill_id)
+          : nullptr;
   // The tab image needs to be lined up with the background image
   // so that it feels partially transparent.  These offsets represent the tab
   // position within the frame background image.
-  const int x_offset = GetMirroredX() + background_offset_.x();
-
-  const SkScalar kMinHoverRadius = 16;
-  const SkScalar radius =
-      std::max(SkFloatToScalar(width() / 4.f), kMinHoverRadius);
-  const bool draw_hover = !is_active && hover_controller_.ShouldDraw();
-  SkPoint hover_location(PointToSkPoint(hover_controller_.location()));
-  const SkColor hover_color =
-      SkColorSetA(toolbar_color, hover_controller_.GetAlpha());
-
-  if (ui::MaterialDesignController::IsModeMaterial()) {
-    gfx::ScopedCanvas scoped_canvas(canvas);
-    const float scale = canvas->UndoDeviceScaleFactor();
-
-    // Draw the fill.
-    SkPath fill;
-    GetFillPath(scale, size(), &fill);
-    SkPaint paint;
-    paint.setAntiAlias(true);
-    {
-      gfx::ScopedCanvas clip_scoper(canvas);
-      canvas->ClipPath(fill, true);
-      if (has_custom_image) {
-        gfx::ScopedCanvas scale_scoper(canvas);
-        canvas->sk_canvas()->scale(scale, scale);
-        canvas->TileImageInt(*fill_image, x_offset, y_offset, 0, 0, width(),
-                             height());
-      } else {
-        paint.setColor(
-            is_active ? toolbar_color
-                      : tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB));
-        canvas->DrawRect(gfx::ScaleToEnclosingRect(GetLocalBounds(), scale),
-                         paint);
-      }
-      if (draw_hover) {
-        hover_location.scale(SkFloatToScalar(scale));
-        DrawHighlight(canvas, hover_location, radius * scale, hover_color);
-      }
-    }
-
-    // Draw the stroke.
-    SkPath stroke;
-    GetBorderPath(scale, size(), false, &stroke);
-    Op(stroke, fill, kDifference_SkPathOp, &stroke);
-    if (!is_active) {
-      // Clip out the bottom line; this will be drawn for us by
-      // TabStrip::PaintChildren().
-      canvas->sk_canvas()->clipRect(
-          SkRect::MakeWH(width() * scale, height() * scale - 1));
-    }
-    paint.setColor(controller_->GetToolbarTopSeparatorColor());
-    canvas->DrawPath(stroke, paint);
-  } else {
-    if (draw_hover) {
-      // Draw everything to a temporary canvas so we can extract an image for
-      // use in masking the hover glow.
-      gfx::Canvas background_canvas(size(), canvas->image_scale(), false);
-      PaintTabFill(&background_canvas, fill_image, x_offset, y_offset, size(),
-                   is_active);
-      gfx::ImageSkia background_image(background_canvas.ExtractImageRep());
-      canvas->DrawImageInt(background_image, 0, 0);
-
-      gfx::Canvas hover_canvas(size(), canvas->image_scale(), false);
-      DrawHighlight(&hover_canvas, hover_location, radius, hover_color);
-      gfx::ImageSkia result = gfx::ImageSkiaOperations::CreateMaskedImage(
-          gfx::ImageSkia(hover_canvas.ExtractImageRep()), background_image);
-      canvas->DrawImageInt(result, 0, 0);
-    } else {
-      PaintTabFill(canvas, fill_image, x_offset, y_offset, size(), is_active);
-    }
+  gfx::Rect offset_rect(GetMirroredX() + background_offset_.x(), y_offset,
+                        width(), height());

[Peter Kasting, 2016/08/09 02:52:31]

Nit: Do you think it's any more readable to do this?  This calls out a little
more (to me) that we're trying to paint "our tab's bounds, relative to some
other point".

  gfx::Rect rect(GetLocalBounds());
  rect.Offset(GetMirroredX() + background_offset_.x(), y_offset);

[Greg Levin, 2016/08/09 17:28:17]

Done.

+  PaintBackgroundParams params(
+      is_active, fill_image, has_custom_image, offset_rect,
+      controller_->GetToolbarTopSeparatorColor(),
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_TOOLBAR),
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB));
 
-    // Now draw the stroke, highlights, and shadows around the tab edge.
-    TabImages* stroke_images =
-        is_active ? &g_active_images : &g_inactive_images;
-    canvas->DrawImageInt(*stroke_images->image_l, 0, 0);
-    canvas->TileImageInt(
-        *stroke_images->image_c, stroke_images->l_width, 0,
-        width() - stroke_images->l_width - stroke_images->r_width, height());
-    canvas->DrawImageInt(*stroke_images->image_r,
-                         width() - stroke_images->r_width, 0);
-  }
+  PaintTabBackgroundUsingParams(canvas, hc, params);
 }
 
 void Tab::PaintPinnedTabTitleChangedIndicatorAndIcon(