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

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 b411d4abe64f4d2d4f01cee0415e322fa1642ff9..7de10a00f4bc4c0698232bdbb11a3327d0e7dc5a 100644
--- a/components/favicon/core/large_icon_service.cc
+++ b/components/favicon/core/large_icon_service.cc
@@ -4,13 +4,28 @@
 
 #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)
@@ -23,25 +38,32 @@ LargeIconService::LargeIconService(FaviconService* favicon_service)
 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,
@@ -58,7 +80,6 @@ bool LargeIconService::ResizeLargeIconIfValid(
   }
 
   *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;
@@ -84,24 +105,49 @@ bool LargeIconService::ResizeLargeIconIfValid(
   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