[LargeIconService] Allow decoding of images in the service

components/favicon/core/large_icon_service.cc

Index: components/favicon/core/large_icon_service.cc
diff --git a/components/favicon/core/large_icon_service.cc b/components/favicon/core/large_icon_service.cc
index 7d519677c51780055520e90d6e9a6f3a0217b1ff..8d7bde46990cbfed6ad0646641a65b794cb9b582 100644
--- a/components/favicon/core/large_icon_service.cc
+++ b/components/favicon/core/large_icon_service.cc
@@ -60,9 +60,11 @@ GURL GetIconUrlForGoogleServerV2(const GURL& page_url,
 // LargeIconService request.
 class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
  public:
+  // Exactly one of the callbacks is expected to be non-null.
   LargeIconWorker(int min_source_size_in_pixel,
                   int desired_size_in_pixel,
-                  favicon_base::LargeIconCallback callback,
+                  favicon_base::LargeIconCallback raw_bitmap_callback,
+                  favicon_base::LargeIconImageCallback image_callback,
                   scoped_refptr<base::TaskRunner> background_task_runner,
                   base::CancelableTaskTracker* tracker);
 
@@ -71,7 +73,7 @@ class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
   // ProcessIconOnBackgroundThread() so we do not perform complex image
   // operations on the UI thread.
   void OnIconLookupComplete(
-      const favicon_base::FaviconRawBitmapResult& bitmap_result);
+      const favicon_base::FaviconRawBitmapResult& raw_bitmap_result);
 
  private:
   friend class base::RefCountedThreadSafe<LargeIconWorker>;
@@ -83,15 +85,19 @@ class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
   // that does not work, computes the icon fallback style and uses it to
   // invoke |callback_|. This must be run on a background thread because image
   // resizing and dominant color extraction can be expensive.
-  void ProcessIconOnBackgroundThread();
+  void ProcessIconOnBackgroundThread(bool return_raw,
+                                     bool return_original_size);
 
   // Must run on a background thread in production.
   // If |bitmap_result_| is square and large enough (>= |min_source_in_pixel_|),
   // resizes it to |desired_size_in_pixel_| (but if |desired_size_in_pixel_| is
-  // 0 then don't resize). If successful, stores the resulting bitmap data
-  // into |resized_bitmap_result| and returns true.
-  bool ResizeLargeIconOnBackgroundThreadIfValid(
-      favicon_base::FaviconRawBitmapResult* resized_bitmap_result);
+  // 0 then don't resize). If successful, stores the resulting raw bitmap data
+  // into |raw_bitmap_result_| and image data in |image_result_| and returns
+  // true. The params |return_raw| and |return_original_size| allow early exit
+  // to skip steps that are not needed.
+  bool DecodeResizeAndEncodeLargeIconOnBackgroundThreadIfValid(
+      bool return_raw,
+      bool return_original_size);
 
   // Must run on the owner (UI) thread in production.
   // Invoked when ProcessIconOnBackgroundThread() is done.
@@ -99,11 +105,14 @@ class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
 
   int min_source_size_in_pixel_;
   int desired_size_in_pixel_;
-  favicon_base::LargeIconCallback callback_;
+  favicon_base::LargeIconCallback raw_bitmap_callback_;
+  favicon_base::LargeIconImageCallback image_callback_;
   scoped_refptr<base::TaskRunner> background_task_runner_;
   base::CancelableTaskTracker* tracker_;
-  favicon_base::FaviconRawBitmapResult bitmap_result_;
-  std::unique_ptr<favicon_base::LargeIconResult> result_;
+
+  favicon_base::FaviconRawBitmapResult raw_bitmap_result_;
+  SkBitmap image_result_;
+  std::unique_ptr<favicon_base::FallbackIconStyle> fallback_icon_style_;
 
   DISALLOW_COPY_AND_ASSIGN(LargeIconWorker);
 };
