ui: Remove use of bitmaps when painting tab backgrounds.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/tabs/tab.h"
 
 #include <stddef.h>
 #include <limits>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/alias.h"
 #include "base/macros.h"
 #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 "cc/paint/paint_recorder.h"
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/tab_contents/core_tab_helper.h"
 #include "chrome/browser/ui/tabs/tab_utils.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/tabs/alert_indicator_button.h"
 #include "chrome/browser/ui/views/tabs/browser_tab_strip_controller.h"
@@ skipping 58 matching lines @@
 
 // How opaque to make the hover state (out of 1).
 const double kHoverOpacity = 0.33;
 
 // Opacity of the active tab background painted over inactive selected tabs.
 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
 // width of the curve making up either the outer or inner edge of the stroke;
 // since these two curves are horizontally offset by 1 px (regardless of scale),
 // the total width of the endcap from tab outer edge to the inside end of the
 // stroke inner edge is (GetUnscaledEndcapWidth() * scale) + 1.
 float GetUnscaledEndcapWidth() {
   return GetLayoutInsets(TAB).left() - 0.5f;
 }
@@ skipping 17 matching lines @@
 bool ShouldThemifyFaviconForUrl(const GURL& url) {
   return url.SchemeIs(content::kChromeUIScheme) &&
          url.host() != chrome::kChromeUIHelpHost &&
          url.host() != chrome::kChromeUIUberHost &&
          url.host() != chrome::kChromeUIAppLauncherPageHost;
 }
 
 // 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>;
+  static const int kCacheSize = 8;
+
+  // 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));
+      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.
   const float bottom = std::ceil(size.height() * scale);
   const float unscaled_endcap_width = GetUnscaledEndcapWidth();
 
   gfx::Path fill;
   fill.moveTo(right - 1, bottom);
   fill.rCubicTo(-0.75 * scale, 0, -1.625 * scale, -0.5 * scale, -2 * scale,
                 -1.5 * scale);
   fill.lineTo(right - 1 - (unscaled_endcap_width - 2) * scale, 2.5 * scale);
   // Prevent overdraw in the center near minimum width (only happens if
   // scale < 2).  We could instead avoid this by increasing the tab inset
   // values, but that would shift all the content inward as well, unless we
   // then overlapped the content on the endcaps, by which point we'd have a
   // huge mess.
   const float scaled_endcap_width = 1 + unscaled_endcap_width * scale;
   const float overlap = scaled_endcap_width * 2 - right;
   const float offset = (overlap > 0) ? (overlap / 2) : 0;
   fill.rCubicTo(-0.375 * scale, -1 * scale, -1.25 * scale + offset,
                 -1.5 * scale, -2 * scale + offset, -1.5 * scale);
   if (overlap < 0)
     fill.lineTo(scaled_endcap_width, scale);
   fill.rCubicTo(-0.75 * scale, 0, -1.625 * scale - offset, 0.5 * scale,
                 -2 * scale - offset, 1.5 * scale);
   fill.lineTo(1 + 2 * scale, bottom - 1.5 * scale);
   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;
+  cache.front().size = size;
+  cache.front().path = fill;
+
   return fill;
 }
 
 // Returns a path corresponding to the tab's outer border for a given tab |size|
 // and |scale|.  If |unscale_at_end| is true, this path will be normalized to a
 // 1x scale by scaling by 1/scale before returning.  If |extend_to_top| is true,
 // the path is extended vertically to the top of the tab bounds.  The caller
 // uses this for Fitts' Law purposes in maximized/fullscreen mode.
 gfx::Path GetBorderPath(float scale,
                         bool unscale_at_end,
@@ skipping 266 matching lines @@
       alert_indicator_button_(nullptr),
       close_button_(nullptr),
       title_(new views::Label()),
       tab_activated_with_last_tap_down_(false),
       hover_controller_(this),
       showing_icon_(false),
       showing_alert_indicator_(false),
       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.
   set_notify_enter_exit_on_child(true);
 
   set_id(VIEW_ID_TAB);
 
   SetBorder(views::CreateEmptyBorder(GetLayoutInsets(TAB)));
 
   title_->SetHorizontalAlignment(gfx::ALIGN_TO_HEAD);
@@ skipping 568 matching lines @@
   else
     StopPulse();
 }
 
 void Tab::PaintTab(gfx::Canvas* canvas, const gfx::Path& clip) {
   const int kActiveTabFillId =
       GetThemeProvider()->HasCustomImage(IDR_THEME_TOOLBAR) ? IDR_THEME_TOOLBAR
                                                             : 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);
 
     const double throb_value = GetThrobValue();
     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();
     }
   }
 
   if (showing_icon_)
     PaintIcon(canvas);
 }
 
 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);
