[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..aea2391adebdc1c283a51cfe08962866e4f2445e 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,12 +38,20 @@ LargeIconService::LargeIconService(FaviconService* favicon_service)
 LargeIconService::~LargeIconService() {
 }
 
+// Main entry point, runs on the UI thread.

[beaudoin, 2015/05/05 19:44:40]

NIT: must run on the UI thread. That comment should probably be in the header
file.

[huangs, 2015/05/05 20:07:23]

Done.

 base::CancelableTaskTracker::TaskId
     LargeIconService::GetLargeIconOrFallbackStyle(
         const GURL& page_url,
         int desired_size_in_pixel,
         const favicon_base::LargeIconCallback& callback,
         base::CancelableTaskTracker* tracker) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[beaudoin, 2015/05/05 19:44:40]

This is typically a DCHECK in Chrome, any reason to use a CHECK here?

[huangs, 2015/05/05 20:07:23]

Because I'm testing with release build.  Changed to DCHECK.

+  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
@@ -37,15 +60,21 @@ base::CancelableTaskTracker::TaskId
       page_url,
       large_icon_types_,
       desired_size_in_pixel,
-      base::Bind(&LargeIconService::RunLargeIconCallback,
-                 base::Unretained(this), callback, desired_size_in_pixel),
+      base::Bind(&LargeIconService::OnIconLookupComplete,
+                 base::Unretained(this),
+                 session),
       tracker);
 }
 
+// static
+// Resizes |bitmap_result| to |desired_size_in_pixel|x|desired_size_in_pixel|.
+// Stores the resized bitmap data in |resized_bitmap_result| and returns true
+// if successful. This runs on a background thread.

[beaudoin, 2015/05/05 19:44:40]

The method comment should be in the .h, except // static

[huangs, 2015/05/05 20:07:23]

I briefly made this local, forgot to remove when I undid the change.

 bool LargeIconService::ResizeLargeIconIfValid(
     int desired_size_in_pixel,
     const favicon_base::FaviconRawBitmapResult& bitmap_result,
     favicon_base::FaviconRawBitmapResult* resized_bitmap_result) {
+  CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[beaudoin, 2015/05/05 19:44:40]

DCHECK?

[huangs, 2015/05/05 20:07:23]

Done.

   // Require bitmap to be valid and square.
   if (!bitmap_result.is_valid() ||
       bitmap_result.pixel_size.width() != bitmap_result.pixel_size.height())
@@ -58,7 +87,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 +112,54 @@ bool LargeIconService::ResizeLargeIconIfValid(
   return true;
 }
 
-void LargeIconService::RunLargeIconCallback(
-    const favicon_base::LargeIconCallback& callback,
-    int desired_size_in_pixel,
+// Runs on the UI thread.
+void LargeIconService::OnIconLookupComplete(
+    scoped_refptr<LargeIconService::Session> session,
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  session->bitmap_result = bitmap_result;
+  session->tracker->PostTask(
+      GetBackgroundTaskRunner().get(),
+      FROM_HERE,
+      base::Bind(&LargeIconService::ProcessIconOnBackgroundThread,
+                 base::Unretained(this),
+                 session));
+}
+
+// Runs on a background thread.
+void LargeIconService::ProcessIconOnBackgroundThread(
+    scoped_refptr<LargeIconService::Session> session) {
+  CHECK(!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)) {

[beaudoin, 2015/05/05 19:44:40]

Indentation looks wrong.

[huangs, 2015/05/05 20:07:23]

Ran "git cl format", let's see how this changes things.

+    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));

[beaudoin, 2015/05/05 19:44:40]

Would it be possible to skip the intermediate function? Something like:

base::Bind(&base::Callback<void(const LargeIconResult&)>::Run,
           base::Unretained(session),
           *session->result)

[huangs, 2015/05/05 20:07:23]

Yes but I think it's cleaner this way.

+}
+
+// Runs on the UI thread.
+void LargeIconService::OnIconProcessingComplete(
+    scoped_refptr<LargeIconService::Session> session) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  session->callback.Run(*session->result);
 }
 
 }  // namespace favicon