@@ -111,87 +120,118 @@ class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
 LargeIconWorker::LargeIconWorker(
     int min_source_size_in_pixel,
     int desired_size_in_pixel,
-    favicon_base::LargeIconCallback callback,
+    favicon_base::LargeIconCallback raw_bitmap_callback,
+    favicon_base::LargeIconImageCallback image_callback,
     scoped_refptr<base::TaskRunner> background_task_runner,
     base::CancelableTaskTracker* tracker)
     : min_source_size_in_pixel_(min_source_size_in_pixel),
       desired_size_in_pixel_(desired_size_in_pixel),
-      callback_(callback),
+      raw_bitmap_callback_(raw_bitmap_callback),
+      image_callback_(image_callback),
       background_task_runner_(background_task_runner),
-      tracker_(tracker) {
-}
+      tracker_(tracker) {}
 
 LargeIconWorker::~LargeIconWorker() {
 }
 
 void LargeIconWorker::OnIconLookupComplete(
-    const favicon_base::FaviconRawBitmapResult& bitmap_result) {
-  bitmap_result_ = bitmap_result;
+    const favicon_base::FaviconRawBitmapResult& raw_bitmap_result) {
+  raw_bitmap_result_ = raw_bitmap_result;
+
+  bool return_raw = !raw_bitmap_callback_.is_null();
+  bool return_original_size =
+      (desired_size_in_pixel_ == 0 ||
+       raw_bitmap_result_.pixel_size.width() == desired_size_in_pixel_);
+
   tracker_->PostTaskAndReply(
       background_task_runner_.get(), FROM_HERE,
-      base::Bind(&LargeIconWorker::ProcessIconOnBackgroundThread, this),
+      base::Bind(&LargeIconWorker::ProcessIconOnBackgroundThread, this,
+                 return_raw, return_original_size),
       base::Bind(&LargeIconWorker::OnIconProcessingComplete, this));
 }
 
