Simplify thumbnail capture logic.

A lot of the metrics that were being used around capture for NTP aren't
actually relevant for thumbnailing for tab hover/switching page
previews. This CL eliminates the extra logic and background work around
thumbnail quality as well as simplifying several of the classes and
utilities involved.

This CL does not:
 - correct the logic for when thumbnails should be captured
   (it is still tuned for the NTP use case)

This will be done in a followup CL.

Bug: 928954
Change-Id: I05c8f5c6a2f276419c72302892c7fe7cb9fa334a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1529853
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642443}

chrome/browser/ui/BUILD.gn

@@ -1062,8 +1062,6 @@ jumbo_split_static_library("ui") {
       "thumbnails/thumbnail_tab_helper.h",
       "thumbnails/thumbnail_utils.cc",
       "thumbnails/thumbnail_utils.h",
-      "thumbnails/thumbnailing_context.cc",
-      "thumbnails/thumbnailing_context.h",
       "toolbar/app_menu_icon_controller.cc",
       "toolbar/app_menu_icon_controller.h",
       "toolbar/app_menu_model.cc",

chrome/browser/ui/thumbnails/thumbnail_tab_helper.cc

@@ -10,6 +10,7 @@
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/tabs/tab_style.h"
 #include "chrome/browser/ui/thumbnails/thumbnail_utils.h"
 #include "chrome/common/chrome_features.h"
 #include "content/public/browser/browser_task_traits.h"
@@ -22,56 +23,8 @@
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/scrollbar_size.h"
 
-using thumbnails::ThumbnailingContext;
-
-namespace {
-
-// The desired thumbnail size in DIP. Note that on 1x devices, we actually take
-// thumbnails of twice that size.
-const int kThumbnailWidth = 154;
-const int kThumbnailHeight = 96;
-
-void ComputeThumbnailScore(const SkBitmap& thumbnail,
-                           scoped_refptr<ThumbnailingContext> context) {
-  base::TimeTicks process_bitmap_start_time = base::TimeTicks::Now();
-
-  context->score.boring_score = color_utils::CalculateBoringScore(thumbnail);
-
-  context->score.good_clipping =
-      thumbnails::IsGoodClipping(context->clip_result);
-
-  base::TimeDelta process_bitmap_time =
-      base::TimeTicks::Now() - process_bitmap_start_time;
-  UMA_HISTOGRAM_TIMES("Thumbnails.ProcessBitmapTime", process_bitmap_time);
-}
-
-}  // namespace
-
-// Overview
-// --------
-// DEPRECATED, thumbnails have been removed from the New Tab Page. See
-// https://crbug.com/893362.
-//
-// This class provides a service for updating thumbnails to be used in the
-// "Most visited" section of the New Tab page. The process is started by
-// StartThumbnailCaptureIfNecessary(), which updates the thumbnail for the
-// current tab if needed. The heuristics to judge whether to update the
-// thumbnail are implemented in ThumbnailService::ShouldAcquirePageThumbnail().
-// There are two triggers that can start the process:
-// - When a renderer is about to be hidden (this usually occurs when the current
-//   tab is closed or another tab is clicked).
-// - Just before navigating away from the current page.
-
 ThumbnailTabHelper::ThumbnailTabHelper(content::WebContents* contents)
-    : content::WebContentsObserver(contents),
-      observer_(this),
-      did_navigation_finish_(false),
-      has_received_document_since_navigation_finished_(false),
-      has_painted_since_document_received_(false),
-      page_transition_(ui::PAGE_TRANSITION_LINK),
-      load_interrupted_(false),
-      waiting_for_capture_(false),
-      weak_factory_(this) {}
+    : content::WebContentsObserver(contents) {}
 
 ThumbnailTabHelper::~ThumbnailTabHelper() = default;
 
@@ -214,7 +167,7 @@ void ThumbnailTabHelper::StartThumbnailCaptureIfNecessary(
   }
 
   // Ignore thumbnail update requests if one is already in progress.
-  if (thumbnailing_context_) {
+  if (thumbnailing_in_progress_) {
     LogThumbnailingOutcome(trigger, Outcome::NOT_ATTEMPTED_IN_PROGRESS);
     return;
   }
@@ -246,34 +199,28 @@ void ThumbnailTabHelper::StartThumbnailCaptureIfNecessary(
     return;
   }
 
-  // TODO(miu): This is the wrong size. It's the size of the view on-screen, and
-  // not the rendering size of the view. This will be replaced with the view's
-  // actual rendering size in a later change. http://crbug.com/73362
-  gfx::Rect copy_rect = gfx::Rect(view->GetViewBounds().size());
+  // Note: this is the size in pixels on-screen, not the size in DIPs.
+  gfx::Size source_size = view->GetViewBounds().size();
   // Clip the pixels that will commonly hold a scrollbar, which looks bad in
   // thumbnails.
-  int scrollbar_size = gfx::scrollbar_size();
-  copy_rect.Inset(0, 0, scrollbar_size, scrollbar_size);
+  const float scale_factor = view->GetDeviceScaleFactor();
+  const int scrollbar_size = gfx::scrollbar_size() * scale_factor;
+  source_size.Enlarge(-scrollbar_size, -scrollbar_size);
 
-  if (copy_rect.IsEmpty()) {
+  if (source_size.IsEmpty()) {
     LogThumbnailingOutcome(trigger, Outcome::NOT_ATTEMPTED_EMPTY_RECT);
     return;
   }
 
-  bool at_top = view->IsScrollOffsetAtTop();
-  bool load_completed = !web_contents()->IsLoading() && !load_interrupted_;
-  thumbnailing_context_ = new ThumbnailingContext(url, at_top, load_completed);
+  thumbnailing_in_progress_ = true;
 
-  ui::ScaleFactor scale_factor =
-      ui::GetSupportedScaleFactor(view->GetDeviceScaleFactor());
-  thumbnailing_context_->clip_result = thumbnails::GetCanvasCopyInfo(
-      copy_rect.size(), scale_factor,
-      gfx::Size(kThumbnailWidth, kThumbnailHeight), &copy_rect,
-      &thumbnailing_context_->requested_copy_size);
+  const gfx::Size desired_size = TabStyle::GetPreviewImageSize();
+  thumbnails::CanvasCopyInfo copy_info =
+      thumbnails::GetCanvasCopyInfo(source_size, scale_factor, desired_size);
   copy_from_surface_start_time_ = base::TimeTicks::Now();
   waiting_for_capture_ = true;
   view->CopyFromSurface(
-      copy_rect, thumbnailing_context_->requested_copy_size,
+      copy_info.copy_rect, copy_info.target_size,
       base::BindOnce(&ThumbnailTabHelper::ProcessCapturedBitmap,
                      weak_factory_.GetWeakPtr(), trigger));
 }
@@ -294,39 +241,13 @@ void ThumbnailTabHelper::ProcessCapturedBitmap(TriggerReason trigger,
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     // From here on, nothing can fail, so log success.
     LogThumbnailingOutcome(trigger, Outcome::SUCCESS);
-    base::PostTaskWithTraitsAndReply(
-        FROM_HERE,
-        {base::TaskPriority::BEST_EFFORT,
-         base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
-        base::BindOnce(&ComputeThumbnailScore, bitmap, thumbnailing_context_),
-        base::BindOnce(&ThumbnailTabHelper::StoreThumbnail,
-                       weak_factory_.GetWeakPtr(), bitmap));
+    thumbnail_ = ThumbnailImage::FromSkBitmap(bitmap);
+    web_contents()->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB);
   } else {
     LogThumbnailingOutcome(
         trigger, was_canceled ? Outcome::CANCELED : Outcome::READBACK_FAILED);
-    // On failure because of shutdown we are not on the UI thread, so ensure
-    // that cleanup happens on that thread.
-    // TODO(treib): Figure out whether it actually happen that we get called
-    // back on something other than the UI thread.
-    base::PostTaskWithTraits(
-        FROM_HERE, {content::BrowserThread::UI},
-        base::BindOnce(&ThumbnailTabHelper::CleanUpFromThumbnailGeneration,
-                       weak_factory_.GetWeakPtr()));
   }
-}
-
-void ThumbnailTabHelper::StoreThumbnail(const SkBitmap& thumbnail) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  thumbnail_ = ThumbnailImage::FromSkBitmap(thumbnail);
-  web_contents()->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB);
-
-  CleanUpFromThumbnailGeneration();
-}
-
-void ThumbnailTabHelper::CleanUpFromThumbnailGeneration() {
-  // Make a note that thumbnail generation is complete.
-  thumbnailing_context_ = nullptr;
+  thumbnailing_in_progress_ = false;
 }
 
 void ThumbnailTabHelper::TabHidden() {

chrome/browser/ui/thumbnails/thumbnail_tab_helper.h

@@ -10,7 +10,6 @@
 #include "base/scoped_observer.h"
 #include "base/time/time.h"
 #include "chrome/browser/ui/thumbnails/thumbnail_image.h"
-#include "chrome/browser/ui/thumbnails/thumbnailing_context.h"
 #include "content/public/browser/render_widget_host_observer.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -88,38 +87,32 @@ class ThumbnailTabHelper
   // Creates a thumbnail from the web contents bitmap.
   void ProcessCapturedBitmap(TriggerReason trigger, const SkBitmap& bitmap);
 
-  // Passes the thumbnail to the thumbnail service.
-  void StoreThumbnail(const SkBitmap& thumbnail);
-
-  // Cleans up after thumbnail generation has ended.
-  void CleanUpFromThumbnailGeneration();
-
   // Called when the current tab gets hidden.
   void TabHidden();
 
   static void LogThumbnailingOutcome(TriggerReason trigger, Outcome outcome);
 
-  ScopedObserver<content::RenderWidgetHost, content::RenderWidgetHostObserver>
-      observer_;
-
-  bool did_navigation_finish_;
-  bool has_received_document_since_navigation_finished_;
-  bool has_painted_since_document_received_;
+  bool did_navigation_finish_ = false;
+  bool has_received_document_since_navigation_finished_ = false;
+  bool has_painted_since_document_received_ = false;
 
-  ui::PageTransition page_transition_;
-  bool load_interrupted_;
+  ui::PageTransition page_transition_ = ui::PAGE_TRANSITION_LINK;
+  bool load_interrupted_ = false;
 
-  scoped_refptr<thumbnails::ThumbnailingContext> thumbnailing_context_;
-  bool waiting_for_capture_;
+  bool thumbnailing_in_progress_ = false;
+  bool waiting_for_capture_ = false;
 
   base::TimeTicks copy_from_surface_start_time_;
 
   ThumbnailImage thumbnail_;
 
-  base::WeakPtrFactory<ThumbnailTabHelper> weak_factory_;
+  ScopedObserver<content::RenderWidgetHost, content::RenderWidgetHostObserver>
+      observer_{this};
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 
+  base::WeakPtrFactory<ThumbnailTabHelper> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(ThumbnailTabHelper);
 };
 

