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..af8d3846de022e47f17b9c76f272e39795cf73a9 100644
--- a/chrome/browser/ui/views/tabs/tab.cc
+++ b/chrome/browser/ui/views/tabs/tab.cc
@@ -117,6 +117,20 @@ const int kImmersiveLoadingStepCount = 32;
 const char kTabCloseButtonName[] = "TabCloseButton";
 
 ////////////////////////////////////////////////////////////////////////////////
+// PaintBackgroundParams
+
+struct PaintBackgroundParams {

[Peter Kasting, 2016/07/29 18:58:52]

Nit: I'd probably define this just above the function that uses it, with no
///// dividers around it.  (This file uses a lot of those, and it's a bit of an
outlier in doing so.)  Regardless, wherever you put it the struct should have a
comment above it saying briefly what it's for.

[Greg Levin, 2016/08/08 18:01:32]

In the final CL, this comes right above TabImageSource, a new locally defined
class like ImageCacheEntryMetadata.  That class does call
PaintTabBackgroundUsingFillId(), which will have a prototype between this struct
and TabImageSource.  So putting this struct here is (or will be) sort of right
above the function that uses it.  I've removed the //// divider.

+  bool is_active;
+  gfx::ImageSkia fill_image;
+  bool has_custom_image;
+  gfx::Point offset;
+  gfx::Size size;

[Greg Levin, 2016/07/29 15:02:56]

Are offset and size logically connected enough that it would make sense to
combine them into
  gfx::Rect offset_rect;

[Peter Kasting, 2016/07/29 18:58:52]

Probably, but if you do this I would also change PaintTabFill() to take a rect.

[Greg Levin, 2016/08/08 18:01:32]

Done ... Do you think this is an improvement?  I'm not sure about it's merits,
but it's caused about 10 new line wraps.

+  SkColor stroke_color;
+  SkColor toolbar_color;
+  SkColor background_color;
+};
+
+////////////////////////////////////////////////////////////////////////////////
 // ImageCacheEntryMetadata
 //
 // All metadata necessary to uniquely identify a cached image.
@@ -351,7 +365,7 @@ void GetBorderPath(float scale,
 }
 
 void PaintTabFill(gfx::Canvas* canvas,
-                  gfx::ImageSkia* fill_image,
+                  const gfx::ImageSkia& fill_image,
                   int x_offset,
                   int y_offset,
                   const gfx::Size& size,
@@ -363,7 +377,7 @@ 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, x_offset, y_offset, g_mask_images.l_width, size.height());
   gfx::ImageSkia theme_l = gfx::ImageSkiaOperations::CreateMaskedImage(
       tab_l, *g_mask_images.image_l);
   canvas->DrawImageInt(
@@ -372,7 +386,7 @@ 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,
+      fill_image, x_offset + size.width() - g_mask_images.r_width, y_offset,
       g_mask_images.r_width, size.height());
   gfx::ImageSkia theme_r = gfx::ImageSkiaOperations::CreateMaskedImage(
       tab_r, *g_mask_images.image_r);
@@ -384,12 +398,100 @@ void PaintTabFill(gfx::Canvas* canvas,
   // 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(),
+      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);
 }
 
+// For use by Tab and TabImageSource

[Peter Kasting, 2016/07/29 18:58:52]

Nit: There's no TabImageSource yet, so this comment is kinda weird, but more
importantly, I don't think this helps understand what this function does, and
it's the sort of comment that could get out of date when refactoring.  If you
think the function deserves a comment, it should probably be something more like
"Helper function to paint a tab's background using the information in
|params|.", but honestly that looks to me like it just restates the declaration,
so I'd probably omit it entirely.

[Greg Levin, 2016/08/08 18:01:32]

Done.