-void LargeIconWorker::ProcessIconOnBackgroundThread() {
-  favicon_base::FaviconRawBitmapResult resized_bitmap_result;
-  if (ResizeLargeIconOnBackgroundThreadIfValid(&resized_bitmap_result)) {
-    result_.reset(
-        new favicon_base::LargeIconResult(resized_bitmap_result));
-  } else {
+void LargeIconWorker::ProcessIconOnBackgroundThread(bool return_raw,
+                                                    bool return_original_size) {
+  if (!DecodeResizeAndEncodeLargeIconOnBackgroundThreadIfValid(
+          return_raw, return_original_size)) {
     // Failed to resize |bitmap_result_|, so compute fallback icon style.
-    std::unique_ptr<favicon_base::FallbackIconStyle> fallback_icon_style(
-        new favicon_base::FallbackIconStyle());
-    if (bitmap_result_.is_valid()) {
-      favicon_base::SetDominantColorAsBackground(
-          bitmap_result_.bitmap_data, fallback_icon_style.get());
+    fallback_icon_style_ =
+        base::WrapUnique(new favicon_base::FallbackIconStyle());
+    if (raw_bitmap_result_.is_valid()) {
+      favicon_base::SetDominantColorAsBackground(raw_bitmap_result_.bitmap_data,
+                                                 fallback_icon_style_.get());
     }
-    result_.reset(
-        new favicon_base::LargeIconResult(fallback_icon_style.release()));
   }
 }
 
-bool LargeIconWorker::ResizeLargeIconOnBackgroundThreadIfValid(
-    favicon_base::FaviconRawBitmapResult* resized_bitmap_result) {
+bool LargeIconWorker::DecodeResizeAndEncodeLargeIconOnBackgroundThreadIfValid(
+    bool return_raw,
+    bool return_original_size) {

[pkotwicz, 2017/04/02 20:46:52]

How about something like this:
Some things about my approach that I like:
- The function parameters have all of the data that the functions need to
operate on. This means that the functions can be in the anonymous namespace. I
think that this makes the code less confusing.
- What ResizeLargeIconOnBackgroundThread() does is clearer now. The function now
does just one thing
- I removed some of the |in_pixel| suffixes because I *think* that it is obvious
that the functions are dealing in pixel

void ProcessIconOnBackgroundThread(
    const favicon_base::FaviconRawBitmapResult& db_result,
    int min_source_size,
    int desired_size,
    favicon_base::FaviconRawBitmapResult* raw_result,
    SkBitmap* bitmap,
    favicon_base::FallbackIconStyle* fallback_style) {
  gfx::Image image;
  if (IsDbResultAdequate(db_result, min_source_size)) {
    image = Resize(db_result, desired_size);
  }

  if (!image.IsEmpty()) {
    if (raw_result) {
      *raw_result = db_result;
      raw_result->pixel_size = gfx::Size(desired_size, desired_size);
      raw_result->bitmap_data = image.As1xPNGBytes();
    }
    if (bitmap) {
      *bitmap = image.AsBitmap();
    }
    return;
  }

  if (!fallback_icon_style)
    return;

  *fallback_icon_style = favicon_base::FallbackIconStyle();
  if (db_result.is_valid()) {
      favicon_base::SetDominantColorAsBackground(
          db_result.bitmap_data, fallback_icon_style);
  }
}

bool IsDbResultAdequate(
    const favicon_base::FaviconRawBitmapResult& db_result,
    int min_source_size) {
  return db_result.is_valid() &&
      db_result.pixel_size.width() == db_result.pixel_size.height()) &&
      db_result.pixel_size.width() >= min_source_size) {
}

gfx::Image ResizeLargeIconOnBackgroundThread(
    const favicon_base::FaviconRawBitmapResult& db_result,
    int desired_size) {
  gfx::Image image =
gfx::Image::CreateFrom1xPNGBytes(db_result.bitmap_data->front(),
                                 db_result.bitmap_data->size());

  if (desired_size == 0 || db_result.pixel_size.width() == desired_size) {
     return image;
  }

  SkBitmap resized = skia::ImageOperations::Resize(
      image.AsBitmap(), skia::ImageOperations::RESIZE_LANCZOS3,
      desired_size, desired_size);
  return gfx::Image::CreateFrom1xBitmap(resized);
}

[jkrcal, 2017/04/03 17:44:55]

Done.

   // Require bitmap to be valid and square.
-  if (!bitmap_result_.is_valid() ||
-      bitmap_result_.pixel_size.width() != bitmap_result_.pixel_size.height())
+  if (!raw_bitmap_result_.is_valid() ||
+      raw_bitmap_result_.pixel_size.width() !=
+          raw_bitmap_result_.pixel_size.height()) {
     return false;
+  }
 
   // Require bitmap to be large enough. It's square, so just check width.
-  if (bitmap_result_.pixel_size.width() < min_source_size_in_pixel_)
+  if (raw_bitmap_result_.pixel_size.width() < min_source_size_in_pixel_)
     return false;
 
-  *resized_bitmap_result = bitmap_result_;
-
-  // Special case: Can use |bitmap_result_| as is.
-  if (desired_size_in_pixel_ == 0 ||
-      bitmap_result_.pixel_size.width() == desired_size_in_pixel_)
+  // Decoding is not needed, we are done.
+  if (return_original_size && return_raw)
     return true;
 
-  // Resize bitmap: decode PNG, resize, and re-encode PNG.
-  SkBitmap decoded_bitmap;
-  if (!gfx::PNGCodec::Decode(bitmap_result_.bitmap_data->front(),
-          bitmap_result_.bitmap_data->size(), &decoded_bitmap))
+  // Decode the image.
+  if (!gfx::PNGCodec::Decode(raw_bitmap_result_.bitmap_data->front(),
+                             raw_bitmap_result_.bitmap_data->size(),
+                             &image_result_))
     return false;
 
-  SkBitmap resized_bitmap = skia::ImageOperations::Resize(
-      decoded_bitmap, skia::ImageOperations::RESIZE_LANCZOS3,
+  // Resizing is not needed, we are done.
+  if (return_original_size)
+    return true;
+
+  image_result_ = skia::ImageOperations::Resize(
+      image_result_, skia::ImageOperations::RESIZE_LANCZOS3,
       desired_size_in_pixel_, desired_size_in_pixel_);
 
+  // Encoding back is not needed, we are done.
+  if (!return_raw)
+    return true;
+
   std::vector<unsigned char> bitmap_data;
-  if (!gfx::PNGCodec::EncodeBGRASkBitmap(resized_bitmap, false, &bitmap_data))
+  if (!gfx::PNGCodec::EncodeBGRASkBitmap(image_result_, false, &bitmap_data))
     return false;
 
-  resized_bitmap_result->pixel_size =
+  // Write the results back into the original |raw_bitmap_result_|.
+  raw_bitmap_result_.pixel_size =
       gfx::Size(desired_size_in_pixel_, desired_size_in_pixel_);
-  resized_bitmap_result->bitmap_data =
+  raw_bitmap_result_.bitmap_data =
       base::RefCountedBytes::TakeVector(&bitmap_data);
   return true;
 }
 
 void LargeIconWorker::OnIconProcessingComplete() {
-  callback_.Run(*result_);
+  // If fallback style is set, return that.
+  if (fallback_icon_style_) {
+    if (raw_bitmap_callback_) {
+      raw_bitmap_callback_.Run(
+          favicon_base::LargeIconResult(fallback_icon_style_.release()));
+    } else {
+      image_callback_.Run(
+          favicon_base::LargeIconImageResult(fallback_icon_style_.release()));
+    }
+    return;
+  }
+
+  // Return the large icon, otherwise.
+  if (raw_bitmap_callback_) {
+    raw_bitmap_callback_.Run(favicon_base::LargeIconResult(raw_bitmap_result_));
+  } else {
+    image_callback_.Run(favicon_base::LargeIconImageResult(
+        gfx::Image::CreateFrom1xBitmap(image_result_)));
+  }
 }
 
 void OnFetchIconFromGoogleServerComplete(
@@ -242,18 +282,42 @@ LargeIconService::~LargeIconService() {
 }
 
 base::CancelableTaskTracker::TaskId
-    LargeIconService::GetLargeIconOrFallbackStyle(
-        const GURL& page_url,
-        int min_source_size_in_pixel,
-        int desired_size_in_pixel,
-        const favicon_base::LargeIconCallback& callback,
-        base::CancelableTaskTracker* tracker) {
+LargeIconService::GetLargeIconOrFallbackStyle(
+    const GURL& page_url,
+    int min_source_size_in_pixel,
+    int desired_size_in_pixel,
+    const favicon_base::LargeIconCallback& raw_bitmap_callback,
+    base::CancelableTaskTracker* tracker) {
+  DCHECK_LE(1, min_source_size_in_pixel);
+  DCHECK_LE(0, desired_size_in_pixel);
+
+  scoped_refptr<LargeIconWorker> worker = new LargeIconWorker(
+      min_source_size_in_pixel, desired_size_in_pixel, raw_bitmap_callback,
+      favicon_base::LargeIconImageCallback(), background_task_runner_, tracker);
+
+  // TODO(beaudoin): For now this is just a wrapper around
+  //   GetLargestRawFaviconForPageURL. Add the logic required to select the best
+  //   possible large icon. Also add logic to fetch-on-demand when the URL of
+  //   a large icon is known but its bitmap is not available.
+  return favicon_service_->GetLargestRawFaviconForPageURL(
+      page_url, large_icon_types_, min_source_size_in_pixel,
+      base::Bind(&LargeIconWorker::OnIconLookupComplete, worker), tracker);
+}
+
+base::CancelableTaskTracker::TaskId
+LargeIconService::GetLargeIconImageOrFallbackStyle(
+    const GURL& page_url,
+    int min_source_size_in_pixel,
+    int desired_size_in_pixel,
+    const favicon_base::LargeIconImageCallback& image_callback,
+    base::CancelableTaskTracker* tracker) {
   DCHECK_LE(1, min_source_size_in_pixel);
   DCHECK_LE(0, desired_size_in_pixel);
 
   scoped_refptr<LargeIconWorker> worker =
       new LargeIconWorker(min_source_size_in_pixel, desired_size_in_pixel,
-                          callback, background_task_runner_, tracker);
+                          favicon_base::LargeIconCallback(), image_callback,
+                          background_task_runner_, tracker);
 
   // TODO(beaudoin): For now this is just a wrapper around
   //   GetLargestRawFaviconForPageURL. Add the logic required to select the best