[Large Icon Service] Move icon resizing into worker thread.

components/favicon/core/large_icon_service.cc

 // Copyright 2015 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 "components/favicon/core/large_icon_service.h"
 
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/task_runner.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon_base/fallback_icon_style.h"
-#include "components/favicon_base/favicon_types.h"
+#include "content/public/browser/browser_thread.h"
 #include "skia/ext/image_operations.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/geometry/size.h"
 
+namespace {
+
+// Return a TaskRunner used to execute background task.
+scoped_refptr<base::TaskRunner> GetBackgroundTaskRunner() {
+  return content::BrowserThread::GetBlockingPool()
+      ->GetTaskRunnerWithShutdownBehavior(
+          base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+}
+
+}  // namespace
+
 namespace favicon {
 
 LargeIconService::LargeIconService(FaviconService* favicon_service)
     : favicon_service_(favicon_service) {
   large_icon_types_.push_back(favicon_base::IconType::FAVICON);
   large_icon_types_.push_back(favicon_base::IconType::TOUCH_ICON);
   large_icon_types_.push_back(favicon_base::IconType::TOUCH_PRECOMPOSED_ICON);
 }
 
 LargeIconService::~LargeIconService() {
 }
 
+// Main entry point. Must run on the UI thread.

[beaudoin, 2015/05/06 14:06:53]

Move comment to .h?

[huangs, 2015/05/06 22:45:14]

Done.

 base::CancelableTaskTracker::TaskId
     LargeIconService::GetLargeIconOrFallbackStyle(
         const GURL& page_url,
         int desired_size_in_pixel,
         const favicon_base::LargeIconCallback& callback,
         base::CancelableTaskTracker* tracker) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  scoped_refptr<LargeIconService::Session> session =
+      new LargeIconService::Session();
+  session->desired_size_in_pixel = desired_size_in_pixel;
+  session->callback = callback;
+  session->tracker = 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_,
-      desired_size_in_pixel,
-      base::Bind(&LargeIconService::RunLargeIconCallback,
-                 base::Unretained(this), callback, desired_size_in_pixel),
+      page_url, large_icon_types_, desired_size_in_pixel,
+      base::Bind(&LargeIconService::OnIconLookupComplete,
+                 base::Unretained(this), session),
       tracker);
 }
 
+// static
 bool LargeIconService::ResizeLargeIconIfValid(
     int desired_size_in_pixel,
     const favicon_base::FaviconRawBitmapResult& bitmap_result,
     favicon_base::FaviconRawBitmapResult* resized_bitmap_result) {
   // Require bitmap to be valid and square.
   if (!bitmap_result.is_valid() ||
       bitmap_result.pixel_size.width() != 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() < desired_size_in_pixel) {
     // TODO(beaudoin): Potentially relax this, and allow icon to be scaled up.
     return false;
   }
 
   *resized_bitmap_result = bitmap_result;
-
   // Special case: Can use |resized_bitmap_result| as is.
   if (bitmap_result.pixel_size.width() == desired_size_in_pixel)
     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))
     return false;
 
   SkBitmap resized_bitmap = skia::ImageOperations::Resize(
       decoded_bitmap, skia::ImageOperations::RESIZE_LANCZOS3,
       desired_size_in_pixel, desired_size_in_pixel);
 
   std::vector<unsigned char> bitmap_data;
   if (!gfx::PNGCodec::EncodeBGRASkBitmap(resized_bitmap, false, &bitmap_data))
     return false;
 
   resized_bitmap_result->pixel_size =
       gfx::Size(desired_size_in_pixel, desired_size_in_pixel);
   resized_bitmap_result->bitmap_data =
       base::RefCountedBytes::TakeVector(&bitmap_data);
   return true;
 }
 
-void LargeIconService::RunLargeIconCallback(
-    const favicon_base::LargeIconCallback& callback,
-    int desired_size_in_pixel,
+// Must run on the UI thread.

[beaudoin, 2015/05/06 14:06:53]

I get the impression these comments are typically in the .h. Comments on
implementation of methods just look strange to me.

[huangs, 2015/05/06 22:45:14]

Done.

+void LargeIconService::OnIconLookupComplete(
+    scoped_refptr<LargeIconService::Session> session,
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
+  session->bitmap_result = bitmap_result;
+  session->tracker->PostTask(
+      GetBackgroundTaskRunner().get(), FROM_HERE,
+      base::Bind(&LargeIconService::ProcessIconOnBackgroundThread,
+                 base::Unretained(this), session));
+}
+
+// Must run on a background thread.
+void LargeIconService::ProcessIconOnBackgroundThread(

[jochen (gone - plz use gerrit), 2015/05/06 12:58:48]

how can you ensure that "this" is still a valid pointer here? should this maybe
be a static method? or a method on Session which is properly refcounted?

[huangs, 2015/05/06 13:28:07]

So making
  OnIconLookupComplete()
  ProcessIconOnBackgroundThread()
  OnIconProcessingComplete()
all static, seeing that they don't use any instance variables? In this case it
would work. But for the general case (if instance variable are used) what would
be the preferred way? 

Also, how about potential use-after-free with caller's |callback|? Or is that
taken care of in Callback?  Thanks!

[huangs, 2015/05/06 22:45:14]

Going with static to keep things simple.

+    scoped_refptr<LargeIconService::Session> session) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   favicon_base::FaviconRawBitmapResult resized_bitmap_result;
-  if (ResizeLargeIconIfValid(desired_size_in_pixel, bitmap_result,
-                             &resized_bitmap_result)) {
-    callback.Run(favicon_base::LargeIconResult(resized_bitmap_result));
-    return;
+  if (ResizeLargeIconIfValid(session->desired_size_in_pixel,
+                             session->bitmap_result, &resized_bitmap_result)) {
+    session->result.reset(
+        new favicon_base::LargeIconResult(resized_bitmap_result));
+  } else {
+    // Failed to resize |bitmap_result|, so compute fallback icon style.
+    scoped_ptr<favicon_base::FallbackIconStyle> fallback_icon_style(
+        new favicon_base::FallbackIconStyle());
+    if (session->bitmap_result.is_valid()) {
+      favicon_base::SetDominantColorAsBackground(
+          session->bitmap_result.bitmap_data, fallback_icon_style.get());
+    }
+    session->result.reset(
+        new favicon_base::LargeIconResult(fallback_icon_style.release()));
   }
 
-  // Failed to resize |bitmap_result|, so compute fallback icon style.
-  favicon_base::LargeIconResult result(new favicon_base::FallbackIconStyle());
-  if (bitmap_result.is_valid()) {
-    favicon_base::SetDominantColorAsBackground(
-        bitmap_result.bitmap_data, result.fallback_icon_style.get());
-  }
-  callback.Run(result);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&LargeIconService::OnIconProcessingComplete,
+                 base::Unretained(this), session));
+}
+
+// Must run on the UI thread.
+void LargeIconService::OnIconProcessingComplete(
+    scoped_refptr<LargeIconService::Session> session) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[jochen (gone - plz use gerrit), 2015/05/06 12:58:48]

same here

[huangs, 2015/05/06 22:45:14]

Done.

+  session->callback.Run(*session->result);
 }
 
 }  // namespace favicon