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

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>
 
@@ skipping 26 matching lines @@
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/models/list_selection_model.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/animation/animation_container.h"
 #include "ui/gfx/animation/throb_animation.h"
 #include "ui/gfx/canvas.h"
 #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"
 #include "ui/gfx/scoped_canvas.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/gfx/vector_icons_public.h"
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/label.h"
@@ skipping 55 matching lines @@
 const double kImmersiveTabMinThrobOpacity = 0.66;
 
 // Number of steps in the immersive mode loading animation.
 const int kImmersiveLoadingStepCount = 32;
 
 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;
   const bool has_custom_image;
   const gfx::Rect rect;
   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
 //
 // All metadata necessary to uniquely identify a cached image.
 struct ImageCacheEntryMetadata {
   ImageCacheEntryMetadata(int resource_id,
                           SkColor fill_color,
                           SkColor stroke_color,
-                          ui::ScaleFactor scale_factor,
                           const gfx::Size& size);
 
   ~ImageCacheEntryMetadata();
 
   bool operator==(const ImageCacheEntryMetadata& rhs) const;
 
   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.
   if (ui::MaterialDesignController::IsModeMaterial())
     resource_id = 0;
   else
     fill_color = stroke_color = SK_ColorTRANSPARENT;
 }
 
 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;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ImageCacheEntry and cache management
 //
 // 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;
   gfx::ImageSkia image;
 };
 
 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() {}
 
 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;
 
 struct TabImages {
@@ skipping 243 matching lines @@
 
   // 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.
   rect.Inset(g_mask_images.l_width, tab_insets.top(), g_mask_images.r_width,
              toolbar_overlap);
   canvas->TileImageInt(fill_image, rect.x(), rect.y(), g_mask_images.l_width,
                        tab_insets.top(), rect.width(), rect.height());
 }
 
 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);
     const float scale = canvas->UndoDeviceScaleFactor();
 
     // Draw the fill.
     gfx::Path fill = GetFillPath(scale, params.rect.size());
     SkPaint paint;
     paint.setAntiAlias(true);
     {
       gfx::ScopedCanvas clip_scoper(canvas);
       canvas->ClipPath(fill, true);
       if (!params.fill_image.isNull()) {
         gfx::ScopedCanvas scale_scoper(canvas);
         canvas->sk_canvas()->scale(scale, scale);
         canvas->TileImageInt(params.fill_image, params.rect.x(),
                              params.rect.y(), 0, 0, params.rect.width(),
                              params.rect.height());
       } else {
         paint.setColor(params.is_active ? params.toolbar_color
                                         : params.background_color);
         canvas->DrawRect(
             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);
       }
     }
 
     // Draw the stroke.
     gfx::Path stroke = GetBorderPath(scale, false, false, params.rect.size());
     Op(stroke, fill, kDifference_SkPathOp, &stroke);
     if (!params.is_active) {
       // Clip out the bottom line; this will be drawn for us by
       // TabStrip::PaintChildren().
       canvas->ClipRect(gfx::RectF(params.rect.width() * scale,
                                   params.rect.height() * scale - 1));
     }
     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(),
                                     false);
       PaintTabFill(&background_canvas, params.fill_image, params.rect,
                    params.is_active);
       gfx::ImageSkia background_image(background_canvas.ExtractImageRep());
       canvas->DrawImageInt(background_image, 0, 0);
 
       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);
     } else {
       PaintTabFill(canvas, params.fill_image, params.rect, params.is_active);
     }
 
     // Now draw the stroke, highlights, and shadows around the tab edge.
     TabImages* stroke_images =
         params.is_active ? &g_active_images : &g_inactive_images;
@@ skipping 1013 matching lines @@
   // 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 || hover_controller_.ShouldDraw()) {
     PaintTabBackgroundUsingFillId(canvas, false, fill_id, has_custom_image,
                                   y_offset);
     return;
   }
 
   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();
   }
   canvas->DrawImageInt(it->image, 0, 0);
 }
 
 void Tab::PaintTabBackgroundUsingFillId(gfx::Canvas* canvas,
                                         bool is_active,
                                         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)
           : nullptr;
   // 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.
   gfx::Rect rect(GetLocalBounds());
   rect.Offset(GetMirroredX() + background_offset_.x(), y_offset);
   PaintBackgroundParams params(
       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(
     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.
@@ skipping 228 matching lines @@
 gfx::Rect Tab::GetImmersiveBarRect() const {
   // The main bar is as wide as the normal tab's horizontal top line.
   // This top line of the tab extends a few pixels left and right of the
   // center image due to pixels in the rounded corner images.
   const int kBarPadding = 1;
   int main_bar_left = g_active_images.l_width - kBarPadding;
   int main_bar_right = width() - g_active_images.r_width + kBarPadding;
   return gfx::Rect(
       main_bar_left, 0, main_bar_right - main_bar_left, kImmersiveBarHeight);
 }