+void PaintTabBackgroundUsingFillId(gfx::Canvas* canvas,
+                                   views::GlowHoverController* hc,
+                                   const PaintBackgroundParams& params) {
+  const SkScalar kMinHoverRadius = 16;
+  const SkScalar radius =
+      std::max(SkFloatToScalar(params.size.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.size, &fill);
+    SkPaint paint;
+    paint.setAntiAlias(true);
+    {
+      gfx::ScopedCanvas clip_scoper(canvas);
+      canvas->ClipPath(fill, true);
+      if (params.has_custom_image) {

[Greg Levin, 2016/07/29 15:02:56]

Given the change below at 1586, it looks like we can remove has_custom_image,
and instead use
    if (!fill_image.isNull()) {
here instead.  Do you see any problem with that?

[Peter Kasting, 2016/07/29 18:58:52]

I think that would work.

[Greg Levin, 2016/08/08 18:01:32]

Done.

+        gfx::ScopedCanvas scale_scoper(canvas);
+        canvas->sk_canvas()->scale(scale, scale);
+        canvas->TileImageInt(params.fill_image, params.offset.x(),
+                             params.offset.y(), 0, 0, params.size.width(),
+                             params.size.height());
+      } else {
+        paint.setColor(params.is_active ? params.toolbar_color
+                                        : params.background_color);
+        canvas->DrawRect(
+            gfx::ScaleToEnclosingRect(gfx::Rect(params.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.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.size.width() * scale, params.size.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.size, canvas->image_scale(), false);
+      PaintTabFill(&background_canvas, params.fill_image, params.offset.x(),
+                   params.offset.y(), params.size, params.is_active);
+      gfx::ImageSkia background_image(background_canvas.ExtractImageRep());
+      canvas->DrawImageInt(background_image, 0, 0);
+
+      gfx::Canvas hover_canvas(params.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.x(),
+                   params.offset.y(), params.size, 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.size.width() - stroke_images->l_width - stroke_images->r_width,
+        params.size.height());
+    canvas->DrawImageInt(*stroke_images->image_r,
+                         params.size.width() - stroke_images->r_width, 0);
+  }
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1476,93 +1578,25 @@ 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;

[Peter Kasting, 2016/07/29 18:58:52]

Nit: Looking at the original CL, I see why you didn't put the ShouldDraw() call
here in the callee.  But I'm not sure why you didn't put |canvas| or |hc| into
|params|.  Seems like those are parameters too.

I know one reason not to put |canvas| in, which is that back in your original
CL, |canvas| is the only thing changing on subsequent calls from TabImageSource.
 So if you omit it, and you add a constructor for PaintBackgroundParams, you
could use that constructor to initialize TabImageSource::params_, and declare it
const.  (Which would be nice.)  But you could still have the hover controller be
a param and have that work.

Given that, I suggest making the hover controller part of the params, and adding
a constructor for the params struct, which you can use here too.  I don't think
we lose a lot of readability converting the separate named field assignments
into arguments to a constructor call, since the list of argument names is above
in this same file to reference.

[Greg Levin, 2016/08/08 18:01:32]

Added constructor, will declare params_ const in TabImageSource in other CL.
The hover controller was originally in params, and was removed as per explicit
request by oshima@ (see first comment on Left):

https://codereview.chromium.org/2118853002/diff2/60001:80001/chrome/browser/u...

As noted, making it a separate argument was a suggestion he made offline.  I
don't recall what his objection was to having hc in params, as it's always NULL
(and never cached) in TabImageSource.

As owner, the final call is yours... you want me to put it back in params?  

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

It sounds like the objection was to depending on the full HoverController object
instead of just on the data that one can get from the hover controller.  (This
would matter for e.g. testing if you wanted to test functionality that would
otherwise require a non-null hover controller, but you didn't want to have to
create a hover controller to do so.)

I don't think your current patch does that; it still depends on the hover
controller but takes it as a separate arg, which sounds strictly worse to me.

So I would either put it back in the params, or I would not pass it at all and
find a way to pass the relevant info to the function.  (I don't know whether
that's terribly feasible, and I wouldn't spend a lot of time and effort trying
to do it.)

+  PaintBackgroundParams params;
+  params.is_active = is_active;
+  if (has_custom_image || !ui::MaterialDesignController::IsModeMaterial())
+    params.fill_image = *GetThemeProvider()->GetImageSkiaNamed(fill_id);

[Greg Levin, 2016/07/29 15:02:56]

Changed so that we only load the image if it's going to be used.

+  params.has_custom_image = has_custom_image;
   // 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);
-    }
-
-    // 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);
-  }
+  params.offset.SetPoint(GetMirroredX() + background_offset_.x(), y_offset);
+  params.size = size();
+  params.stroke_color = controller_->GetToolbarTopSeparatorColor();
+  params.toolbar_color =
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_TOOLBAR);
+  params.background_color =
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB);
+
+  ::PaintTabBackgroundUsingFillId(canvas, hc, params);

[Peter Kasting, 2016/07/29 18:58:52]

Nit: This no longer takes a fill ID, so the name isn't quite right.  I'd
probably call it PaintTabBackgroundUsingParams().  That allows eliminating the
:: too.

[Greg Levin, 2016/08/08 18:01:32]

Done.

 }
 
 void Tab::PaintPinnedTabTitleChangedIndicatorAndIcon(