[Extensions] Update developerPrivate API to use base-64 data urls for icons

chrome/browser/extensions/api/developer_private/extension_info_generator.h

Index: chrome/browser/extensions/api/developer_private/extension_info_generator.h
diff --git a/chrome/browser/extensions/api/developer_private/extension_info_generator.h b/chrome/browser/extensions/api/developer_private/extension_info_generator.h
index 29dc73eaf17b63a1b0fc86e0877afc3595d00c54..27482d5a6e655436fd9dee3fae9960f5b476fb7f 100644
--- a/chrome/browser/extensions/api/developer_private/extension_info_generator.h
+++ b/chrome/browser/extensions/api/developer_private/extension_info_generator.h
@@ -5,18 +5,25 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_DEVELOPER_PRIVATE_EXTENSION_INFO_GENERATOR_H_
 #define CHROME_BROWSER_EXTENSIONS_API_DEVELOPER_PRIVATE_EXTENSION_INFO_GENERATOR_H_
 
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/common/extensions/api/developer_private.h"
 
 namespace content {
 class BrowserContext;
 }
 
+namespace gfx {
+class Image;
+}
+
 namespace extensions {
 class ErrorConsole;
 class Extension;
 class ExtensionActionAPI;
 class ExtensionPrefs;
 class ExtensionSystem;
+class ImageLoader;
 class WarningService;
 
 // Generates the developerPrivate api's specification for ExtensionInfo.
@@ -25,24 +32,40 @@ class ExtensionInfoGenerator {
   using ExtensionInfoList =
       std::vector<linked_ptr<api::developer_private::ExtensionInfo>>;
 
+  using ExtensionInfosCallback = base::Callback<void(const ExtensionInfoList&)>;
+
   explicit ExtensionInfoGenerator(content::BrowserContext* context);
   ~ExtensionInfoGenerator();
 
-  // Returns the ExtensionInfo for a given |extension| and |state|.
-  scoped_ptr<api::developer_private::ExtensionInfo> CreateExtensionInfo(
-      const Extension& extension,
-      api::developer_private::ExtensionState state);
-  // Returns an ExtensionInfo for the given |extension_id|, if the extension
-  // can be found.
-  scoped_ptr<api::developer_private::ExtensionInfo> CreateExtensionInfo(
-      const std::string& id);
+  // Creates and asynchronously returns an ExtensionInfo for the given
+  // |extension_id|, if the extension can be found.
+  // If the extension cannot be found, an empty vector is passed to |callback|.
+  void CreateExtensionInfo(const std::string& id,
+                           const ExtensionInfosCallback& callback);
 
-  // Returns a collection of ExtensionInfos, optionally including disabled and
-  // terminated.
-  ExtensionInfoList CreateExtensionsInfo(bool include_disabled,
-                                         bool include_terminated);
+  // Creates and asynchronously returns a collection of ExtensionInfos,
+  // optionally including disabled and terminated.
+  void CreateExtensionsInfo(bool include_disabled,
+                            bool include_terminated,
+                            const ExtensionInfosCallback& callback);
 
  private:
+  // Creates an ExtensionInfo for the given |extension| and |state|, and
+  // asynchronously adds it to the |list|.
+  void CreateExtensionInfoHelper(const Extension& extension,
+                                 api::developer_private::ExtensionState state);
+
+  // Callback for the asynchronous image loading.
+  void OnImageLoaded(scoped_ptr<api::developer_private::ExtensionInfo> info,
+                     const gfx::Image& image);
+
+  // Returns the icon url for the default icon to use.
+  const std::string& GetDefaultIconUrl(bool is_app, bool is_disabled);
+
+  // Returns an icon url from the given image, optionally applying a greyscale.
+  std::string GetIconUrlFromImage(const gfx::Image& image,
+                                  bool should_greyscale);
+
   // Various systems, cached for convenience.
   content::BrowserContext* browser_context_;
   ExtensionSystem* extension_system_;
@@ -50,6 +73,24 @@ class ExtensionInfoGenerator {
   ExtensionActionAPI* extension_action_api_;
   WarningService* warning_service_;
   ErrorConsole* error_console_;
+  ImageLoader* image_loader_;
+
+  // The number of pending image loads.
+  size_t pending_image_loads_;
+
+  // Default icons, cached and lazily initialized.
+  std::string default_app_icon_url_;
+  std::string default_extension_icon_url_;
+  std::string default_disabled_app_icon_url_;
+  std::string default_disabled_extension_icon_url_;
+
+  // The list of extension infos that have been generated.
+  ExtensionInfoList list_;
+
+  // The callback to run once all infos have been created.
+  ExtensionInfosCallback callback_;

[not at google - send to devlin, 2015/04/22 21:38:37]

|callback_| and |list_| mean that there can be only one outgoing call to
CreateExtension[s]Info. The easiest way for callers to think about that is that
ExtensionInfoGenerator is therefore single-use; construct one each time a call
needs to be made.

However, that then kills your default icon caching. However, callers don't
actually cache them anyway so it's no worse.

Here are some options:
- Hide the ability to construct an ExtensionInfoGenerator, and instead make its
constructor private with CreateExtension[s]Info static and take an extra
BrowserContext variable. Typesafe way of making it single-use.
- Pass around |list_| and |callback_| through your callback chains.
- Cache the default icons globally rather than per ExtensionInfoGenerator (not
mutually exclusive to other options).

[Devlin, 2015/04/23 19:17:21]

Yeah, always meant this to be a one-time-use thing (passing around list_ and
callback_ gets pretty crazy since these are all asynchronous in multiple
places).  The disadvantage to doing a static method is that early-aborting if
our owner goes away isn't an option, and it makes for a slightly less pretty
interface, IMO.  Would a comment and a few DCHECKS not be sufficient (I've added
these, so you can see what you think)?  (We do have a bunch of single use things
already).  If you feel strongly, I can change it.

[not at google - send to devlin, 2015/04/23 22:25:21]

Doesn't look like it'd be that bad to me, you could bundle the 2 arguments into
a struct if it really bothers you.
Good point.
I agree; which is why passing around the arguments is my preferred way of doing
it.
Much prefer type safety over comments + DCHECK, but to put things into
perspective, we *are* just talking about the developerPrivate API here. If I've
made you feel guilty enough to change it then great; if not, let's save some
time and get this submitted.

[Devlin, 2015/04/27 17:06:30]

But it's not easy to pass them around, even in a struct - we need to *store* the
list and the callback somewhere, and can't just bind them in, because there are
multiple calls to create the extension info (but only one list/callback).  We
could, I suppose, store collections of lists/callbacks on this object, and curry
in pointers to them, and then delete them when they are done - but that seems
like a recipe for pain and segfaults.

Given that this is just the dev private API, I'm inclined to do the easy way,
and if we ever find that we can't stand it, we'll change it then. :)

+
+  base::WeakPtrFactory<ExtensionInfoGenerator> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionInfoGenerator);
 };