+}
+
+void Tab::PaintTabBackground(gfx::Canvas* canvas,
+                             bool active,
+                             int fill_id,
+                             int y_offset,
+                             const gfx::Path* clip) {
+  // |y_offset| is only set when |fill_id| is being used.
+  DCHECK(!y_offset || !fill_id);

[Peter Kasting, 2017/04/07 00:59:19]

Nit: I'm not sure why this DCHECK is here since, while it's true, it's not
actually an invariant this function relies on or needs to understand.

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

I added this because I think it should be an invariant. Anything that is passed
to PaintTabBackground{Fill,Stroke} needs to be used to invalidate the cache.
However y_offset does not but only because we have a branch earlier that skips
the cache when there's a fill_id, and y_offset is only used with fill_id. This
DCHECK proves that y_offset is always 0 to the caching code.

This was failing in some tests because in the active case we were sometimes
passing a y offset without a fill id. I've fixed that, and it also relates to
the naming of those variables being wrong..they are fixed in the process.

[danakj, 2017/04/07 21:31:08]

Oh I had the logic inverted though. I miss DCHECK_IMPLIES. Fixed it to !y_offset
|| fill_id because y_offset => fill_id.

+
+  const ui::ThemeProvider* tp = GetThemeProvider();
+  SkColor active_color = tp->GetColor(ThemeProperties::COLOR_TOOLBAR);

[Peter Kasting, 2017/04/07 00:59:19]

Nit: These could all be const

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

Sure, I guess I don't see much reason for putting const on local value-type
variables, but I can.

+  SkColor inactive_color = tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB);
+  bool hover = !active && hover_controller_.ShouldDraw();
+
+  // If there is a |fill_id| we don't try to cache. This could be improved
+  // but would require knowing then the image from the ThemeProvider had been
+  // changed, and invalidating when more position values changed.

[Peter Kasting, 2017/04/07 00:59:19]

Nit: more position values changed -> the X coordinate changes?

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

Also background_offset_ (both x() which is used directly and y() which is passed
in as the y_offset).

+  // Stroke does not depend on |fill_id|.
+  // Similarly, if |hover| effects should be shown, we don't try to cache since
+  // they change on every invalidation and we would need to invalidate the cache
+  // based on the hover states.
+  if (fill_id || hover) {
+    gfx::Path fill_path = GetFillPath(canvas->image_scale(), size());
+    PaintTabBackgroundFill(canvas, fill_path, active, hover, active_color,
+                           inactive_color, fill_id, y_offset);
+    gfx::ScopedCanvas scoped_canvas(clip ? canvas : nullptr);
+    if (clip)
+      canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true);

[Peter Kasting, 2017/04/07 00:59:19]

These bits about scoping a canvas, then clipping, are repeated below.  I wonder
if we can use a lambda or binding or something to only write them once.

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

It's a nice thought, the problem is we have to change the PaintTabBackgroundFill
calls out with painting from the cache in the middle of it. I can't think of an
easy way to do this that would not be harder to read.

[Peter Kasting, 2017/04/08 00:59:37]

The way I can think of, which may not be a win, is to basically have something
akin to:

