Restrict salient image size before storing

It is a waste of storage to save images that are larger than device
display size. On tablet we should be even more aggressive about resizing
those images, because showing them won't take up the entire screen.

BUG=454623

Review URL: https://codereview.chromium.org/916783003

Cr-Commit-Position: refs/heads/master@{#317628}

chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc

@@ -18,7 +18,10 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/referrer.h"
 #include "net/base/load_flags.h"
+#include "skia/ext/image_operations.h"
+#include "ui/base/device_form_factor.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/android/device_display_info.h"
 #include "ui/gfx/image/image_skia.h"
 
 using content::Referrer;
@@ -33,6 +36,30 @@ BookmarkImageServiceAndroid::BookmarkImageServiceAndroid(
           EnhancedBookmarkModelFactory::GetForBrowserContext(browserContext),
           make_scoped_refptr(content::BrowserThread::GetBlockingPool())),
       browser_context_(browserContext) {
+  // The images we're saving will be used locally. So it's wasteful to store
+  // images larger than the device resolution.
+  gfx::DeviceDisplayInfo display_info;
+  int max_length = std::min(display_info.GetPhysicalDisplayWidth(),
+                            display_info.GetPhysicalDisplayHeight());
+  // GetPhysicalDisplay*() returns 0 for pre-JB MR1. If so, fall back to the
+  // second best option we have.
+  if (max_length == 0) {
+    max_length = std::min(display_info.GetDisplayWidth(),
+                          display_info.GetDisplayHeight());
+  }
+
+  if (ui::GetDeviceFormFactor() == ui::DEVICE_FORM_FACTOR_TABLET) {
+    max_length = max_length / 2;
+  } else if (ui::GetDeviceFormFactor() == ui::DEVICE_FORM_FACTOR_PHONE) {
+    max_length = max_length * 3 / 4;
+  }
+
+  DCHECK(max_length > 0);
+  max_size_.set_height(max_length);
+  max_size_.set_width(max_length);
+}
+
+BookmarkImageServiceAndroid::~BookmarkImageServiceAndroid() {
 }
 
 void BookmarkImageServiceAndroid::RetrieveSalientImage(
@@ -45,7 +72,7 @@ void BookmarkImageServiceAndroid::RetrieveSalientImage(
       enhanced_bookmark_model_->bookmark_model()
           ->GetMostRecentlyAddedUserNodeForURL(page_url);
   if (!bookmark || !image_url.is_valid()) {
-    ProcessNewImage(page_url, update_bookmark, gfx::Image(), image_url);
+    ProcessNewImage(page_url, update_bookmark, image_url, gfx::Image());
     return;
   }
 
@@ -184,6 +211,28 @@ void BookmarkImageServiceAndroid::RetrieveSalientImageFromContextCallback(
                        referrer_policy, update_bookmark);
 }
 
+gfx::Image BookmarkImageServiceAndroid::ResizeImage(gfx::Image image) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  gfx::Image resized_image = image;
+  if (image.Width() > max_size_.width() &&
+      image.Height() > max_size_.height()) {
+    float resize_ratio = std::min(((float)max_size_.width()) / image.Width(),
+                                  ((float)max_size_.height()) / image.Height());
+    // +0.5f is for correct rounding. Without it, it's possible that dest_width
+    // is smaller than max_size_.Width() by one.
+    int dest_width = static_cast<int>(resize_ratio * image.Width() + 0.5f);
+    int dest_height = static_cast<int>(resize_ratio * image.Height() + 0.5f);
+
+    resized_image =
+        gfx::Image::CreateFrom1xBitmap(skia::ImageOperations::Resize(
+            image.AsBitmap(), skia::ImageOperations::RESIZE_BEST, dest_width,
+            dest_height));
+    resized_image.AsImageSkia().MakeThreadSafe();
+  }
+  return resized_image;
+}
+
 void BookmarkImageServiceAndroid::BitmapFetcherHandler::Start(
     content::BrowserContext* browser_context,
     const std::string& referrer,
@@ -209,7 +258,7 @@ void BookmarkImageServiceAndroid::BitmapFetcherHandler::OnFetchComplete(
     imageSkia.MakeThreadSafe();
     image = gfx::Image(imageSkia);
   }
-  service_->ProcessNewImage(page_url_, update_bookmark_, image, url);
+  service_->ProcessNewImage(page_url_, update_bookmark_, url, image);
 
   delete this;
 }

chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h

@@ -24,6 +24,8 @@ class BookmarkImageServiceAndroid : public BookmarkImageService {
  public:
   explicit BookmarkImageServiceAndroid(content::BrowserContext* browserContext);
 
+  ~BookmarkImageServiceAndroid() override;
+
   void RetrieveSalientImage(const GURL& page_url,
                             const GURL& image_url,
                             const std::string& referrer,
@@ -48,9 +50,14 @@ class BookmarkImageServiceAndroid : public BookmarkImageService {
                                                bool update_bookmark,
                                                const base::Value* result);
 
+  gfx::Image ResizeImage(gfx::Image image) override;
+
   content::BrowserContext* browser_context_;
   // The script injected in a page to extract image urls.
   base::string16 script_;
+  // Maximum size for retrieved salient images in pixels. This is used when
+  // resizing an image.
+  gfx::Size max_size_;
 
   class BitmapFetcherHandler : private chrome::BitmapFetcherDelegate {
    public:

components/enhanced_bookmarks/bookmark_image_service.cc

@@ -190,8 +190,8 @@ void BookmarkImageService::SalientImageForUrl(const GURL& page_url,
 
 void BookmarkImageService::ProcessNewImage(const GURL& page_url,
                                            bool update_bookmarks,
-                                           const gfx::Image& image,
-                                           const GURL& image_url) {
+                                           const GURL& image_url,
+                                           const gfx::Image& image) {
   DCHECK(CalledOnValidThread());
   PostTaskToStoreImage(image, image_url, page_url);
   if (update_bookmarks && image_url.is_valid()) {
@@ -212,12 +212,13 @@ bool BookmarkImageService::IsPageUrlInProgress(const GURL& page_url) {
   return in_progress_page_urls_.find(page_url) != in_progress_page_urls_.end();
 }
 
-ImageRecord BookmarkImageService::StoreImage(const gfx::Image& image,
-                                             const GURL& image_url,
-                                             const GURL& page_url) {
-  ImageRecord image_info(image, image_url, SK_ColorBLACK);
-  if (!image.IsEmpty()) {
-    image_info.dominant_color = DominantColorForImage(image);
+ImageRecord BookmarkImageService::ResizeAndStoreImage(const gfx::Image& image,
+                                                      const GURL& image_url,
+                                                      const GURL& page_url) {
+  gfx::Image resized_image = ResizeImage(image);
+  ImageRecord image_info(resized_image, image_url, SK_ColorBLACK);
+  if (!resized_image.IsEmpty()) {
+    image_info.dominant_color = DominantColorForImage(resized_image);
     // TODO(lpromero): this should be saved all the time, even when there is an
     // empty image. http://crbug.com/451450
     pool_->PostNamedSequencedWorkerTask(
@@ -234,8 +235,8 @@ void BookmarkImageService::PostTaskToStoreImage(const gfx::Image& image,
   DCHECK(CalledOnValidThread());
 
   base::Callback<ImageRecord(void)> task =
-      base::Bind(&BookmarkImageService::StoreImage, base::Unretained(this),
-                 image, image_url, page_url);
+      base::Bind(&BookmarkImageService::ResizeAndStoreImage,
+                 base::Unretained(this), image, image_url, page_url);
   base::Callback<void(const ImageRecord&)> reply =
       base::Bind(&BookmarkImageService::OnStoreImagePosted,
                  base::Unretained(this), page_url);

components/enhanced_bookmarks/bookmark_image_service.h

@@ -84,17 +84,20 @@ class BookmarkImageService : public KeyedService,
   // Returns true if the image for the page_url is currently being fetched.
   bool IsPageUrlInProgress(const GURL& page_url);
 
-  // Stores the image to local storage. If update_bookmarks is true, relates the
-  // corresponding bookmark to image_url.
+  // Stores the new image to local storage. If update_bookmarks is true, relates
+  // the corresponding bookmark to image_url.
   void ProcessNewImage(const GURL& page_url,
                        bool update_bookmarks,
-                       const gfx::Image& image,
-                       const GURL& image_url);
+                       const GURL& image_url,
+                       const gfx::Image& image);
+
+  // Resizes large images to proper size that fits device display. This method
+  // should _not_ run on the UI thread.
+  virtual gfx::Image ResizeImage(gfx::Image image) = 0;
 
   // Sets a new image for a bookmark. If the given page_url is bookmarked and
   // the image is retrieved from the image_url, then the image is locally
   // stored. If update_bookmark is true the URL is also added to the bookmark.
-  // This is the only method subclass needs to implement.
   virtual void RetrieveSalientImage(
       const GURL& page_url,
       const GURL& image_url,
@@ -122,12 +125,13 @@ class BookmarkImageService : public KeyedService,
   // Processes the requests that have been waiting on an image.
   void ProcessRequests(const GURL& page_url, const ImageRecord& image);
 
-  // Once an image is retrieved this method updates the store with it. Returns
-  // the newly formed ImageRecord.  This is typically called on |pool_|, the
-  // background sequenced worker pool for this object.
-  ImageRecord StoreImage(const gfx::Image& image,
-                         const GURL& image_url,
-                         const GURL& page_url);
+  // Once an image is retrieved this method calls ResizeImage() and updates the
+  // store with the smaller image, then returns the newly formed ImageRecord.
+  // This is typically called on |pool_|, the background sequenced worker pool
+  // for this object.
+  ImageRecord ResizeAndStoreImage(const gfx::Image& image,
+                                  const GURL& image_url,
+                                  const GURL& page_url);
 
   // Calls |StoreImage| in the background.  This should only be called from the
   // main thread.