Refactored loading of applications / extensions icons.

chrome/browser/extensions/image_loader.h

Index: chrome/browser/extensions/image_loader.h
diff --git a/chrome/browser/extensions/image_loader.h b/chrome/browser/extensions/image_loader.h
index 5e6bc3ed3b5f4dd6ba24d4847f9be7ce2afdc8e4..015679cd3bedee1be24bed4741a788112629b5ae 100644
--- a/chrome/browser/extensions/image_loader.h
+++ b/chrome/browser/extensions/image_loader.h
@@ -10,11 +10,14 @@
 #include "base/callback_forward.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/common/cancelable_task_tracker.h"
+#include "chrome/common/favicon/favicon_types.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service.h"
 #include "extensions/common/extension_resource.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/layout.h"
 #include "ui/gfx/size.h"
+#include "url/gurl.h"

[Finnur, 2013/09/30 15:11:21]

nit: Gurl and chrome::FaviconBitmapResult can not be forward declared instead
(and the .h moved to the .cc file)?

[dvh-g, 2013/10/01 04:19:26]

Done.

 
 class Profile;
 
@@ -68,8 +71,7 @@ class ImageLoader : public BrowserContextKeyedService {
   // a convenience wrapper around ImageLoaderFactory::GetForProfile.
   static ImageLoader* Get(Profile* profile);
 
-  ImageLoader();
-  virtual ~ImageLoader();
+  explicit ImageLoader(Profile* profile);

[Finnur, 2013/09/30 15:11:21]

Shouldn't this be below the static function below (IsComponent...)?

[dvh-g, 2013/10/01 04:19:26]

Done.

 
   // Checks whether image is a component extension resource. Returns false
   // if a given |resource| does not have a corresponding image in bundled
@@ -96,10 +98,39 @@ class ImageLoader : public BrowserContextKeyedService {
   // type.
   void LoadImagesAsync(const extensions::Extension* extension,
                        const std::vector<ImageRepresentation>& info_list,
-                       const base::Callback<void(const gfx::Image&)>& callback);
+                       const base::Callback<void(const gfx::Image&)>&
+                       callback);
+
+  // Load the icon of the given extension.
+  void LoadExtensionIconAsync(const extensions::Extension* extension,
+                              int icon_size,
+                              bool grayscale,
+                              const base::Callback<void(const gfx::Image&)>&
+                              callback);

[Finnur, 2013/09/30 15:11:21]

nit: indent callback some more? It looks like the fifth parameter (same below).

[dvh-g, 2013/10/01 04:19:26]

Done.

+
+  // Load the data URL of the icon of the given extension.

[Finnur, 2013/09/30 15:11:21]

The pre-existing API was pretty clear. You had two "load image asynchronously"
functions, one that loads many at a time and the other that loads only one.
Pretty self-explanatory.

Now we have four very similar functions that all do slightly different tasks and
it is not as clear which to pick just from looking at the header file. I'd like
to know why these four variations are provided and be able to determine (from
the comments) which one to pick.

Additional question: I take it we plan to keep them all around forever (that
this is not a transition period)?

[dvh-g, 2013/10/01 04:19:26]

- One loads the icon of an extension (it's a specific resource of an
app/extension) and will return a image.
- The other one loads the icon of an extension and will return a image data URL
from it.

Do you have better naming for them / suggestion?
This is not a transition period.

[Finnur, 2013/10/01 11:14:15]

I don't think these need better names, necessarily. What is needed is more
guidance for people who want to use this API since the comments for these two
new functions are extremely skimpy on the details. I don't know if there ever
will be doubt (for consumers of this class) which of these four functions is the
right one for them, but it would be great if the comments could help them
(people looking at this API) choose the right function.

+  void LoadExtensionIconDataURLAsync(const extensions::Extension* extension,
+                                     int icon_size,
+                                     bool grayscale,
+                                     const base::Callback<void(const GURL&)>&
+                                     callback);
+
+ public:
+   virtual ~ImageLoader();

[Finnur, 2013/09/30 15:11:21]

Umm... what's the point of moving this into a separate public section at the
bottom? Did you mean to make this private? If so, why not move it to the already
existing private section below?

[dvh-g, 2013/10/01 04:19:26]

I moved it at the previous location, below ImageLoader().

 
  private:
-  base::WeakPtrFactory<ImageLoader> weak_ptr_factory_;
+   base::WeakPtrFactory<ImageLoader> weak_ptr_factory_;

[Finnur, 2013/09/30 15:11:21]

nit: The indentation for this member and the next four looks wrong (it went from
2 to 3 spaces, when it should have only two)

[dvh-g, 2013/10/01 04:19:26]

Done.

+
+   Profile* profile_;
+
+   // Task tracker when getting favicon.
+   CancelableTaskTracker cancelable_task_tracker_;
+
+   // Cache for the default app icon.
+   scoped_ptr<SkBitmap> default_app_data_;
+
+   // Cache for the extension app icon.
+   scoped_ptr<SkBitmap> default_extension_data_;
 
   static void LoadImagesOnBlockingPool(
       const std::vector<ImageRepresentation>& info_list,
@@ -109,6 +140,123 @@ class ImageLoader : public BrowserContextKeyedService {
   void ReplyBack(
       const std::vector<LoadResult>* load_result,
       const base::Callback<void(const gfx::Image&)>& callback);
+
+  // The sequence for LoadExtensionIconAsync() is the following:
+  // 1) It loads the icon image using LoadImageAsync()

[Finnur, 2013/09/30 15:11:21]

nit: End comments with period.

[dvh-g, 2013/10/01 04:19:26]

Done.

+  // 2) When it finishes, LoadExtensionIconLoaded() will be called.
+  // 3) On success, it will call FinalizeImage(). If it failed, it will call
+  // LoadIconFailed(). See below for more on those methods.
+  void LoadExtensionIconLoaded(const extensions::Extension* extension,

[Finnur, 2013/09/30 15:11:21]

Weird name: LoadFooLoaded. LoadFooDone is better, IMHO.

[dvh-g, 2013/10/01 04:19:26]

Done.

+                               int icon_size,
+                               bool grayscale,
+                               const base::Callback<void(const gfx::Image&)>&
+                               callback,
+                               const gfx::Image& image);
+
+  // Called when the extension doesn't have an icon. We fall back to multiple
+  // sources, using the following order:
+  //  1) The icons as listed in the extension / app manifests.

[Finnur, 2013/09/30 15:11:21]

Extension is a superset of apps, when it comes to Extension pointers but I don't
think listing (in the comments) the fact that extensions can be apps helps to
clarify the comment. In fact, I suspect it does the opposite. I would just drop
the words (or app) in these comments.

[dvh-g, 2013/10/01 04:19:26]

Done.

+  //  2) If a 16px icon and the extension has a launch URL, see if Chrome
+  //     has a corresponding favicon.
+  //  3) If still no matches, load the default extension / application icon.
+  void LoadIconFailed(const extensions::Extension* extension,
+                      int icon_size,
+                      bool grayscale,
+                      const base::Callback<void(const gfx::Image&)>& callback);
+
+  // Loads the favicon image for the given app/extension associated. If the

[Finnur, 2013/09/30 15:11:21]

... for the given |extension|.

[dvh-g, 2013/10/01 04:19:26]

Done.

[Finnur, 2013/10/01 11:14:15]

'Associated' is redundant.

On 2013/10/01 04:19:26, dvh-g wrote:

[dvh, 2013/10/02 05:15:08]

Done.

+  // image does not exist, we fall back to the default image.
+  void LoadFaviconImage(const extensions::Extension* extension,
+                        int icon_size,
+                        bool grayscale,
+                        const base::Callback<void(const gfx::Image&)>&
+                        callback);
+
+  // FaviconService callback. It will call FinalizedImage() on success or try
+  // an other fallback.

[Finnur, 2013/09/30 15:11:21]

type: s/an other/another/

[dvh-g, 2013/10/01 04:19:26]

Done.

+  void OnFaviconDataAvailable(
+      const extensions::Extension* extension,
+      int icon_size,
+      bool grayscale,
+      const base::Callback<void(const gfx::Image&)>& callback,
+      const chrome::FaviconBitmapResult& bitmap_result);
+
+  // The sequence for LoadDefaultImage() will be the following:
+  // 1) LoadDefaultImage() will invoke LoadDefaultImageOnFileThread() on the
+  // file thread.
+  // 2) LoadDefaultImageOnFileThread() will perform the work, then invoke
+  // LoadDefaultImageDone() on the UI thread.
+  void LoadDefaultImage(const extensions::Extension* extension,
+                        int icon_size,
+                        bool grayscale,
+                        const base::Callback<void(const gfx::Image&)>&
+                        callback);

[Finnur, 2013/09/30 15:11:21]

nit: indent? Same above and below.

[dvh-g, 2013/10/01 04:19:26]

Done.

+
+  // Loads the default image on the file thread.
+  void LoadDefaultImageOnFileThread(
+      const extensions::Extension* extension,
+      int icon_size,
+      bool grayscale,
+      const base::Callback<void(const gfx::Image&)>& callback);
+
+  // When loading of default image is done, it will call FinalizeImage().
+  void LoadDefaultImageDone(const gfx::Image& image,
+                            bool grayscale,
+                            const base::Callback<void(const gfx::Image&)>&
+                            callback);
+
+  // Performs any remaining transformations (like desaturating the |image|),
+  // then returns the |image| to the |callback|.
+  //
+  // The sequence for FinalizeImage() will be the following:
+  // 1) FinalizeImage() will invoke FinalizeImageOnFileThread() on the file
+  // thread.
+  // 2) FinalizeImageOnFileThread() will perform the work, then invoke
+  // FinalizeImageDone() on the UI thread.
+  void FinalizeImage(const gfx::Image& image,
+                     bool grayscale,
+                     const base::Callback<void(const gfx::Image&)>& callback);
+
+  // Process on the "finalize" operation on the file thread.

[Finnur, 2013/09/30 15:11:21]

nit: 'on' is redundant.

[dvh-g, 2013/10/01 04:19:26]

Done.

+  void FinalizeImageOnFileThread(const gfx::Image& image,
+                                 bool grayscale,
+                                 const base::Callback<void(const gfx::Image&)>&
+                                 callback);
+
+  // Called when the "finalize" operation on the file thread is done.
+  void FinalizeImageDone(const gfx::Image& image,
+                         const base::Callback<void(const gfx::Image&)> &
+                         callback);
+
+  // The sequence for LoadExtensionIconDataURLAsync() will be the following:
+  // 1) Call LoadExtensionIconAsync() to fetch the icon of the app/extension.
+  // 2) When the icon is loaded, OnIconAvailable() will be called and will
+  // invoke ConvertIconToURLOnFileThread() on the file thread.
+  // 3) OnIconConvertedToURL() will be called on the UI thread when it's done
+  // and will call the callback.
+  //
+  // LoadExtensionIconDataURLAsync() will use LoadExtensionIconAsync() to get
+  // the icon of the app/extension. The following method will be called when
+  // the image has been fetched.
+  void OnIconAvailable(const base::Callback<void(const GURL&)>& callback,
+                       const gfx::Image& image);
+
+  // ConvertIconToURLOnFileThread() will convert the image to a PNG image data
+  // URL in the file thread.

[Finnur, 2013/09/30 15:11:21]

nit: s/in/on/

[dvh-g, 2013/10/01 04:19:26]

Done.

+  void ConvertIconToURLOnFileThread(const gfx::Image& image,
+                                    const base::Callback<void(const GURL&)>&
+                                    callback);
+
+  // This method will call the callback of LoadExtensionIconDataURLAsync() with
+  // the result.
+  void OnIconConvertedToURL(const GURL& url,
+                            const base::Callback<void(const GURL&)>& callback);
+
+  // Returns the bitmap for the default extension.
+  const SkBitmap* GetDefaultExtensionImage();
+
+  // Returns the bitmap for the default app image.
+  const SkBitmap* GetDefaultAppImage();
 };
 
 }  // namespace extensions