chrome/browser/ui/thumbnails/thumbnail_utils.cc

@@ -13,85 +13,62 @@
 namespace thumbnails {
 
 bool IsGoodClipping(ClipResult clip_result) {
-  return clip_result == CLIP_RESULT_WIDER_THAN_TALL ||
-         clip_result == CLIP_RESULT_TALLER_THAN_WIDE ||
-         clip_result == CLIP_RESULT_NOT_CLIPPED;
+  return clip_result == ClipResult::kSourceWiderThanTall ||
+         clip_result == ClipResult::kSourceTallerThanWide ||
+         clip_result == ClipResult::kSourceNotClipped;
 }
 
-ClipResult GetCanvasCopyInfo(const gfx::Size& source_size,
-                             ui::ScaleFactor scale_factor,
-                             const gfx::Size& target_size,
-                             gfx::Rect* clipping_rect,
-                             gfx::Size* copy_size) {
+CanvasCopyInfo GetCanvasCopyInfo(const gfx::Size& source_size,
+                                 float scale_factor,
+                                 const gfx::Size& target_size) {
   DCHECK(!source_size.IsEmpty());
   DCHECK(!target_size.IsEmpty());
-  ClipResult clip_result = thumbnails::CLIP_RESULT_NOT_CLIPPED;
-  *clipping_rect = GetClippingRect(source_size, target_size, &clip_result);
-  *copy_size = GetCopySizeForThumbnail(scale_factor, target_size);
-  return clip_result;
-}
+  DCHECK_GT(scale_factor, 0.0f);
 
-// RenderWidgetHostView::CopyFromSurface() can be costly especially when it is
-// necessary to read back the web contents image data from GPU. As the cost is
-// roughly proportional to the number of the copied pixels, the size of the
-// copied pixels should be as small as possible.
-gfx::Size GetCopySizeForThumbnail(ui::ScaleFactor scale_factor,
-                                  const gfx::Size& thumbnail_size) {
-  // The copy size returned is the pixel equivalent of |thumbnail_size|, which
-  // is in DIPs.
-  if (scale_factor == ui::SCALE_FACTOR_100P) {
-    // In the case of 1x devices, we get a thumbnail twice as big and reduce
-    // it at serve time to improve quality.
-    scale_factor = ui::SCALE_FACTOR_200P;
-  }
-  float scale = GetScaleForScaleFactor(scale_factor);
-  // Limit the scale factor to a maximum of 2x for privacy reasons; see
-  // crbug.com/670488.
-  scale = std::min(2.0f, scale);
-  return gfx::ScaleToFlooredSize(thumbnail_size, scale);
-}
+  CanvasCopyInfo copy_info;
 
-gfx::Rect GetClippingRect(const gfx::Size& source_size,
-                          const gfx::Size& desired_size,
-                          ClipResult* clip_result) {
-  DCHECK(clip_result);
-
-  float desired_aspect =
-      static_cast<float>(desired_size.width()) / desired_size.height();
+  const float desired_aspect =
+      float{target_size.width()} / float{target_size.height()};
 
   // Get the clipping rect so that we can preserve the aspect ratio while
   // filling the destination.
-  gfx::Rect clipping_rect;
-  if (source_size.width() < desired_size.width() ||
-      source_size.height() < desired_size.height()) {
+  if (source_size.width() < target_size.width() ||
+      source_size.height() < target_size.height()) {
     // Source image is smaller: we clip the part of source image within the
     // dest rect, and then stretch it to fill the dest rect. We don't respect
     // the aspect ratio in this case.
-    clipping_rect = gfx::Rect(desired_size);
-    *clip_result = thumbnails::CLIP_RESULT_SOURCE_IS_SMALLER;
+    copy_info.copy_rect = gfx::Rect(target_size);
+    copy_info.clip_result = ClipResult::kSourceSmallerThanTarget;
+
   } else {
-    float src_aspect =
-        static_cast<float>(source_size.width()) / source_size.height();
+    const float src_aspect =
+        float{source_size.width()} / float{source_size.height()};
+
     if (src_aspect > desired_aspect) {
       // Wider than tall, clip horizontally: we center the smaller
       // thumbnail in the wider screen.
-      int new_width = static_cast<int>(source_size.height() * desired_aspect);
-      int x_offset = (source_size.width() - new_width) / 2;
-      clipping_rect.SetRect(x_offset, 0, new_width, source_size.height());
-      *clip_result =
+      const int new_width = source_size.height() * desired_aspect;
+      const int x_offset = (source_size.width() - new_width) / 2;
+      copy_info.clip_result =
           (src_aspect >= history::ThumbnailScore::kTooWideAspectRatio)
-              ? thumbnails::CLIP_RESULT_MUCH_WIDER_THAN_TALL
-              : thumbnails::CLIP_RESULT_WIDER_THAN_TALL;
+              ? ClipResult::kSourceMuchWiderThanTall
+              : ClipResult::kSourceWiderThanTall;
+      copy_info.copy_rect.SetRect(x_offset, 0, new_width, source_size.height());
+
     } else if (src_aspect < desired_aspect) {
-      clipping_rect =
+      copy_info.clip_result = ClipResult::kSourceTallerThanWide;
+      copy_info.copy_rect =
           gfx::Rect(source_size.width(), source_size.width() / desired_aspect);
-      *clip_result = thumbnails::CLIP_RESULT_TALLER_THAN_WIDE;
+
     } else {
-      clipping_rect = gfx::Rect(source_size);
-      *clip_result = thumbnails::CLIP_RESULT_NOT_CLIPPED;
+      copy_info.clip_result = ClipResult::kSourceNotClipped;
+      copy_info.copy_rect = gfx::Rect(source_size);
     }
   }
-  return clipping_rect;
+
+  copy_info.target_size = gfx::ScaleToFlooredSize(target_size, scale_factor);
+
+  return copy_info;
 }
 
 }  // namespace thumbnails

chrome/browser/ui/thumbnails/thumbnail_utils.h

@@ -12,51 +12,42 @@
 
 namespace thumbnails {
 
-// The result of clipping. This can be used to determine if the
-// generated thumbnail is good or not.
-enum ClipResult {
-  // Clipping is not done yet.
-  CLIP_RESULT_UNPROCESSED,
-  // The source image is smaller.
-  CLIP_RESULT_SOURCE_IS_SMALLER,
-  // Wider than tall by twice or more, clip horizontally.
-  CLIP_RESULT_MUCH_WIDER_THAN_TALL,
-  // Wider than tall, clip horizontally.
-  CLIP_RESULT_WIDER_THAN_TALL,
-  // Taller than wide, clip vertically.
-  CLIP_RESULT_TALLER_THAN_WIDE,
-  // The source and destination aspect ratios are identical.
-  CLIP_RESULT_NOT_CLIPPED,
+// The result of clipping. This can be used to determine if the generated
+// thumbnail is good or not.
+enum class ClipResult {
+  kSourceNotClipped,         // Source and target aspect ratios are identical.
+  kSourceSmallerThanTarget,  // Source image is smaller than target.
+  kSourceMuchWiderThanTall,  // Wider than tall by 2x+, clip horizontally.
+  kSourceWiderThanTall,      // Wider than tall, clip horizontally.
+  kSourceTallerThanWide,     // Taller than wide, clip vertically.
+};
+
+// Describes how a thumbnail bitmap should be generated from a target surface.
+struct CanvasCopyInfo {
+  // How the source canvas is clipped to achieve the target size.
+  ClipResult clip_result = ClipResult::kSourceNotClipped;
+
+  // Cropping rectangle for the source canvas, in pixels (not DIPs).
+  gfx::Rect copy_rect;
+
+  // Size of the target bitmap in pixels.
+  gfx::Size target_size;
 };
 
 bool IsGoodClipping(ClipResult clip_result);
 
 // The implementation of the 'classic' thumbnail cropping algorithm. It is not
-// content-driven in any meaningful way (save for score calculation). Rather,
-// the choice of a cropping region is based on relation between source and
-// target sizes. The selected source region is then rescaled into the target
-// thumbnail image.
-
+// content-driven in any meaningful way. Rather, the choice of a cropping region
+// is based on relation between source and target sizes. The selected source
+// region is then rescaled into the target thumbnail image.
+//
 // Provides information necessary to crop-and-resize image data from a source
 // canvas of |source_size|. Auxiliary |scale_factor| helps compute the target
-// thumbnail size to be copied from the backing store, in pixels. Parameters
-// of the required copy operation are assigned to |clipping_rect| (cropping
-// rectangle for the source canvas) and |copy_size| (the size of the copied
-// bitmap in pixels). The return value indicates the type of clipping that
-// will be done.
-ClipResult GetCanvasCopyInfo(const gfx::Size& source_size,
-                             ui::ScaleFactor scale_factor,
-                             const gfx::Size& target_size,
-                             gfx::Rect* clipping_rect,
-                             gfx::Size* copy_size);
-
-// Returns the size copied from the backing store. |thumbnail_size| is in
-// DIP, returned size in pixels.
-gfx::Size GetCopySizeForThumbnail(ui::ScaleFactor scale_factor,
-                                  const gfx::Size& thumbnail_size);
-gfx::Rect GetClippingRect(const gfx::Size& source_size,
-                          const gfx::Size& desired_size,
-                          ClipResult* clip_result);
+// thumbnail size to be copied from the backing store, in pixels. The return
+// value contains the type of clip and the clip parameters.
+CanvasCopyInfo GetCanvasCopyInfo(const gfx::Size& source_size,
+                                 float scale_factor,
+                                 const gfx::Size& target_size);
 
 }  // namespace thumbnails
 

chrome/browser/ui/thumbnails/thumbnail_utils_unittest.cc

@@ -4,7 +4,6 @@
 
 #include "chrome/browser/ui/thumbnails/thumbnail_utils.h"
 
-#include "chrome/browser/ui/thumbnails/thumbnailing_context.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/test/mock_render_process_host.h"
@@ -20,115 +19,57 @@ using content::WebContents;
 
 typedef testing::Test SimpleThumbnailCropTest;
 
-TEST_F(SimpleThumbnailCropTest, GetCanvasCopyInfo) {
-  gfx::Size thumbnail_size(200, 120);
-  gfx::Size expected_2x_size = gfx::ScaleToFlooredSize(thumbnail_size, 2.0);
-  float desired_aspect =
-      static_cast<float>(thumbnail_size.width()) / thumbnail_size.height();
-  gfx::Rect clipping_rect_result;
-  gfx::Size target_size_result;
+namespace thumbnails {
 
-  thumbnails::ClipResult clip_result = thumbnails::GetCanvasCopyInfo(
-      gfx::Size(400, 210), ui::SCALE_FACTOR_200P, thumbnail_size,
-      &clipping_rect_result, &target_size_result);
-  gfx::Size clipping_size = clipping_rect_result.size();
-  float clip_aspect =
-      static_cast<float>(clipping_size.width()) / clipping_size.height();
-  EXPECT_EQ(thumbnails::CLIP_RESULT_WIDER_THAN_TALL, clip_result);
-  EXPECT_EQ(expected_2x_size, target_size_result);
-  EXPECT_NEAR(desired_aspect, clip_aspect, 0.01);
+// Test which generates thumbnails from various source scales.
+class ThumbnailUtilsTest : public testing::TestWithParam<float> {
+ public:
+  ThumbnailUtilsTest() = default;
 
-  clip_result = thumbnails::GetCanvasCopyInfo(
-      gfx::Size(600, 200), ui::SCALE_FACTOR_200P, thumbnail_size,
-      &clipping_rect_result, &target_size_result);
-  clipping_size = clipping_rect_result.size();
-  clip_aspect =
-      static_cast<float>(clipping_size.width()) / clipping_size.height();
-  EXPECT_EQ(thumbnails::CLIP_RESULT_MUCH_WIDER_THAN_TALL, clip_result);
-  EXPECT_EQ(expected_2x_size, target_size_result);
-  EXPECT_NEAR(desired_aspect, clip_aspect, 0.01);
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ThumbnailUtilsTest);
+};
 
-  clip_result = thumbnails::GetCanvasCopyInfo(
-      gfx::Size(300, 600), ui::SCALE_FACTOR_200P, thumbnail_size,
-      &clipping_rect_result, &target_size_result);
-  clipping_size = clipping_rect_result.size();
-  clip_aspect =
-      static_cast<float>(clipping_size.width()) / clipping_size.height();
-  EXPECT_EQ(thumbnails::CLIP_RESULT_TALLER_THAN_WIDE, clip_result);
-  EXPECT_EQ(expected_2x_size, target_size_result);
-  EXPECT_NEAR(desired_aspect, clip_aspect, 0.01);
-
-  clip_result = thumbnails::GetCanvasCopyInfo(
-      gfx::Size(200, 100), ui::SCALE_FACTOR_200P, thumbnail_size,
-      &clipping_rect_result, &target_size_result);
-  EXPECT_EQ(thumbnails::CLIP_RESULT_SOURCE_IS_SMALLER, clip_result);
-  EXPECT_EQ(expected_2x_size, target_size_result);
+static float GetAspect(const gfx::Size& size) {
+  return float{size.width()} / float{size.height()};
 }
 
-TEST_F(SimpleThumbnailCropTest, GetCanvasCopyInfoDifferentScales) {
-  gfx::Size thumbnail_size(200, 120);
-
-  gfx::Rect clipping_rect_result;
-  gfx::Size target_size_result;
-
-  gfx::Size expected_2x_size = gfx::ScaleToFlooredSize(thumbnail_size, 2.0);
-
-  // Test at 1x scale. Expect a 2x thumbnail (we do this for quality).
-  thumbnails::GetCanvasCopyInfo(gfx::Size(400, 210), ui::SCALE_FACTOR_100P,
-                                thumbnail_size, &clipping_rect_result,
-                                &target_size_result);
-  EXPECT_EQ(expected_2x_size, target_size_result);
-
-  // Test at 1.5x scale.
-  gfx::Size expected_15x_size = gfx::ScaleToFlooredSize(thumbnail_size, 1.5);
-  thumbnails::GetCanvasCopyInfo(gfx::Size(400, 210), ui::SCALE_FACTOR_150P,
-                                thumbnail_size, &clipping_rect_result,
-                                &target_size_result);
-  EXPECT_EQ(expected_15x_size, target_size_result);
-
-  // Test at 2x scale.
-  thumbnails::GetCanvasCopyInfo(gfx::Size(400, 210), ui::SCALE_FACTOR_200P,
-                                thumbnail_size, &clipping_rect_result,
-                                &target_size_result);
-  EXPECT_EQ(expected_2x_size, target_size_result);
-
-  // Test at 3x scale. Expect a 2x (!) thumbnail (see crbug.com/670488).
-  thumbnails::GetCanvasCopyInfo(gfx::Size(400, 210), ui::SCALE_FACTOR_300P,
-                                thumbnail_size, &clipping_rect_result,
-                                &target_size_result);
-  EXPECT_EQ(expected_2x_size, target_size_result);
+INSTANTIATE_TEST_SUITE_P(,
+                         ThumbnailUtilsTest,
+                         testing::ValuesIn({1.0f, 1.25f, 1.62f, 2.0f}));
+
+TEST_P(ThumbnailUtilsTest, GetCanvasCopyInfo) {
+  constexpr gfx::Size kThumbnailSize(200, 120);
+  const float scale_factor = GetParam();
+  const gfx::Size expected_size =
+      gfx::ScaleToFlooredSize(kThumbnailSize, scale_factor);
+  const float desired_aspect = GetAspect(kThumbnailSize);
+  const gfx::Size wider_than_tall_source(400, 210);
+  const gfx::Size much_wider_than_tall_source(600, 200);
+  const gfx::Size taller_than_wide_source(300, 600);
+  const gfx::Size small_source(200, 100);
+
+  CanvasCopyInfo result =
+      GetCanvasCopyInfo(wider_than_tall_source, scale_factor, kThumbnailSize);
+  EXPECT_EQ(ClipResult::kSourceWiderThanTall, result.clip_result);
+  EXPECT_EQ(expected_size, result.target_size);
+  EXPECT_NEAR(desired_aspect, GetAspect(result.copy_rect.size()), 0.01);
+
+  result = GetCanvasCopyInfo(much_wider_than_tall_source, scale_factor,
+                             kThumbnailSize);
+  EXPECT_EQ(ClipResult::kSourceMuchWiderThanTall, result.clip_result);
+  EXPECT_EQ(expected_size, result.target_size);
+  EXPECT_NEAR(desired_aspect, GetAspect(result.copy_rect.size()), 0.01);
+
+  result =
+      GetCanvasCopyInfo(taller_than_wide_source, scale_factor, kThumbnailSize);
+  EXPECT_EQ(ClipResult::kSourceTallerThanWide, result.clip_result);
+  EXPECT_EQ(expected_size, result.target_size);
+  EXPECT_NEAR(desired_aspect, GetAspect(result.copy_rect.size()), 0.01);
+
+  result = GetCanvasCopyInfo(small_source, scale_factor, kThumbnailSize);
+  EXPECT_EQ(ClipResult::kSourceSmallerThanTarget, result.clip_result);
+  EXPECT_EQ(expected_size, result.target_size);
 }
 
-TEST_F(SimpleThumbnailCropTest, GetClippingRect) {
-  const gfx::Size desired_size(300, 200);
-  thumbnails::ClipResult clip_result;
-  // Try out 'microsource'.
-  gfx::Rect clip_rect = thumbnails::GetClippingRect(gfx::Size(300, 199),
-                                                    desired_size, &clip_result);
-  EXPECT_EQ(thumbnails::CLIP_RESULT_SOURCE_IS_SMALLER, clip_result);
-  EXPECT_EQ(gfx::Point(0, 0).ToString(), clip_rect.origin().ToString());
-  EXPECT_EQ(desired_size.ToString(), clip_rect.size().ToString());
-
-  // Portrait source.
-  clip_rect = thumbnails::GetClippingRect(gfx::Size(500, 1200), desired_size,
-                                          &clip_result);
-  EXPECT_EQ(thumbnails::CLIP_RESULT_TALLER_THAN_WIDE, clip_result);
-  EXPECT_EQ(gfx::Point(0, 0).ToString(), clip_rect.origin().ToString());
-  EXPECT_EQ(500, clip_rect.width());
-  EXPECT_GE(1200, clip_rect.height());
-
-  clip_rect = thumbnails::GetClippingRect(gfx::Size(2000, 800), desired_size,
-                                          &clip_result);
-  EXPECT_TRUE(clip_result == thumbnails::CLIP_RESULT_WIDER_THAN_TALL ||
-              clip_result == thumbnails::CLIP_RESULT_MUCH_WIDER_THAN_TALL);
-  EXPECT_EQ(0, clip_rect.y());
-  EXPECT_LT(0, clip_rect.x());
-  EXPECT_GE(2000, clip_rect.width());
-  EXPECT_EQ(800, clip_rect.height());
-
-  clip_rect = thumbnails::GetClippingRect(gfx::Size(900, 600), desired_size,
-                                          &clip_result);
-  EXPECT_EQ(thumbnails::CLIP_RESULT_NOT_CLIPPED, clip_result);
-  EXPECT_EQ(gfx::Point(0, 0).ToString(), clip_rect.origin().ToString());
-  EXPECT_EQ(gfx::Size(900, 600).ToString(), clip_rect.size().ToString());
-}
+}  // namespace thumbnails

chrome/browser/ui/thumbnails/thumbnailing_context.cc

-// Copyright (c) 2013 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/thumbnails/thumbnailing_context.h"
-
-namespace thumbnails {
-
-ThumbnailingContext::ThumbnailingContext(const GURL& url,
-                                         bool at_top,
-                                         bool load_completed)
-    : url(url), clip_result(CLIP_RESULT_UNPROCESSED) {
-  score.at_top = at_top;
-  score.load_completed = load_completed;
-}
-
-ThumbnailingContext::~ThumbnailingContext() = default;
-
-}  // namespace thumbnails

chrome/browser/ui/thumbnails/thumbnailing_context.h

-// Copyright (c) 2013 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.
-
-#ifndef CHROME_BROWSER_UI_THUMBNAILS_THUMBNAILING_CONTEXT_H_
-#define CHROME_BROWSER_UI_THUMBNAILS_THUMBNAILING_CONTEXT_H_
-
-#include "base/memory/ref_counted.h"
-#include "chrome/browser/ui/thumbnails/thumbnail_utils.h"
-#include "components/history/core/common/thumbnail_score.h"
-#include "ui/gfx/geometry/size.h"
-#include "url/gurl.h"
-
-namespace thumbnails {
-
-// Holds the information needed for processing a thumbnail.
-struct ThumbnailingContext : base::RefCountedThreadSafe<ThumbnailingContext> {
-  ThumbnailingContext(const GURL& url, bool at_top, bool load_completed);
-
-  GURL url;
-  ClipResult clip_result;
-  gfx::Size requested_copy_size;
-  history::ThumbnailScore score;
-
- private:
-  ~ThumbnailingContext();
-
-  friend class base::RefCountedThreadSafe<ThumbnailingContext>;
-};
-
-}  // namespace thumbnails
-
-#endif  // CHROME_BROWSER_UI_THUMBNAILS_THUMBNAILING_CONTEXT_H_