ui: Remove use of bitmaps when painting tab backgrounds.

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 2ffdb637649a0ae160fef1571625e00f30de891b..aa8535345443b96946352039e15751a8831472db 100644
--- a/chrome/browser/ui/views/tabs/tab.cc
+++ b/chrome/browser/ui/views/tabs/tab.cc
@@ -14,7 +14,7 @@
 #include "base/metrics/user_metrics.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
-#include "cc/paint/paint_shader.h"
+#include "cc/paint/paint_flags.h"
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/browser.h"
@@ -93,95 +93,9 @@ const double kSelectedTabOpacity = 0.3;
 // Inactive selected tabs have their throb value scaled by this.
 const double kSelectedTabThrobScale = 0.95 - kSelectedTabOpacity;
 
-// Max number of images to cache. This has to be at least two since rounding
-// errors may lead to tabs in the same tabstrip having different sizes.
-// 8 = normal/incognito, active/inactive, 2 sizes within tabstrip.
-const size_t kMaxImageCacheSize = 8;
-
 const char kTabCloseButtonName[] = "TabCloseButton";
 
 ////////////////////////////////////////////////////////////////////////////////
-// ImageCacheEntryMetadata
-//
-// All metadata necessary to uniquely identify a cached image.
-struct ImageCacheEntryMetadata {
-  ImageCacheEntryMetadata(SkColor fill_color,
-                          SkColor stroke_color,
-                          bool use_fill_and_stroke_images,
-                          float scale_factor,
-                          const gfx::Size& size);
-
-  ~ImageCacheEntryMetadata();
-
-  bool operator==(const ImageCacheEntryMetadata& rhs) const;
-
-  SkColor fill_color;
-  SkColor stroke_color;
-  bool use_fill_and_stroke_images;
-  float scale_factor;
-  gfx::Size size;
-};
-
-ImageCacheEntryMetadata::ImageCacheEntryMetadata(
-    SkColor fill_color,
-    SkColor stroke_color,
-    bool use_fill_and_stroke_images,
-    float scale_factor,
-    const gfx::Size& size)
-    : fill_color(fill_color),
-      stroke_color(stroke_color),
-      use_fill_and_stroke_images(use_fill_and_stroke_images),
-      scale_factor(scale_factor),
-      size(size) {}
-
-ImageCacheEntryMetadata::~ImageCacheEntryMetadata() {}
-
-bool ImageCacheEntryMetadata::operator==(
-    const ImageCacheEntryMetadata& rhs) const {
-  return fill_color == rhs.fill_color && stroke_color == rhs.stroke_color &&
-         use_fill_and_stroke_images == rhs.use_fill_and_stroke_images &&
-         scale_factor == rhs.scale_factor && size == rhs.size;
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// ImageCacheEntry and cache management
-//
-// A cached image and the metadata used to generate it.
-struct ImageCacheEntry {
-  ImageCacheEntry(const ImageCacheEntryMetadata& metadata,
-                  const gfx::ImageSkia& fill_image,
-                  const gfx::ImageSkia& stroke_image);
-  ~ImageCacheEntry();
-
-  ImageCacheEntryMetadata metadata;
-  gfx::ImageSkia fill_image;
-  gfx::ImageSkia stroke_image;
-};
-
-ImageCacheEntry::ImageCacheEntry(const ImageCacheEntryMetadata& metadata,
-                                 const gfx::ImageSkia& fill_image,
-                                 const gfx::ImageSkia& stroke_image)
-    : metadata(metadata), fill_image(fill_image), stroke_image(stroke_image) {}
-
-ImageCacheEntry::~ImageCacheEntry() {}
-
-typedef std::list<ImageCacheEntry> ImageCache;
-
-// As the majority of the tabs are inactive, and painting tabs is slowish,
-// we cache a handful of the inactive tab backgrounds here.
-static ImageCache* g_image_cache = nullptr;
-
-// Performs a one-time initialization of static resources such as tab images.
-void InitTabResources() {
-  static bool initialized = false;
-  if (initialized)
-    return;
-
-  initialized = true;
-  g_image_cache = new ImageCache();
-}
-
-////////////////////////////////////////////////////////////////////////////////
 // Drawing and utility functions
 
 // Returns the width of the tab endcap at scale 1.  More precisely, this is the
@@ -219,6 +133,28 @@ bool ShouldThemifyFaviconForUrl(const GURL& url) {
 // Returns a path corresponding to the tab's content region inside the outer
 // stroke.
 gfx::Path GetFillPath(float scale, const gfx::Size& size) {
+  struct FillPathCache {
+    // The parameters used to build the path.
+    float scale;
+    gfx::Size size;
+
+    // The constructed path based on the parameters.
+    gfx::Path path;
+  };
+  // A cache sorted from most recently used to least.
+  static auto& cache = *new std::list<FillPathCache>;

[Peter Kasting, 2017/04/06 18:49:45]

Can we use a base::MRUCache instead of manually rolling the cache management?

Nit: I tend to like using CR_DEFINE_STATIC_LOCAL for the idiom of declaring a
leaky static local

[danakj, 2017/04/07 19:10:49]

We could, I had looked there first, but it just looked like a heck of a lot of
code to do erase and push_front, it makes us use a std::map and to define
operator<, which both seem overkill for a list of 8 items. We can just walk the
list searching. Do you really think it's a good fit here?
Oh, I didn't know that exists and TBH I'm not sure it should. It's comment is
wrong now, and we've been converting LazyInstances to be what I wrote here. I
get the feeling that macro exists to not write |type| twice, but auto does that
for us now. What value do u get out of the macro that you prefer it?

[Peter Kasting, 2017/04/07 23:34:02]

Would it make sense to move MRUCache to one of the flat container types since
its size is likely small?  Would that also avoid the need to define operator<()?

That would be done as a separate CL of course...

It seems like if we have an MRUCache type, but it doesn't actually fit well
where people want an MRU cache, it's not serving its purpose very well, so I'd
like to fix that.  From the perspective of code here, it's kinda the same set of
tradeoffs as using things from <algorithm>: you get to express more directly and
hopefully with known-bug-free code what you're trying to do, though you may have
to jump through hoops to be able to do it.
Not writing the type twice is nice; trying to be consistent with what other
Chrome code does is nice (at least until threadsafe statics, I think most people
used this macro instead of writing it out); and I find the macro name a bit more
obvious as to its purpose than the actual underlying syntax as used directly
here.

None of these are huge things.  It's possible this is worth a quick discussion
on cxx or chromium-dev to get a feel for how we should recommend people do
things going forward and whether we want to retire the macro.

+  static const int kCacheSize = 8;

[Peter Kasting, 2017/04/06 18:49:45]

Nit: constexpr size_t

[danakj, 2017/04/07 19:10:49]

Done.

+
+  // Find a match in the cache and move it to the front and return it.
+  for (auto it = cache.begin(); it != cache.end(); ++it) {
+    if (it->scale == scale && it->size == size) {
+      std::list<FillPathCache> hit;
+      hit.splice(hit.begin(), cache, it, std::next(it));
+      cache.splice(cache.begin(), std::move(hit));

[Peter Kasting, 2017/04/07 23:34:02]

Can we just use std::rotate?  This should work with lists and seems clearer and
safer than using a temp and some splices.

[danakj, 2017/04/18 16:02:42]

I deleted the cache here anyhow, but I think rotate() would require
moving/copying SkPaths around in the container. I used splice because that let
me use a pointer indirection (list nodes) without dealing with pointers myself
via unique_ptr.

+      return cache.front().path;
+    }
+  }
+
   const float right = size.width() * scale;
   // The bottom of the tab needs to be pixel-aligned or else when we call
   // ClipPath with anti-aliasing enabled it can cause artifacts.
@@ -248,6 +184,16 @@ gfx::Path GetFillPath(float scale, const gfx::Size& size) {
   fill.rCubicTo(-0.375 * scale, scale, -1.25 * scale, 1.5 * scale, -2 * scale,
                 1.5 * scale);
   fill.close();
+
+  // If the cache is max size we need to drop the least recently used from the
+  // cache. Then add the new path to the front of the cache.
+  if (cache.size() >= kCacheSize)
+    cache.pop_back();
+  cache.emplace_front();
+  cache.front().scale = scale;

[Peter Kasting, 2017/04/06 18:49:45]

Nit: I'd rather define a constructor so we can just call emplace_front() with
these args

[danakj, 2017/04/07 19:10:49]

Done. Made a constructor and a HasMatchingParameters, and it's now a class not a
struct.

+  cache.front().size = size;
+  cache.front().path = fill;
+
   return fill;
 }
 
@@ -534,7 +480,6 @@ Tab::Tab(TabController* controller, gfx::AnimationContainer* container)
       showing_close_button_(false),
       button_color_(SK_ColorTRANSPARENT) {
   DCHECK(controller);
-  InitTabResources();
 
   // So we get don't get enter/exit on children and don't prematurely stop the
   // hover.
@@ -1123,8 +1068,8 @@ void Tab::PaintTab(gfx::Canvas* canvas, const gfx::Path& clip) {
                                                             : 0;
   const int y_offset = -GetLayoutInsets(TAB).top();
   if (IsActive()) {
-    PaintTabBackgroundUsingFillId(canvas, canvas, true, kActiveTabFillId,
-                                  y_offset);
+    PaintTabBackground(canvas, true /* active */, kActiveTabFillId, y_offset,
+                       nullptr /* clip */);
   } else {
     PaintInactiveTabBackground(canvas, clip);
 
@@ -1132,8 +1077,8 @@ void Tab::PaintTab(gfx::Canvas* canvas, const gfx::Path& clip) {
     if (throb_value > 0) {
       canvas->SaveLayerAlpha(gfx::ToRoundedInt(throb_value * 0xff),
                              GetLocalBounds());
-      PaintTabBackgroundUsingFillId(canvas, canvas, true, kActiveTabFillId,
-                                    y_offset);
+      PaintTabBackground(canvas, true /* active */, kActiveTabFillId, y_offset,
+                         nullptr /* clip */);
       canvas->Restore();
     }
   }
@@ -1146,130 +1091,97 @@ void Tab::PaintInactiveTabBackground(gfx::Canvas* canvas,
                                      const gfx::Path& clip) {
   bool has_custom_image;
   int fill_id = controller_->GetBackgroundResourceId(&has_custom_image);
-  const ui::ThemeProvider* tp = GetThemeProvider();
 
-  // We only cache the image when it's the default image and we're not hovered,
-  // to avoid caching a background image that isn't the same for all tabs.
   if (has_custom_image) {
+    const ui::ThemeProvider* tp = GetThemeProvider();
+
     // If the theme is providing a custom background image, then its top edge
     // should be at the top of the tab. Otherwise, we assume that the background
     // image is a composited foreground + frame image.  Note that if the theme
     // is only providing a custom frame image, |has_custom_image| will be true,
     // but we should use the |background_offset_| here.
-    const int y_offset =
-        tp->HasCustomImage(fill_id) ? 0 : background_offset_.y();
-    PaintTabBackgroundUsingFillId(canvas, canvas, false, fill_id, y_offset);
-    return;
+    int y_offset = tp->HasCustomImage(fill_id) ? 0 : background_offset_.y();
+    PaintTabBackground(canvas, false /* active */, fill_id, y_offset,
+                       nullptr /* clip */);
+  } else {
+    PaintTabBackground(canvas, false /* active */, 0 /* fill_id */,
+                       0 /* y_offset */,
+                       controller_->MaySetClip() ? &clip : nullptr);
   }
-  if (hover_controller_.ShouldDraw()) {
-    PaintTabBackgroundUsingFillId(canvas, canvas, false, 0, 0);
-    return;
+}
+
+void Tab::PaintTabBackground(gfx::Canvas* canvas,
+                             bool active,
+                             int fill_id,
+                             int y_offset,
+                             const gfx::Path* clip) {
+  gfx::Path fill_path = GetFillPath(canvas->image_scale(), size());
+
+  PaintTabBackgroundFill(canvas, fill_path, active, fill_id, y_offset);
+  {

[Peter Kasting, 2017/04/06 18:49:45]

Nit: You could save a line or two and reduce the indenting by just not scoping
this, since it's the last thing in the function (so the actual lifetimes won't
be expanded), and we're unlikely to add more drawing later... up to you

[danakj, 2017/04/07 19:10:49]

Done.

+    gfx::ScopedCanvas scoped_canvas(clip ? canvas : nullptr);
+    if (clip)
+      canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true);
+    PaintTabBackgroundStroke(canvas, fill_path, active);
   }
+}
 
-  // For efficiency, we don't use separate fill and stroke images unless we
-  // really need to clip the stroke and not the fill (for stacked tabs).  This
-  // saves memory and avoids an extra image draw at the cost of recalculating
-  // the images when MaySetClip() toggles.
-  const bool use_fill_and_stroke_images = controller_->MaySetClip();
-
-  const ImageCacheEntryMetadata metadata(
-      tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB),
-      controller_->GetToolbarTopSeparatorColor(), use_fill_and_stroke_images,
-      canvas->image_scale(), size());
-  auto it = std::find_if(
-      g_image_cache->begin(), g_image_cache->end(),
-      [&metadata](const ImageCacheEntry& e) { return e.metadata == metadata; });
-  if (it == g_image_cache->end()) {
-    gfx::Canvas tmp_canvas(size(), canvas->image_scale(), false);
-    if (use_fill_and_stroke_images) {
-      gfx::Canvas tmp_fill_canvas(size(), canvas->image_scale(), false);
-      PaintTabBackgroundUsingFillId(&tmp_fill_canvas, &tmp_canvas, false, 0, 0);
-      g_image_cache->emplace_front(
-          metadata, gfx::ImageSkia(tmp_fill_canvas.ExtractImageRep()),
-          gfx::ImageSkia(tmp_canvas.ExtractImageRep()));
-    } else {
-      PaintTabBackgroundUsingFillId(&tmp_canvas, &tmp_canvas, false, 0, 0);
-      g_image_cache->emplace_front(
-          metadata, gfx::ImageSkia(),
-          gfx::ImageSkia(tmp_canvas.ExtractImageRep()));
-    }
-    if (g_image_cache->size() > kMaxImageCacheSize)
-      g_image_cache->pop_back();
-    it = g_image_cache->begin();
+void Tab::PaintTabBackgroundFill(gfx::Canvas* canvas,
+                                 const gfx::Path& fill_path,
+                                 bool active,
+                                 int fill_id,
+                                 int y_offset) {
+  const ui::ThemeProvider* tp = GetThemeProvider();
+  SkColor active_color = tp->GetColor(ThemeProperties::COLOR_TOOLBAR);
+  SkColor inactive_color = tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB);
+
+  gfx::ScopedCanvas scoped_canvas(canvas);
+  const float scale = canvas->UndoDeviceScaleFactor();
+
+  canvas->ClipPath(fill_path, true);
+  if (fill_id) {
+    gfx::ScopedCanvas scale_scoper(canvas);
+    canvas->sk_canvas()->scale(scale, scale);
+    canvas->TileImageInt(*tp->GetImageSkiaNamed(fill_id),
+                         GetMirroredX() + background_offset_.x(), y_offset, 0,
+                         0, width(), height());
+  } else {
+    cc::PaintFlags flags;
+    flags.setAntiAlias(true);
+    flags.setColor(active ? active_color : inactive_color);
+    canvas->DrawRect(gfx::ScaleToEnclosingRect(GetLocalBounds(), scale), flags);
   }
 
-  gfx::ScopedCanvas scoped_canvas(
-      use_fill_and_stroke_images ? canvas : nullptr);
-  if (use_fill_and_stroke_images) {
-    canvas->DrawImageInt(it->fill_image, 0, 0);
-    canvas->sk_canvas()->clipPath(clip, SkClipOp::kDifference, true);
+  if (!active && hover_controller_.ShouldDraw()) {
+    SkPoint hover_location(gfx::PointToSkPoint(hover_controller_.location()));
+    hover_location.scale(SkFloatToScalar(scale));
+    const SkScalar kMinHoverRadius = 16;
+    const SkScalar radius =
+        std::max(SkFloatToScalar(width() / 4.f), kMinHoverRadius);
+    DrawHighlight(canvas, hover_location, radius * scale,
+                  SkColorSetA(active_color, hover_controller_.GetAlpha()));
   }
-  canvas->DrawImageInt(it->stroke_image, 0, 0);
 }
 
-void Tab::PaintTabBackgroundUsingFillId(gfx::Canvas* fill_canvas,
-                                        gfx::Canvas* stroke_canvas,
-                                        bool is_active,
-                                        int fill_id,
-                                        int y_offset) {
-  gfx::Path fill;
-  cc::PaintFlags flags;
-  flags.setAntiAlias(true);
+void Tab::PaintTabBackgroundStroke(gfx::Canvas* canvas,
+                                   const gfx::Path& fill_path,
+                                   bool active) {
+  SkColor color = controller_->GetToolbarTopSeparatorColor();
 
-  // Draw the fill.
-  {
-    gfx::ScopedCanvas scoped_canvas(fill_canvas);
-    const float scale = fill_canvas->UndoDeviceScaleFactor();
-    const ui::ThemeProvider* tp = GetThemeProvider();
-    const SkColor toolbar_color = tp->GetColor(ThemeProperties::COLOR_TOOLBAR);
-
-    fill = GetFillPath(scale, size());
-    {
-      gfx::ScopedCanvas clip_scoper(fill_canvas);
-      fill_canvas->ClipPath(fill, true);
-      if (fill_id) {
-        gfx::ScopedCanvas scale_scoper(fill_canvas);
-        fill_canvas->sk_canvas()->scale(scale, scale);
-        fill_canvas->TileImageInt(*tp->GetImageSkiaNamed(fill_id),
-                                  GetMirroredX() + background_offset_.x(),
-                                  y_offset, 0, 0, width(), height());
-      } else {
-        flags.setColor(
-            is_active ? toolbar_color
-                      : tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB));
-        fill_canvas->DrawRect(
-            gfx::ScaleToEnclosingRect(GetLocalBounds(), scale), flags);
-      }
+  gfx::ScopedCanvas scoped_canvas(canvas);
+  const float scale = canvas->UndoDeviceScaleFactor();
 
-      if (!is_active && hover_controller_.ShouldDraw()) {
-        SkPoint hover_location(
-            gfx::PointToSkPoint(hover_controller_.location()));
-        hover_location.scale(SkFloatToScalar(scale));
-        const SkScalar kMinHoverRadius = 16;
-        const SkScalar radius =
-            std::max(SkFloatToScalar(width() / 4.f), kMinHoverRadius);
-        DrawHighlight(fill_canvas, hover_location, radius * scale,
-                      SkColorSetA(toolbar_color, hover_controller_.GetAlpha()));
-      }
-    }
-  }
-
-  // Draw the stroke.
-  {
-    gfx::ScopedCanvas scoped_canvas(stroke_canvas);
-    const float scale = stroke_canvas->UndoDeviceScaleFactor();
-
-    gfx::Path stroke = GetBorderPath(scale, false, false, size());
-    Op(stroke, fill, kDifference_SkPathOp, &stroke);
-    if (!is_active) {
-      // Clip out the bottom line; this will be drawn for us by
-      // TabStrip::PaintChildren().
-      stroke_canvas->ClipRect(
-          gfx::RectF(width() * scale, height() * scale - 1));
-    }
-    flags.setColor(controller_->GetToolbarTopSeparatorColor());
-    stroke_canvas->DrawPath(stroke, flags);
+  gfx::Path stroke = GetBorderPath(scale, false, false, size());
+  Op(stroke, fill_path, kDifference_SkPathOp, &stroke);
+  if (!active) {
+    // Clip out the bottom line; this will be drawn for us by
+    // TabStrip::PaintChildren().
+    canvas->ClipRect(gfx::RectF(width() * scale, height() * scale - 1));
   }
+  cc::PaintFlags flags;
+  flags.setAntiAlias(true);
+  flags.setColor(color);
+  canvas->DrawPath(stroke, flags);
 }
 
 void Tab::PaintPinnedTabTitleChangedIndicatorAndIcon(