Fix pixelation of tab borders when device scale changes (abandoned)

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 c76f68ce3437911c58368c7bbb32e4e5370cfdb4..5867330eaeaa65ff1af2e98b80bf2773708188ab 100644
--- a/chrome/browser/ui/views/tabs/tab.cc
+++ b/chrome/browser/ui/views/tabs/tab.cc
@@ -44,6 +44,7 @@
 #include "ui/gfx/color_analysis.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/geometry/rect_conversions.h"
+#include "ui/gfx/image/canvas_image_source.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/path.h"
@@ -418,7 +419,6 @@ struct Tab::ImageCacheEntryMetadata {
   ImageCacheEntryMetadata(int resource_id,
                           SkColor fill_color,
                           SkColor stroke_color,
-                          ui::ScaleFactor scale_factor,
                           const gfx::Size& size);
 
   ~ImageCacheEntryMetadata();
@@ -429,7 +429,6 @@ struct Tab::ImageCacheEntryMetadata {
   int resource_id;     // Only needed by pre-MD
   SkColor fill_color;  // Both colors only needed by MD
   SkColor stroke_color;

[Greg Levin, 2016/07/12 23:23:45]

I've removed cache as a differentiator of cache entries.  Am I correct that one
CanvasImageSource will handle all relevant scales?

-  ui::ScaleFactor scale_factor;
   gfx::Size size;
 };
 
@@ -437,15 +436,11 @@ Tab::ImageCacheEntryMetadata::ImageCacheEntryMetadata(
     int resource_id,
     SkColor fill_color,
     SkColor stroke_color,
-    ui::ScaleFactor scale_factor,
     const gfx::Size& size)
     : resource_id(resource_id),
       fill_color(fill_color),
       stroke_color(stroke_color),
-      scale_factor(scale_factor),
       size(size) {
-  DCHECK_NE(ui::SCALE_FACTOR_NONE, scale_factor);
-
   // Some fields are only relevant for pre-MD vs. MD.  Erase the irrelevant ones
   // so they don't cause incorrect cache misses.
   // TODO(pkasting): Remove |resource_id| field when non-MD code is deleted.
@@ -460,8 +455,43 @@ Tab::ImageCacheEntryMetadata::~ImageCacheEntryMetadata() {}
 bool Tab::ImageCacheEntryMetadata::operator==(
     const ImageCacheEntryMetadata& rhs) const {
   return resource_id == rhs.resource_id && fill_color == rhs.fill_color &&
-      stroke_color == rhs.stroke_color && scale_factor == rhs.scale_factor &&
-      size == rhs.size;
+         stroke_color == rhs.stroke_color && size == rhs.size;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Tab::TabImageSource
+//
+// This subclass of CanvasImageSource allows the inactive tab image to be cached
+// once and rendered correctly for all scale factors.
+class Tab::TabImageSource : public gfx::CanvasImageSource {
+ public:
+  TabImageSource(Tab* tab, int fill_id, int y_offset);
+  ~TabImageSource() override {}
+
+  // gfx::CanvasImageSource override.
+  void Draw(gfx::Canvas* canvas) override;
+
+ private:
+  PaintBackgroundParams params_;
+
+  DISALLOW_COPY_AND_ASSIGN(TabImageSource);
+};
+
+Tab::TabImageSource::TabImageSource(Tab* tab, int fill_id, int y_offset)
+    : gfx::CanvasImageSource(tab->size(), true) {
+  params_.tp = tab->GetThemeProvider();
+  params_.hc = nullptr;
+  params_.is_active = false;
+  params_.fill_id = fill_id;
+  params_.has_custom_image = false;
+  params_.x_offset = 0;
+  params_.y_offset = y_offset;
+  params_.size = tab->size();
+  params_.stroke_color = tab->controller()->GetToolbarTopSeparatorColor();
+}
+
+void Tab::TabImageSource::Draw(gfx::Canvas* canvas) {
+  PaintTabBackgroundUsingFillId(canvas, params_);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -469,7 +499,9 @@ bool Tab::ImageCacheEntryMetadata::operator==(
 
 struct Tab::ImageCacheEntry {
   ImageCacheEntry(const ImageCacheEntryMetadata& metadata,
-                  const gfx::ImageSkia& image);
+                  Tab* tab,
+                  int fill_id,
+                  int y_offset);
   ~ImageCacheEntry();
 
   ImageCacheEntryMetadata metadata;
@@ -477,8 +509,11 @@ struct Tab::ImageCacheEntry {
 };
 
 Tab::ImageCacheEntry::ImageCacheEntry(const ImageCacheEntryMetadata& metadata,
-                                      const gfx::ImageSkia& image)
-    : metadata(metadata), image(image) {}
+                                      Tab* tab,
+                                      int fill_id,
+                                      int y_offset)
+    : metadata(metadata),
+      image(new TabImageSource(tab, fill_id, y_offset), tab->size()) {}
 
 Tab::ImageCacheEntry::~ImageCacheEntry() {}
 
@@ -810,7 +845,7 @@ bool Tab::GetHitTestMask(gfx::Path* mask) const {
   if (ui::MaterialDesignController::IsModeMaterial()) {
     SkPath border;
     const float scale = GetWidget()->GetCompositor()->device_scale_factor();
-    GetBorderPath(scale, extend_to_top, &border);
+    GetBorderPath(scale, size(), extend_to_top, &border);
     mask->addPath(border, SkMatrix::MakeScale(1 / scale));
   } else {
     // Hit mask constants.
@@ -1289,16 +1324,12 @@ void Tab::PaintInactiveTabBackground(gfx::Canvas* canvas) {
 
   const ImageCacheEntryMetadata metadata(
       fill_id, tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB),
-      controller_->GetToolbarTopSeparatorColor(),
-      ui::GetSupportedScaleFactor(canvas->image_scale()), size());
+      controller_->GetToolbarTopSeparatorColor(), size());
   auto it = std::find_if(
       image_cache_->begin(), image_cache_->end(),
       [&metadata](const ImageCacheEntry& e) { return e.metadata == metadata; });
   if (it == image_cache_->end()) {
-    gfx::Canvas tmp_canvas(size(), canvas->image_scale(), false);
-    PaintTabBackgroundUsingFillId(&tmp_canvas, false, fill_id, false, y_offset);
-    image_cache_->emplace_front(metadata,
-                                gfx::ImageSkia(tmp_canvas.ExtractImageRep()));
+    image_cache_->emplace_front(metadata, this, fill_id, y_offset);

[Greg Levin, 2016/07/12 23:23:45]

Previously, the image would be rendered here, then passed to the cache.  Now, we
only give the cache the needed parameters to construct a TabImageSource; the
rendering will be done on demand by TabImageSource::Draw.

     if (image_cache_->size() > kMaxImageCacheSize)
       image_cache_->pop_back();
     it = image_cache_->begin();
@@ -1311,21 +1342,40 @@ 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);
+  PaintBackgroundParams params;
+  params.tp = GetThemeProvider();
+  params.hc = &hover_controller_;
+  params.is_active = is_active;
+  params.fill_id = fill_id;
+  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();
+  params.x_offset = GetMirroredX() + background_offset_.x();
+  params.y_offset = y_offset;
+  params.size = size();
+  params.stroke_color = controller_->GetToolbarTopSeparatorColor();
+
+  PaintTabBackgroundUsingFillId(canvas, params);
+}
+
+// static
+// For use by Tab and TabImageSource
+void Tab::PaintTabBackgroundUsingFillId(gfx::Canvas* canvas,
+                                        const PaintBackgroundParams& params) {
+  const SkColor toolbar_color =
+      params.tp->GetColor(ThemeProperties::COLOR_TOOLBAR);
+  gfx::ImageSkia* fill_image = params.tp->GetImageSkiaNamed(params.fill_id);

[oshima, 2016/07/13 20:55:28]

instead of passing tp/fill_id etc, can you just pass

fill_image, tab_color, and hover_color?

[Greg Levin, 2016/07/15 22:24:48]

I'm still constructing TabImageSource with tab and fill_id, but I'm converting
those into fill_image, toolbar_color, and background_color in the TabImageSource
constructor, so that ThemeProvider* isn't stored in the struct.

 
   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()));
+      std::max(SkFloatToScalar(params.size.width() / 4.f), kMinHoverRadius);
+  const bool draw_hover =
+      !params.is_active && params.hc && params.hc->ShouldDraw();

[oshima, 2016/07/13 20:55:28]

you can also just pas nullptr if !ShouldDraw()

[Greg Levin, 2016/07/15 22:24:48]

Done.

+  SkPoint hover_location(
+      PointToSkPoint(draw_hover ? params.hc->location() : gfx::Point()));
   const SkColor hover_color =
-      SkColorSetA(toolbar_color, hover_controller_.GetAlpha());
+      SkColorSetA(toolbar_color, draw_hover ? params.hc->GetAlpha() : 255);
 
   if (ui::MaterialDesignController::IsModeMaterial()) {
     gfx::ScopedCanvas scoped_canvas(canvas);
@@ -1333,23 +1383,24 @@ void Tab::PaintTabBackgroundUsingFillId(gfx::Canvas* canvas,
 
     // Draw the fill.
     SkPath fill;
-    GetFillPath(scale, &fill);
+    GetFillPath(scale, params.size, &fill);
     SkPaint paint;
     paint.setAntiAlias(true);
     {
       gfx::ScopedCanvas clip_scoper(canvas);
       canvas->ClipPath(fill, true);
-      if (has_custom_image) {
+      if (params.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());
+        canvas->TileImageInt(*fill_image, params.x_offset, params.y_offset, 0,
+                             0, params.size.width(), params.size.height());
       } else {
         paint.setColor(
-            is_active ? toolbar_color
-                      : tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB));
-        canvas->DrawRect(gfx::ScaleToEnclosingRect(GetLocalBounds(), scale),
-                         paint);
+            params.is_active
+                ? toolbar_color
+                : params.tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB));
+        canvas->DrawRect(
+            gfx::ScaleToEnclosingRect(gfx::Rect(params.size), scale), paint);
       }
       if (draw_hover) {
         hover_location.scale(SkFloatToScalar(scale));
@@ -1359,50 +1410,55 @@ void Tab::PaintTabBackgroundUsingFillId(gfx::Canvas* canvas,
 
     // Draw the stroke.
     SkPath stroke;
-    GetBorderPath(scale, false, &stroke);
+    GetBorderPath(scale, params.size, false, &stroke);
     Op(stroke, fill, kDifference_SkPathOp, &stroke);
-    if (!is_active) {
+    if (!params.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));
+      canvas->sk_canvas()->clipRect(SkRect::MakeWH(
+          params.size.width() * scale, params.size.height() * scale - 1));
     }
-    paint.setColor(controller_->GetToolbarTopSeparatorColor());
+    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(size(), canvas->image_scale(), false);
-      PaintTabFill(&background_canvas, fill_image, x_offset, y_offset,
-                   is_active);
+      gfx::Canvas background_canvas(params.size, canvas->image_scale(), false);
+      PaintTabFill(&background_canvas, fill_image, params.x_offset,
+                   params.y_offset, params.size, params.is_active);
       gfx::ImageSkia background_image(background_canvas.ExtractImageRep());
       canvas->DrawImageInt(background_image, 0, 0);
 
-      gfx::Canvas hover_canvas(size(), canvas->image_scale(), false);
+      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, fill_image, x_offset, y_offset, is_active);
+      PaintTabFill(canvas, fill_image, params.x_offset, params.y_offset,

[Greg Levin, 2016/07/12 23:23:45]

I'm a little concerned about this use of x_offset.  As I understand the code,
this block is triggered for non-MD, inactive, unhovered tabs.  In that case,
this function is being called by TabImageSource::Draw, where x_offset = 0, not a
tab-specific value.  On the other hand, it kinda looks like the image painted
and cached here by the old version of the code was using an x_offset specific to
the originating tab, so maybe it's fine.

+                   params.size, params.is_active);
     }
 
     // Now draw the stroke, highlights, and shadows around the tab edge.
-    TabImages* stroke_images = is_active ? &active_images_ : &inactive_images_;
+    TabImages* stroke_images =
+        params.is_active ? &active_images_ : &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());
+        params.size.width() - stroke_images->l_width - stroke_images->r_width,
+        params.size.height());
     canvas->DrawImageInt(*stroke_images->image_r,
-                         width() - stroke_images->r_width, 0);
+                         params.size.width() - stroke_images->r_width, 0);
   }
 }
 