* Create locals for "paint fill" and "paint stroke" which are Closures, or
lambdas or something
* Do something like:

  if (fill_id || hover) {
    paint_fill = Bind(PaintTabBackgroundFill, args);
    paint_stroke = Bind(PaintTabBackgroundStroke, args);
  } else {
    ... check cache
    paint_fill = Bind(PlaybackPaintRecord, args);
    paint_stroke = Bind(PlaybackPaintRecord, args);
  }

  paint_fill();
  gfx::ScopedCanvas scoped_canvas(clip ? canvas : nullptr);
  if (clip)
    canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true);
  canvas->sk_canvas()->PlaybackPaintRecord(cache.stroke_record);

One can imagine splitting the last bit into a function, so the caller here could
avoid temps by instead just calling the function and passing the correct things
directly.

+    PaintTabBackgroundStroke(canvas, fill_path, active);
     return;
   }
 
-  // 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();
+  BackgroundCache& cache =
+      active ? background_active_cache_ : background_inactive_cache_;
+  bool matches_cache = cache.scale == canvas->image_scale() &&

[Peter Kasting, 2017/04/07 00:59:19]

Nit: I liked how the old code had a metadata struct for this stuff that could be
constructed, compared, and used when enqueueing new entries.  It is more verbose
when defining the cache structs, but it lets the code here just say "here's the
metadata for the current object" and then do things like checking for a match or
resetting the cached entry very simply and with minimal repetitiveness.

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

It feels like it should be less, but It's a more lines of code, and they move
further away, to do the same thing, as well as duplicates the storage of all the
fields in the metadata.

 2 files changed, 34 insertions(+), 17 deletions(-)

Is just adding a metadata class, 2 constructors, and a comparator.

Counter proposal, I make a Check and Set methods so we can compile-time verify
that we're not skipping fields?

 2 files changed, 39 insertions(+), 22 deletions(-)

This adds more code but it feels less indirect without making a key structure I
guess, cuz we're comparing and setting to the same values that we see written as
arguments to PaintTabBackground{Fill,Stroke}(). wdyt?

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

It's definitely more verbose overall.  I'm OK with that -- they get out of the
way of code here, so they let the code here be more expressive about what it's
trying to do without spelling things out.

I don't think I follow the duplicating the storage comment.
If I understand what you're proposing correctly, I'm pretty much fine with this,
as it accomplishes the same goal of moving the minutiae of the checks away from
here.  It seems like it would still force you to pass all the args to each
function each time, which is kind of unfortunate.  Maybe we can construct a new
cache entry up-front (without paint records in it) and use it for the
comparison, then if we want to add it set its paint records and add?

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

The duplicate storage I mean like..


float scale ..
Size size..
SkColor active_color..

Metadata m(scale, size, active_color);

if (m != m_)
  DoThings(scale, size, active_color);

m_ = m;

Here we're storing scale/size/active_color a second time in this function,
inside m. But then using it to compare but not using it for the methods. It
disconnects the comparison from passing the things to do the actual work, and we
have 2 scales 2 sizes, 2 colors etc.

Whereas what I did is


float scale ..
Size size..
SkColor active_color..

if (!m_.Compare(scale, size, active_color))
  DoThings(scale, size, active_color);

M_.Set(scale, size, active_color);

This is the same overall work. m_=m would be copying all the local fields too.
But this you see the comparison and the cache are the same physical fields as
used to do the work, and you only have 1 instance of each variable inside the
method.

+                       cache.size == size() &&
+                       cache.active_color == active_color &&
+                       cache.inactive_color == inactive_color;
+  if (!matches_cache) {
+    gfx::Path fill_path = GetFillPath(canvas->image_scale(), size());
+    cc::PaintRecorder recorder;
 
-  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()));
+    {
+      gfx::Canvas cache_canvas(
+          recorder.beginRecording(size().width(), size().height()),
+          canvas->image_scale());
+      PaintTabBackgroundFill(&cache_canvas, fill_path, active, hover,
+                             active_color, inactive_color, fill_id, y_offset);
+      cache.fill_record = recorder.finishRecordingAsPicture();
     }
