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 "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 332 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 563 matching lines @@
   if (data().blocked == old.blocked)
     return;
 
   if (data().blocked)
     StartPulse();
   else
     StopPulse();
 }
 
 void Tab::PaintTab(gfx::Canvas* canvas, const gfx::Path& clip) {
   const int kActiveTabFillId =

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

btw while we're pointing at naming, these should not be kSyntax cuz they are not
constant for the duration of the program.

       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);
+    bool active = true;
+    gfx::Path* no_clip = nullptr;

[Peter Kasting, 2017/04/06 00:52:11]

Nit: Please don't pull this kind of thing out.  It's actually more cumbersome
for me to read than the inline definition, where if I can't recall the meaning
of the arg both my IDE and codesearch provide it for me very quickly, primarily
because the majority of the time I'm scanning and don't need to know what any of
the params mean.

I'm not trying to ban doing this across Chromium... just don't do it here :)

[danakj, 2017/04/06 17:05:28]

If I didn't give it a name this way I'd want to write write it as "nullptr /*
clip */", but the way you wrote example code below makes me think you don't like
that either. This makes the code harder to read outside of your IDE though. :\ I
can do this but I will have to go look at the function decl to figure out what
false/0/0/nullptr mean here as will most devs, and this gets particularly less
pleasant in places like git blame or looking at diffs which are not part of an
IDE for you either notably.

Does this apply to the active bool also? I am assuming it does.

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

 don't love it enough to do it myself, but I don't mind it near so much, because
it keeps the annotation inline in the call, so it makes it easy to scan past
when you're not caring about that stuff.

If you really wanted to pull something out, I'd do something like

  constexpr bool kDrawAsActive = true;
  constexpr gfx::Path* kDontClip = nullptr;

above the top conditional so these can be reused in the two arms.

Using constexpr kConstantName makes it clearer to me that these aren't
temporaries, just ways of giving values a name.

WDYT?  Is that clearer or less clear than naming the args inline?

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

Sure, I think that's nicer than inline comments. I don't super care for kSyntax
for local variables since it makes inconsistent names with compile-vs-runtime
const variables, but I don't mind and I can see your point about them.

+    PaintTabBackground(canvas, active, kActiveTabFillId, y_offset, no_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);
+      bool active = true;
+      gfx::Path* no_clip = nullptr;
+      PaintTabBackground(canvas, active, kActiveTabFillId, y_offset, no_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();
+  int background_resource_id =
+      controller_->GetBackgroundResourceId(&has_custom_image);
 
-  // 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.
+  int fill_id = 0;
+  int y_offset = 0;
+  bool may_clip = controller_->MaySetClip();
+
   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;
-  }
-  if (hover_controller_.ShouldDraw()) {
-    PaintTabBackgroundUsingFillId(canvas, canvas, false, 0, 0);
-    return;
+    fill_id = background_resource_id;
+    y_offset =
+        tp->HasCustomImage(background_resource_id) ? 0 : background_offset_.y();
+    may_clip = false;
   }
 
-  // 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();
-  }
-
-  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);
+  bool active = false;
+  PaintTabBackground(canvas, active, fill_id, y_offset,
+                     may_clip ? &clip : nullptr);

[Peter Kasting, 2017/04/06 00:52:11]

Nit: All the temps have made this function hard for me to understand, because I
need to keep looking from the function call to multiple different places above
to determine what the values are.

How about:

  bool has_custom_image;
  int fill_id = controller_->GetBackgroundResourceId(&has_custom_image);
  if (has_custom_image) {
    // ...
    const bool fill_id_is_custom = GetThemeProvider()->HasCustomImage(fill_id);
    const int y_offset = fill_id_is_custom ? 0 : background_offset_.y();
    PaintTabBackground(canvas, false, fill_id, y_offset, nullptr);
  } else {
    PaintTabBackground(canvas, false, 0, 0,
                       controller_->MaySetClip() ? &clip : nullptr);
  }

This makes it much more immediately obvious to me that one case paints a custom
image at a possibly-custom offset, and the other doesn't, but might clip.

[danakj, 2017/04/06 17:05:28]

I really liked not splitting this into multiple calls to PaintTabBackground, as
that makes much harder to call it with the wrong things IMO, as there's no
copypasta problems. For instance, was it intended that we only clip in the last
case? It seems weird to me, and there's no comment explaining why like there is
for other things. I think this code makes it more explicit by setting may_clip =
false in the block, rather than 2 function calls where one may have just
forgotten to use the clip over time. But done.

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

After suggesting rewriting yesterday, I asked myself the same question about how
this clipping was really working.  I didn't have time to answer that last night,
but I took a look at it again today.

