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 f6ea473a87b0a0f68ab4a386f9740a41eff69c0c..d32e8cdc01c9a987f924e782db6d70943453e392 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"
@@ -119,19 +120,25 @@ const char kTabCloseButtonName[] = "TabCloseButton";
 // Parameters for PaintTabBackgroundUsingParams().
 struct PaintBackgroundParams {
   PaintBackgroundParams(bool is_active,
-                        gfx::ImageSkia* fill_image_ptr,
+                        gfx::ImageSkia* fill_image,
                         bool has_custom_image,
-                        gfx::Rect rect,
+                        const gfx::Rect& rect,
                         SkColor stroke_color,
                         SkColor toolbar_color,
-                        SkColor background_color)
+                        SkColor background_color,
+                        bool draw_hover,
+                        const gfx::Point& hover_location,
+                        SkAlpha hover_alpha)
       : is_active(is_active),
-        fill_image(fill_image_ptr ? *fill_image_ptr : gfx::ImageSkia()),
+        fill_image(fill_image ? *fill_image : gfx::ImageSkia()),
         has_custom_image(has_custom_image),
         rect(rect),
         stroke_color(stroke_color),
         toolbar_color(toolbar_color),
-        background_color(background_color) {}
+        background_color(background_color),
+        draw_hover(draw_hover),
+        hover_location(PointToSkPoint(hover_location)),
+        hover_alpha(hover_alpha) {}
 
   const bool is_active;
   const gfx::ImageSkia fill_image;
@@ -140,8 +147,51 @@ struct PaintBackgroundParams {
   const SkColor stroke_color;
   const SkColor toolbar_color;
   const SkColor background_color;
+  const bool draw_hover;
+  const SkPoint hover_location;
+  const SkAlpha hover_alpha;
 };
 
+void PaintTabBackgroundUsingParams(gfx::Canvas* canvas,
+                                   const PaintBackgroundParams& params);

[Peter Kasting, 2016/08/10 21:02:03]

Nit: Maybe this function should have gone here?...

[Greg Levin, 2016/08/12 19:48:15]

I'll move it (well, "them" now) if you think that't better.  I put it where I
did for consistency... to keep structs and classes together above, and
non-member paint utility functions together below.  I felt the extra declaration
was worth keeping like content grouped in the file.

+
+////////////////////////////////////////////////////////////////////////////////
+// TabImageSource
+//
+// This subclass of CanvasImageSource allows the inactive tab image to be cached
+// once and rendered correctly for all scale factors.

[Peter Kasting, 2016/08/10 21:02:03]

Nit: Maybe "...inactive tab image to be cached once for each requested scale
factor."?  Since we don't really cache the image "once" across all scale
factors.

[Greg Levin, 2016/08/12 19:48:15]

Done.

