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

components/favicon/core/large_icon_service.h

Index: components/favicon/core/large_icon_service.h
diff --git a/components/favicon/core/large_icon_service.h b/components/favicon/core/large_icon_service.h
index bb8c1381f01ceaff4f7ece32adfbef69066f6056..2ca53f559ecc658fb27fedac8d96f3e30d1cbc38 100644
--- a/components/favicon/core/large_icon_service.h
+++ b/components/favicon/core/large_icon_service.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2015 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -7,28 +7,43 @@
 
 #include <vector>
 
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "components/favicon_base/favicon_callback.h"
+#include "components/favicon_base/favicon_types.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 class GURL;
 
-namespace favicon_base {
-struct FaviconRawBitmapResult;
-}
-
 namespace favicon {
 
 class FaviconService;
+class LargeIconResult;

[pkotwicz, 2015/05/07 14:52:14]

The forward declaration is unnecessary because you are including
favicon_types.h?

[huangs, 2015/05/07 21:01:59]

Done.

 
 // The large icon service provides methods to access large icons. It relies on
 // the favicon service.
 class LargeIconService : public KeyedService {
  public:
+  class Session : public base::RefCountedThreadSafe<Session> {
+   public:
+    Session();
+    ~Session();

[pkotwicz, 2015/05/07 14:52:14]

Nit: Please add a new line

[jochen (gone - plz use gerrit), 2015/05/07 15:00:41]

ref counted dtor should be private and friend RefCountedThreadSafe (see the
comment in ref_counted.h for an example)

[huangs, 2015/05/07 21:01:59]

Done.

[huangs, 2015/05/07 21:01:59]

Done.

+    int desired_size_in_pixel;

[jochen (gone - plz use gerrit), 2015/05/07 15:00:41]

classes should have getters/setters

[huangs, 2015/05/07 21:01:59]

This would add bunch of boiler-plates, and I think it makes the code less
readable.  struct would make more sense, but that's blocked.  So I'm making
Session manage the callback chain => everything is internal, and params injected
by ctor.

+    favicon_base::LargeIconCallback callback;
+    base::CancelableTaskTracker* tracker;
+    favicon_base::FaviconRawBitmapResult bitmap_result;
+    scoped_ptr<favicon_base::LargeIconResult> result;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Session);
+  };
+
   explicit LargeIconService(FaviconService* favicon_service);
 
   ~LargeIconService() override;
 
+  // Main entry point, must run on the UI thread.
   // Requests the best large icon for the page at |page_url| given the requested
   // |desired_size_in_pixel|. If no good large icon can be found, returns the
   // fallback style to use, for which the background is set to the dominant
@@ -41,23 +56,33 @@ class LargeIconService : public KeyedService {
     const favicon_base::LargeIconCallback& callback,
     base::CancelableTaskTracker* tracker);
 
- private:
+  // Must run on a background thread.
   // 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.
-  bool ResizeLargeIconIfValid(
+  static bool ResizeLargeIconIfValid(
       int desired_size_in_pixel,
       const favicon_base::FaviconRawBitmapResult& bitmap_result,
       favicon_base::FaviconRawBitmapResult* resized_bitmap_result);
 
-  // Intermediate callback for GetLargeIconOrFallbackStyle(). Tries to resize
-  // |bitmap_result| and pass the output to |callback|. If that does not work,
-  // computes the icon fallback style and uses it to invoke |callback|.
-  void RunLargeIconCallback(
-      const favicon_base::LargeIconCallback& callback,
-      int desired_size_in_pixel,
+ private:
+  // Intermediate callback for GetLargeIconOrFallbackStyle(). Must run on the
+  // UI thread, and should not perform complex image operations.
+  static void OnIconLookupComplete(
+      scoped_refptr<Session> session,
       const favicon_base::FaviconRawBitmapResult& bitmap_result);
 
+  // Must run on a background thread

[pkotwicz, 2015/05/07 14:52:14]

Nit: missing period "thread."

[huangs, 2015/05/07 21:01:59]

Done.

+  // Tries to resize |bitmap_result| and pass the output to |callback|. If that
+  // does not work, computes the icon fallback style and uses it to invoke
+  // |callback|. The runs on a background thread since image resizing and

[pkotwicz, 2015/05/07 14:52:14]

How about: "The runs on a background thread since" -> "This must be run on a
background thread because"

[huangs, 2015/05/07 21:01:59]

Done.

+  // dominant color extraction can be expensive.
+  static void ProcessIconOnBackgroundThread(scoped_refptr<Session> session);
+
+  // Must run on the UI thread.
+  // Callback to be invoked when ProcessIconOnBackgroundThread() is ready.

[pkotwicz, 2015/05/07 14:52:14]

How about: "Invoked when ProcessIconOnBackgroundThread() is done."

[huangs, 2015/05/07 21:01:59]

Done.

+  static void OnIconProcessingComplete(scoped_refptr<Session> session);
+
   FaviconService* favicon_service_;
 
   // A pre-populated list of the types of icon files to consider when looking