[Thumbnails] Specify copy size in Pixels, not DIPs

chrome/browser/thumbnails/simple_thumbnail_crop.cc

Index: chrome/browser/thumbnails/simple_thumbnail_crop.cc
diff --git a/chrome/browser/thumbnails/simple_thumbnail_crop.cc b/chrome/browser/thumbnails/simple_thumbnail_crop.cc
index b426325c7c4e586ad5c465df10a9ca92bcb34c5e..b5eebf869d6996acd78775dc35e4f55ca4234576 100644
--- a/chrome/browser/thumbnails/simple_thumbnail_crop.cc
+++ b/chrome/browser/thumbnails/simple_thumbnail_crop.cc
@@ -7,6 +7,7 @@
 #include "base/metrics/histogram.h"
 #include "content/public/browser/browser_thread.h"
 #include "skia/ext/platform_canvas.h"
+#include "ui/base/layout.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/geometry/size_conversions.h"
 #include "ui/gfx/image/image_skia.h"
@@ -25,15 +26,14 @@ SimpleThumbnailCrop::SimpleThumbnailCrop(const gfx::Size& target_size)
   DCHECK(!target_size.IsEmpty());
 }
 
-ClipResult SimpleThumbnailCrop::GetCanvasCopyInfo(
-    const gfx::Size& source_size,
-    ui::ScaleFactor scale_factor,
-    gfx::Rect* clipping_rect,
-    gfx::Size* target_size) const {
+ClipResult SimpleThumbnailCrop::GetCanvasCopyInfo(const gfx::Size& source_size,
+                                                  ui::ScaleFactor scale_factor,
+                                                  gfx::Rect* clipping_rect,
+                                                  gfx::Size* copy_size) const {
   DCHECK(!source_size.IsEmpty());
   ClipResult clip_result = thumbnails::CLIP_RESULT_NOT_CLIPPED;
   *clipping_rect = GetClippingRect(source_size, target_size_, &clip_result);
-  *target_size = GetCopySizeForThumbnail(scale_factor, target_size_);
+  *copy_size = GetCopySizeForThumbnail(scale_factor, target_size_);
   return clip_result;
 }
 
@@ -74,55 +74,14 @@ SkBitmap SimpleThumbnailCrop::GetClippedBitmap(const SkBitmap& bitmap,
   return clipped_bitmap;
 }
 
-// Returns the size used by RenderWidgetHost::CopyFromBackingStore.
-//
-// The size is calculated in such a way that the copied size in pixel becomes
-// equal to (f * kThumbnailWidth, f * kThumbnailHeight), where f is the scale
-// of ui::SCALE_FACTOR_200P. Since RenderWidgetHost::CopyFromBackingStore takes
-// the size in DIP, we need to adjust the size based on |view|'s device scale
-// factor in order to copy the pixels with the size above.
-//
-// The copied size was chosen for the following reasons.
-//
-// 1. When the scale factor of the primary monitor is ui::SCALE_FACTOR_200P, the
-// generated thumbnail size is (f * kThumbnailWidth, f * kThumbnailHeight).
-// In order to avoid degrading the image quality by magnification, the size
-// of the copied pixels should be equal to or larger than this thumbnail size.
-//
-// 2. RenderWidgetHost::CopyFromBackingStore can be costly especially when
-// it is necessary to read back the web contents image data from GPU. As the
-// cost is roughly propotional to the number of the copied pixels, the size of
-// the copied pixels should be as small as possible.

[motek., 2015/03/26 21:06:02]

You should leave a trace of connection to RenderWidgetHost somewhere here.
Otherwise it is easy to get misguided by that downsample routine and just look
for the wrong thing. 

[Mathieu, 2015/03/26 21:19:48]

Done.

-//
-// When the scale factor of the primary monitor is ui::SCALE_FACTOR_100P,
-// we still copy the pixels with the same size as ui::SCALE_FACTOR_200P (2.0f)
-// because the resampling method used in RenderWidgetHost::CopyFromBackingStore
-// is not good enough for the resampled image to be used directly for the
-// thumbnail (http://crbug.com/141235). We assume this is not an issue in case of
-// ui::SCALE_FACTOR_200P because the high resolution thumbnail on high density
-// display alleviates the aliasing.
-// TODO(mazda): Copy the pixels with the smaller size in the case of
-// ui::SCALE_FACTOR_100P once the resampling method has been improved.
 // static
 gfx::Size SimpleThumbnailCrop::GetCopySizeForThumbnail(
     ui::ScaleFactor scale_factor,
     const gfx::Size& thumbnail_size) {
-  gfx::Size copy_size(thumbnail_size);
-  switch (scale_factor) {
-    case ui::SCALE_FACTOR_100P:
-      copy_size = gfx::ToFlooredSize(gfx::ScaleSize(copy_size, 2.0f));
-      break;
-    case ui::SCALE_FACTOR_200P:
-      // Use the size as-is.
-      break;
-    default:
-      DLOG(WARNING) << "Unsupported scale factor. Use the same copy size as "
-                    << "ui::SCALE_FACTOR_100P";
-      copy_size = gfx::ToFlooredSize(gfx::ScaleSize(
-          copy_size, gfx::ImageSkia::GetMaxSupportedScale()));
-      break;
-  }
-  return copy_size;
+  // The copy size returned is the pixel equivalent of |thumbnail_size|, which
+  // is in DIPs.
+  float scale = GetScaleForScaleFactor(scale_factor);
+  return gfx::ToFlooredSize(gfx::ScaleSize(thumbnail_size, scale));
 }
 
 gfx::Rect SimpleThumbnailCrop::GetClippingRect(const gfx::Size& source_size,