-    if (g_image_cache->size() > kMaxImageCacheSize)
-      g_image_cache->pop_back();
-    it = g_image_cache->begin();
+    {
+      gfx::Canvas cache_canvas(
+          recorder.beginRecording(size().width(), size().height()),
+          canvas->image_scale());
+      PaintTabBackgroundStroke(canvas, fill_path, active);
+      cache.stroke_record = recorder.finishRecordingAsPicture();
+    }
+
+    cache.scale = canvas->image_scale();
+    cache.size = size();
+    cache.active_color = active_color;
+    cache.inactive_color = inactive_color;
   }
 
-  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);
-  }
-  canvas->DrawImageInt(it->stroke_image, 0, 0);
+  canvas->sk_canvas()->PlaybackPaintRecord(cache.fill_record);
+  gfx::ScopedCanvas scoped_canvas(clip ? canvas : nullptr);
+  if (clip)
+    canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true);
+  canvas->sk_canvas()->PlaybackPaintRecord(cache.stroke_record);
 }
 
-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::PaintTabBackgroundFill(gfx::Canvas* canvas,
+                                 const gfx::Path& fill_path,
+                                 bool active,
+                                 bool hover,
+                                 SkColor active_color,
+                                 SkColor inactive_color,
+                                 int fill_id,
+                                 int y_offset) {
+  gfx::ScopedCanvas scoped_canvas(canvas);
+  const float scale = canvas->UndoDeviceScaleFactor();
 
-  // Draw the fill.
-  {
-    gfx::ScopedCanvas scoped_canvas(fill_canvas);
-    const float scale = fill_canvas->UndoDeviceScaleFactor();
+  canvas->ClipPath(fill_path, true);
+  if (fill_id) {
     const ui::ThemeProvider* tp = GetThemeProvider();

[Peter Kasting, 2017/04/07 00:59:19]

Nit: Just inline this below?

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

Done.

-    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);
-      }
-
-      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()));
-      }
-    }
+    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);
   }
 
-  // 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);
+  if (hover) {

[Peter Kasting, 2017/04/07 00:59:20]

Nit: Maybe name this |draw_highlight| or |draw_hover|?

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

went with paint_hover_effect. "draw" is a overloaded term that i try to only use
when referring to compositor drawing its final output to the display. here we're
painting. I expect our PaintCanvas and such APIs to reflect this better over
time.

+    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()));
   }
 }
 
+void Tab::PaintTabBackgroundStroke(gfx::Canvas* canvas,
+                                   const gfx::Path& fill_path,
+                                   bool active) {
+  SkColor color = controller_->GetToolbarTopSeparatorColor();
+
+  gfx::ScopedCanvas scoped_canvas(canvas);
+  const float scale = canvas->UndoDeviceScaleFactor();
+
+  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(
     gfx::Canvas* canvas,
     const gfx::Rect& favicon_draw_bounds) {
   // The pinned tab title changed indicator consists of two parts:
   // . a clear (totally transparent) part over the bottom right (or left in rtl)
   //   of the favicon. This is done by drawing the favicon to a canvas, then
   //   drawing the clear part on top of the favicon.
   // . a circle in the bottom right (or left in rtl) of the favicon.
   if (!favicon_.isNull()) {
     const float kIndicatorCropRadius = 4.5;
@@ skipping 201 matching lines @@
   gfx::Rect bounds = favicon_bounds_;
   if (bounds.IsEmpty())
     return;
 
   // Extends the area to the bottom when the crash animation is in progress.
   if (crash_icon_animation_->is_animating())
     bounds.set_height(height() - bounds.y());
   bounds.set_x(GetMirroredXForRect(bounds));
   SchedulePaintInRect(bounds);
 }
+
+Tab::BackgroundCache::BackgroundCache() = default;
+Tab::BackgroundCache::~BackgroundCache() = default;