Restrict salient image size before storing

chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc

 // Copyright 2014 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 "base/json/json_parser.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/bitmap_fetcher/bitmap_fetcher.h"
 #include "chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h"
 #include "chrome/browser/enhanced_bookmarks/enhanced_bookmark_model_factory.h"
 #include "chrome/grit/browser_resources.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/enhanced_bookmarks/enhanced_bookmark_model.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_frame_host.h"
 #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;
 using bookmarks::BookmarkNode;
 
 namespace enhanced_bookmarks {
 
 BookmarkImageServiceAndroid::BookmarkImageServiceAndroid(
     content::BrowserContext* browserContext)
     : BookmarkImageService(
           browserContext->GetPath(),
           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(
     const GURL& page_url,
     const GURL& image_url,
     const std::string& referrer,
     net::URLRequest::ReferrerPolicy referrer_policy,
     bool update_bookmark) {
   const BookmarkNode* bookmark =
       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;
   }
 
   BitmapFetcherHandler* bitmap_fetcher_handler =
       new BitmapFetcherHandler(this, image_url);
   bitmap_fetcher_handler->Start(
       browser_context_, referrer, referrer_policy,
       net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES,
       update_bookmark, page_url);
 }
@@ skipping 118 matching lines @@
         referrer_policy = net::URLRequest::NEVER_CLEAR_REFERRER;
         break;
       default:
         NOTREACHED();
     }
   }
   RetrieveSalientImage(page_url, GURL(image_url), referrer.url.spec(),
                        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,
     net::URLRequest::ReferrerPolicy referrer_policy,
     int load_flags,
     bool update_bookmark,
     const GURL& page_url) {
   update_bookmark_ = update_bookmark;
   page_url_ = page_url;
 
   bitmap_fetcher_.Start(browser_context->GetRequestContext(), referrer,
                         referrer_policy, load_flags);
 }
 
 void BookmarkImageServiceAndroid::BitmapFetcherHandler::OnFetchComplete(
     const GURL url,
     const SkBitmap* bitmap) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   gfx::Image image;
   if (bitmap) {
     gfx::ImageSkia imageSkia = gfx::ImageSkia::CreateFrom1xBitmap(*bitmap);
     imageSkia.MakeThreadSafe();
     image = gfx::Image(imageSkia);
   }
-  service_->ProcessNewImage(page_url_, update_bookmark_, image, url);
+  service_->ProcessNewImage(page_url_, update_bookmark_, url, image);
 
   delete this;
 }
 
 }  // namespace enhanced_bookmarks