The short answer is we should clip (if MaySetClip() is true) all the time.

The clip is for the stacked tabs case.  Consider two stacked tabs overlaid like
this:

   ________
  /   /    \
 / A /  B   \
/___/________\

When A is drawn, |clip| is the outer path of B.  We want to clip A's stroke by
this path so we don't get weird "darker stroke" overlap artifacts at the top and
bottom intersection points.  But we _don't_ want to clip A's fill, because B's
left stroke needs to "draw atop" it or it will look like there's a weird gap
back to the window frame behind.

The code was conditional before because when we have cached images, we can't
clip the fill and stroke differently, so we have to pick whether to clip both or
neither, and clipping neither looks less wrong.  What I don't fully understand
is why the custom image case (which never caches) didn't also clip, which could
have been done by refactoring the code.  I suspect the issue is that I
implemented stacked tab support in the new programmatic rendering after I
implemented caching, and so I noticed the clipping artifacts for the cached
images case and fixed them as best I could, but I didn't test stacked tabs +
custom tab theme to notice how this also applied there.  Or maybe I just didn't
realize this could be done better before.

In any case, the interesting question now is what would happen if we basically
forced MaySetClip() to return true all the time (to all callers).  I filed a bug
against myself on the ramifications of that.

OK, so all that said, how should this code be written?

I was trying to come up with a way last night to get the single call you like
while having the temp setting feel more readable to me.  I realized that there's
a way to clean this up more after this refactoring lands, and filed another bug
against myself to do that.  For now we can write something pretty similar to
what you proposed before, and I can simplify later:

  int fill_id = controller_->GetBackgroundResourceId(&has_custom_image);
  int y = 0;
  if (!has_custom_image) {
    fill_id = 0;
  } else if (!GetThemeProvider()->HasCustomImage(fill_id)) {
    // ...
    y = background_offset_.y();
  }
  PaintTabBackground(canvas, false, fill_id, y,
                     controller_->MaySetClip() ? &clip : nullptr);

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

Done. Also observing that we always now clip inactive tabs if MaySetClip(), and
we never clip active tabs.

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

Correct; the active tab should always be frontmost, so it should never need to
be clipped.

 }
 
-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::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());
 
-  // Draw the fill.
+  PaintTabBackgroundFill(canvas, fill_path, active, fill_id, y_offset);
   {
-    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);
-      }
-
-      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()));
-      }
+    base::Optional<gfx::ScopedCanvas> scoped_canvas;

[Peter Kasting, 2017/04/06 00:52:11]

Nit: It seems like it would be marginally simpler to just do:

  gfx::ScopedCanvas clip_scoper(nullptr);
  if (clip) {
    clip_scoper = gfx::ScopedCanvas(canvas);
    canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true);
  }
  PaintTabBackgroundStroke(canvas, fill_path, active);

Of course, just always initing the ScopedCanvas, as the old code did, would be
even simpler.  Can we ensure that Save()/Restore() have no perf cost if there's
no change to the clip/transform/etc. (i.e. the save wasn't actually necessary)? 
That would mean callers could use this freely, without worrying about scoping
only in the cases where it's necessary.

[danakj, 2017/04/06 17:05:28]

Ya that's fair, if you can move, then we can init with nullptr since
ScopedCanvas has a do-nothing mode.  I was kinda building around assuming that
making a ScopedCanvas would always do something but that's not the case. Doing
this the old way has the disadvantage of duplicating the conditional in the call
to the constructor and then below it, which is a potential for bugs if they
diverged at some point unintentionally, and is more branches for a reader to
look at.

I do think that this is less obvious to a reader than optional though:

    gfx::ScopedCanvas scoped_canvas(nullptr);
    if (clip) {
      scoped_canvas = gfx::ScopedCanvas(canvas);

As now you have to know what each constructor of ScopedCanvas would do. Whereas
with optional there's no ScopedCanvas at all so nothing to really know for the
first case.

Skia's raster does have peephole optimizations for removing ops that do nothing,
but I don't think there's anything to handle as complex an interaction as you
propose here. While it's possible we could add something like that into our
future raster implementation, it's always cheaper to avoid doing work than to
undo it later.

I left this the way it was before I got here, and removed the changes to
ScopedCanvas. In the future I think we can make PaintCanvas::Save() return a
scoped object, and then code like this could be more clear without having to
call constructors explicitly and more difficult to do it wrong.

+    if (clip) {
+      scoped_canvas.emplace(canvas);
+      canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true);
     }
-  }
-
-  // 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);
+    PaintTabBackgroundStroke(canvas, fill_path, active);
   }
 }
 
+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);
+  }
+
+  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()));
+  }
+}
+
+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);
 }