+// static
 void Tab::PaintTabFill(gfx::Canvas* canvas,
                        gfx::ImageSkia* fill_image,
                        int x_offset,
                        int y_offset,
+                       const gfx::Size& size,
                        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
@@ -1411,7 +1467,7 @@ void Tab::PaintTabFill(gfx::Canvas* canvas,
 
   // Draw left edge.
   gfx::ImageSkia tab_l = gfx::ImageSkiaOperations::CreateTiledImage(
-      *fill_image, x_offset, y_offset, mask_images_.l_width, height());
+      *fill_image, x_offset, y_offset, mask_images_.l_width, size.height());
   gfx::ImageSkia theme_l =
       gfx::ImageSkiaOperations::CreateMaskedImage(tab_l, *mask_images_.image_l);
   canvas->DrawImageInt(
@@ -1420,22 +1476,22 @@ void Tab::PaintTabFill(gfx::Canvas* canvas,
 
   // Draw right edge.
   gfx::ImageSkia tab_r = gfx::ImageSkiaOperations::CreateTiledImage(
-      *fill_image, x_offset + width() - mask_images_.r_width, y_offset,
-      mask_images_.r_width, height());
+      *fill_image, x_offset + size.width() - mask_images_.r_width, y_offset,
+      mask_images_.r_width, size.height());
   gfx::ImageSkia theme_r =
       gfx::ImageSkiaOperations::CreateMaskedImage(tab_r, *mask_images_.image_r);
   canvas->DrawImageInt(theme_r, 0, 0, theme_r.width(),
                        theme_r.height() - toolbar_overlap,
-                       width() - theme_r.width(), 0, theme_r.width(),
+                       size.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 + mask_images_.l_width,
-                       y_offset + tab_insets.top(), mask_images_.l_width,
-                       tab_insets.top(),
-                       width() - mask_images_.l_width - mask_images_.r_width,
-                       height() - tab_insets.top() - toolbar_overlap);
+  canvas->TileImageInt(
+      *fill_image, x_offset + mask_images_.l_width, y_offset + tab_insets.top(),
+      mask_images_.l_width, tab_insets.top(),
+      size.width() - mask_images_.l_width - mask_images_.r_width,
+      size.height() - tab_insets.top() - toolbar_overlap);
 }
 
 void Tab::PaintPinnedTabTitleChangedIndicatorAndIcon(
@@ -1674,11 +1730,12 @@ void Tab::ScheduleIconPaint() {
   SchedulePaintInRect(bounds);
 }
 
-void Tab::GetFillPath(float scale, SkPath* fill) const {
-  const float right = width() * scale;
+// static
+void Tab::GetFillPath(float scale, const gfx::Size& size, SkPath* fill) {
+  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(height() * scale);
+  const float bottom = std::ceil(size.height() * scale);
   const float unscaled_endcap_width = GetUnscaledEndcapWidth();
 
   fill->moveTo(right - 1, bottom);
@@ -1705,10 +1762,14 @@ void Tab::GetFillPath(float scale, SkPath* fill) const {
   fill->close();
 }
 
-void Tab::GetBorderPath(float scale, bool extend_to_top, SkPath* path) const {
+// static
+void Tab::GetBorderPath(float scale,
+                        const gfx::Size& size,
+                        bool extend_to_top,
+                        SkPath* path) {
   const float top = scale - 1;
-  const float right = width() * scale;
-  const float bottom = height() * scale;
+  const float right = size.width() * scale;
+  const float bottom = size.height() * scale;
   const float unscaled_endcap_width = GetUnscaledEndcapWidth();
 
   path->moveTo(0, bottom);