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..eceb19c0f2154ef48807ac9800f862af0abc4fbb 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;
@@ -33,6 +36,27 @@ 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::max(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());
+  }
+  // For tablet, having an image larger than 500dp 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, int(500 * display_info.GetDIPScale()));

[noyau (Ping after 24h), 2015/02/13 09:57:42]

The comment says 600, the code uses 500.

Is the same condition used to change the UI to 3/4 of the size on tablet? Should
this condition be extracted in some common code so the image is never reduced to
something too small the day they make a giant phone even more giant that the
ones we have now?

[Ian Wen, 2015/02/13 22:51:24]

500 was used intentionally, because I wanted to make resizing to be more
aggressive, given that large images might cause janky scrolling for grid view,
and most users won't go to the detail page anyway.

In the next patch, I changed the logic to ratio cutoff, instead of a fixed
value. And depending on tablet or on phone, it adopts different sizes. (Android
has a large number of different devices...)

+  DCHECK(max_length > 0);
+  max_size_.reset(new gfx::Size(max_length, max_length));
+}
+
+BookmarkImageServiceAndroid::~BookmarkImageServiceAndroid() {
 }
 
 void BookmarkImageServiceAndroid::RetrieveSalientImage(
@@ -45,7 +69,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 +208,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_image = image;
+  if (max_size_->width() > 0 && max_size_->height() > 0 &&

[noyau (Ping after 24h), 2015/02/13 09:57:42]

You've already done a DCHECK for this condition in the constructor, no need to
test it again.

[Ian Wen, 2015/02/13 22:51:24]

Done.

+      (image.Width() > max_size_->width() ||
+       image.Height() > max_size_->height())) {

[noyau (Ping after 24h), 2015/02/13 09:57:42]

If you resize an image that is really tall, or an image that is really thin, you
will end up with an unexploitable image after the resize. This condition should
probably be a &&, to keep images that already have a dimension that is too small
as is. 

[Ian Wen, 2015/02/13 22:51:24]

Done.

+    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 +257,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;
 }