+class TabImageSource : public gfx::CanvasImageSource {
+ public:
+  TabImageSource(Tab* tab, int fill_id, int y_offset);
+  ~TabImageSource() override {}
+
+  // gfx::CanvasImageSource override.

[Peter Kasting, 2016/08/10 21:02:03]

I think you should also override HasRepresentationAtAllScales() to return true? 
(I'm not terribly familiar with ImageSkiaSource.)

[Greg Levin, 2016/08/12 19:48:15]

Done.

+  void Draw(gfx::Canvas* canvas) override;
+
+ private:
+  const PaintBackgroundParams params_;
+
+  DISALLOW_COPY_AND_ASSIGN(TabImageSource);
+};
+
+TabImageSource::TabImageSource(Tab* tab, int fill_id, int y_offset)
+    : gfx::CanvasImageSource(tab->size(), true),
+      params_(false,
+              tab->GetThemeProvider()->GetImageSkiaNamed(fill_id),
+              false,
+              gfx::Rect(gfx::Point(0, y_offset), tab->size()),
+              tab->controller()->GetToolbarTopSeparatorColor(),
+              tab->GetThemeProvider()->GetColor(ThemeProperties::COLOR_TOOLBAR),
+              tab->GetThemeProvider()->GetColor(
+                  ThemeProperties::COLOR_BACKGROUND_TAB),
+              false,
+              gfx::Point(),
+              255) {}
+
+void TabImageSource::Draw(gfx::Canvas* canvas) {
+  PaintTabBackgroundUsingParams(canvas, params_);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // ImageCacheEntryMetadata
 //
@@ -150,7 +200,6 @@ struct ImageCacheEntryMetadata {
   ImageCacheEntryMetadata(int resource_id,
                           SkColor fill_color,
                           SkColor stroke_color,
-                          ui::ScaleFactor scale_factor,
                           const gfx::Size& size);
 
   ~ImageCacheEntryMetadata();
@@ -160,22 +209,17 @@ struct ImageCacheEntryMetadata {
   int resource_id;     // Only needed by pre-MD
   SkColor fill_color;  // Both colors only needed by MD
   SkColor stroke_color;
-  ui::ScaleFactor scale_factor;
   gfx::Size size;
 };
 
 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.
@@ -190,8 +234,7 @@ ImageCacheEntryMetadata::~ImageCacheEntryMetadata() {}
 bool 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;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -200,7 +243,9 @@ bool ImageCacheEntryMetadata::operator==(
 // A cached image and the metadata used to generate it.
 struct ImageCacheEntry {
   ImageCacheEntry(const ImageCacheEntryMetadata& metadata,
-                  const gfx::ImageSkia& image);
+                  Tab* tab,
+                  int fill_id,
+                  int y_offset);
   ~ImageCacheEntry();
 
   ImageCacheEntryMetadata metadata;
@@ -208,8 +253,11 @@ struct ImageCacheEntry {
 };
 
 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()) {}

[Peter Kasting, 2016/08/10 21:02:03]

I don't suppose you'd like to write a followup cleanup CL that makes ImageSkia
(and ImageSkiaStorage) take a unique_ptr instead of a raw pointer here, so
reviewers didn't have to worry about whether this was a memory leak?

[Greg Levin, 2016/08/12 19:48:15]

I'll ask oshima@ about it...

 
 ImageCacheEntry::~ImageCacheEntry() {}
 
@@ -473,16 +521,12 @@ void PaintTabFill(gfx::Canvas* canvas,
 }
 
 void PaintTabBackgroundUsingParams(gfx::Canvas* canvas,
-                                   views::GlowHoverController* hc,
                                    const PaintBackgroundParams& params) {
   const SkScalar kMinHoverRadius = 16;
   const SkScalar radius =
       std::max(SkFloatToScalar(params.rect.width() / 4.f), kMinHoverRadius);
-  const bool draw_hover = !params.is_active && hc;
-  SkPoint hover_location(
-      gfx::PointToSkPoint(draw_hover ? hc->location() : gfx::Point()));
   const SkColor hover_color =
-      SkColorSetA(params.toolbar_color, draw_hover ? hc->GetAlpha() : 255);
+      SkColorSetA(params.toolbar_color, params.hover_alpha);
 
   if (ui::MaterialDesignController::IsModeMaterial()) {
     gfx::ScopedCanvas scoped_canvas(canvas);
@@ -508,9 +552,9 @@ void PaintTabBackgroundUsingParams(gfx::Canvas* canvas,
             gfx::ScaleToEnclosingRect(gfx::Rect(params.rect.size()), scale),
             paint);
       }
-      if (draw_hover) {
-        hover_location.scale(SkFloatToScalar(scale));
-        DrawHighlight(canvas, hover_location, radius * scale, hover_color);
+      if (params.draw_hover) {
+        DrawHighlight(canvas, params.hover_location * SkFloatToScalar(scale),

[Peter Kasting, 2016/08/10 21:02:03]

I think you want to omit the SkFloatToScalar() call here.

[Greg Levin, 2016/08/12 19:48:15]

Done.

+                      radius * scale, hover_color);
       }
     }
 
@@ -526,7 +570,7 @@ void PaintTabBackgroundUsingParams(gfx::Canvas* canvas,
     paint.setColor(params.stroke_color);
     canvas->DrawPath(stroke, paint);
   } else {
-    if (draw_hover) {
+    if (params.draw_hover) {
       // Draw everything to a temporary canvas so we can extract an image for
       // use in masking the hover glow.
       gfx::Canvas background_canvas(params.rect.size(), canvas->image_scale(),
@@ -538,7 +582,7 @@ void PaintTabBackgroundUsingParams(gfx::Canvas* canvas,
 
       gfx::Canvas hover_canvas(params.rect.size(), canvas->image_scale(),
                                false);
-      DrawHighlight(&hover_canvas, hover_location, radius, hover_color);
+      DrawHighlight(&hover_canvas, params.hover_location, radius, hover_color);
       gfx::ImageSkia result = gfx::ImageSkiaOperations::CreateMaskedImage(
           gfx::ImageSkia(hover_canvas.ExtractImageRep()), background_image);
       canvas->DrawImageInt(result, 0, 0);
@@ -1572,16 +1616,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());

[Peter Kasting, 2016/08/10 21:02:03]

Fundamentally, why didn't this prevent the bug from occurring?  It seems like
when the scale factor changed, this value should have changed, and we should
have thrown out the cached image.

[Greg Levin, 2016/08/12 19:48:15]

In the case where the device / native scale factor is 1.25 (e.g.,
--ash-host-window-bounds=1920x1080*1.25),
canvas->image_scale() returns 1.25 at default resolution, and 1 the rest of the
time (under all other CTRL+SHIFT+ +/- induced zooms).  So most of the time,
zooming doesn't change the canvas's scale.  More to the point,
ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_100P, so this quantity was
constant under all zooms, and had no effect on metadata differentiation.

(I don't know why we don't get
ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_125P
I don't know much about scaling code, and didn't dig any further.  oshima@ knows
a lot more about this stuff.)

[Peter Kasting, 2016/08/13 06:38:33]

Would just removing the call to GetSupportedScaleFactor() (and thus using
image_scale() directly) fix all the problems, or would we still be broken (or be
broken in a new way) when zooming with ctrl-shift-+/-?  (And if broken, how many
other parts of the browser chrome are broken in that case?)
It might be good to get his answer on this if the route above doesn't work as a
solution here, since maybe making that change would help?

[Greg Levin, 2016/08/15 18:40:18]

GetSupportedScaleFactor() is somewhat misleadingly named, and I misunderstood
its purpose.  It doesn't refer to device scales, but to image resource scales. 
That is, we have image resources at 100% and 200%, and those are the supported
scales.  Since our pre-rendered resources only come in those two scales, it
seems we want to stick to those two for these dynamically generated resources as
well.  So ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_100P just means that
100% is a better match than 200% for device scaling of 125%.
Providing images at different scales is the purpose of CanvasImageSource, and
was oshima@'s recommendation for fixing this bug.
Does the above satisfy your concerns here?  Is there anything further you'd like
me to ask or do?

[Peter Kasting, 2016/08/15 21:44:52]

Ah!  That's interesting.

That means at least for MD, we should definitely drop the
GetSupportedScaleFactor call, since we're not using image resources to draw the
tab edges.  I would try just doing that and seeing if it fixes this bug.  If so,
we should maybe undo parts of your previous change to introduce the params
struct.

For pre-MD we maybe should keep using this, but honestly pre-MD is now dead and
we don't care about fixing bugs in it, so I wouldn't worry about trying to
special-case it.

+      controller_->GetToolbarTopSeparatorColor(), 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);
-    PaintTabBackgroundUsingFillId(&tmp_canvas, false, fill_id, false, y_offset);
-    g_image_cache->emplace_front(metadata,
-                                 gfx::ImageSkia(tmp_canvas.ExtractImageRep()));
+    g_image_cache->emplace_front(metadata, this, fill_id, y_offset);
     if (g_image_cache->size() > kMaxImageCacheSize)
       g_image_cache->pop_back();
     it = g_image_cache->begin();
@@ -1594,8 +1634,6 @@ void Tab::PaintTabBackgroundUsingFillId(gfx::Canvas* canvas,
                                         int fill_id,
                                         bool has_custom_image,
                                         int y_offset) {
-  views::GlowHoverController* hc =
-      hover_controller_.ShouldDraw() ? &hover_controller_ : nullptr;
   gfx::ImageSkia* fill_image =
       has_custom_image || !ui::MaterialDesignController::IsModeMaterial()
           ? GetThemeProvider()->GetImageSkiaNamed(fill_id)
@@ -1609,9 +1647,11 @@ void Tab::PaintTabBackgroundUsingFillId(gfx::Canvas* canvas,
       is_active, fill_image, has_custom_image, rect,
       controller_->GetToolbarTopSeparatorColor(),
       GetThemeProvider()->GetColor(ThemeProperties::COLOR_TOOLBAR),
-      GetThemeProvider()->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB));
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB),
+      !is_active && hover_controller_.ShouldDraw(),
+      hover_controller_.location(), hover_controller_.GetAlpha());
 
-  PaintTabBackgroundUsingParams(canvas, hc, params);
+  PaintTabBackgroundUsingParams(canvas, params);
 }
 
 void Tab::PaintPinnedTabTitleChangedIndicatorAndIcon(