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)
+    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(
     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_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,
     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);
+
+  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;
 }
 
 }  // namespace enhanced_bookmarks