Restrict salient image size before storing

chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc

Index: chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
diff --git a/chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc b/chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
index 5671c783b2578bf443d94d567e3a09454003bfe0..1740cc594e6a87f21aa4611ed9fcc05a89c97492 100644
--- a/chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
+++ b/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/resource/resource_bundle.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;
@@ -27,12 +30,36 @@ using bookmarks::BookmarkNode;
 namespace enhanced_bookmarks {
 
 BookmarkImageServiceAndroid::BookmarkImageServiceAndroid(
-    content::BrowserContext* browserContext)
+    content::BrowserContext* browserContext,
+    scoped_refptr<base::SequencedWorkerPool> pool)
     : BookmarkImageService(
           browserContext->GetPath(),
           EnhancedBookmarkModelFactory::GetForBrowserContext(browserContext),
-          make_scoped_refptr(content::BrowserThread::GetBlockingPool())),
-      browser_context_(browserContext) {
+          pool),
+      browser_context_(browserContext),
+      pool_(pool) {
+  // 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::max(display_info.GetDisplayWidth(),
+                          display_info.GetDisplayHeight());
+  }
+  max_length /= display_info.GetDIPScale();

[Kibeom Kim (inactive), 2015/02/12 23:24:15]

IIRC, gfx::Image's DPI concept doesn't depend on the running device setting
automatically, and in our case I think it's set to be "no scale", at
gfx::ImageSkia::CreateFrom1xBitmap() and no DPI information is added. Can you
double check?

I compared bitmap->width() inside |
BookmarkImageServiceAndroid::BitmapFetcherHandler::OnFetchComplet| and
image.Width() inside |BookmarkImageServiceAndroid::ResizeImage| and it was the
same.

[Ian Wen, 2015/02/13 00:52:41]

Yes you were right. The size of the dialog is always that large, which tricked
me into believing it represents the size of the image correctly. Done.

+  // For tablet, having an image larger than 600dip does not make sense, because
+  // we are showing it inside of a dialog, which only takes about 3/4 the
+  // display width. This restriction will not affect phones (we defined phones
+  // to be some devices having one dimension less than 600dip).
+  max_length = std::min(max_length, 600);
+  DCHECK(max_length > 0);
+  max_size_.reset(new gfx::Size(max_length, 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,30 @@ void BookmarkImageServiceAndroid::RetrieveSalientImageFromContextCallback(
                        referrer_policy, update_bookmark);
 }
 
+gfx::Image BookmarkImageServiceAndroid::ResizeImage(gfx::Image image) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  gfx::Image resized_uimage = image;
+  if (max_size_->width() > 0 && max_size_->height() > 0 &&
+      (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_uimage =
+        gfx::Image::CreateFrom1xBitmap(skia::ImageOperations::Resize(
+            image.AsBitmap(), skia::ImageOperations::RESIZE_BEST, dest_width,
+            dest_height));
+    resized_uimage.AsImageSkia().MakeThreadSafe();
+  }
+  return resized_uimage;
+}
+
 void BookmarkImageServiceAndroid::BitmapFetcherHandler::Start(
     content::BrowserContext* browser_context,
     const std::string& referrer,
@@ -209,7 +260,15 @@ void BookmarkImageServiceAndroid::BitmapFetcherHandler::OnFetchComplete(
     imageSkia.MakeThreadSafe();
     image = gfx::Image(imageSkia);
   }
-  service_->ProcessNewImage(page_url_, update_bookmark_, image, url);
+
+  base::Callback<gfx::Image(void)> task =

[noyau (Ping after 24h), 2015/02/12 08:34:42]

Could you move this code in the superclass? With a virtual implementation for
resizeImage?

This would make it easier for us to implement the same thing in iOS.

[Ian Wen, 2015/02/12 22:49:47]

Done.

+      base::Bind(&BookmarkImageServiceAndroid::ResizeImage,
+                 base::Unretained(service_), image);
+  base::Callback<void(const gfx::Image&)> reply =
+      base::Bind(&BookmarkImageServiceAndroid::ProcessNewImage,
+                 base::Unretained(service_), page_url_, update_bookmark_, url);
+  base::PostTaskAndReplyWithResult(service_->pool_.get(), FROM_HERE, task,
+                                   reply);
 
   delete this;
 }