Refactor extension app icon.

chrome/browser/extensions/extension_app_icon_service.h

Index: chrome/browser/extensions/extension_app_icon_service.h
diff --git a/chrome/browser/extensions/extension_app_icon_service.h b/chrome/browser/extensions/extension_app_icon_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..601a2027ce5e74424c85a27cabeaf95ae95a43db
--- /dev/null
+++ b/chrome/browser/extensions/extension_app_icon_service.h
@@ -0,0 +1,90 @@
+// Copyright 2017 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_APP_ICON_SERVICE_H_
+#define CHROME_BROWSER_EXTENSIONS_EXTENSION_APP_ICON_SERVICE_H_
+
+#include <map>
+#include <memory>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "extensions/browser/extension_registry_observer.h"
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+
+class ExtensionAppIcon;
+class ExtensionAppIconDelegate;
+
+// Factory for ExtensionAppIcon. Each created icon is tracked by this service.
+// Once some condition that affects how extension app icon should look is
+// changed then corresponded app icons are automatically updated. This service
+// is bound to content::BrowserContext.
+// Usage: ExtensionAppIconService::Get(context)->CreateIcon().
+class ExtensionAppIconService : public KeyedService,

[Devlin, 2017/04/20 20:17:41]

Do we even need this class?  Is there a reason the icons couldn't observe the
registry directly for extension loaded/unloaded calls?

[khmel, 2017/04/20 20:21:53]

This would requires tons on listeners and very inefficient form memory and
performance point of view. (Each icon has to observe callbacks for all extension
events (not only personal event). Also each icon has to create
LauncherExtensionAppUpdater (name and location of this class is bad but this is
out of scope current CL)). Instead one service is created that receives
callbacks and dispatches to corresponded icons.

[Devlin, 2017/04/26 22:41:57]

Ah, I missed that call to UpdateIcon (and only saw the ones from the OnUnload). 
I wouldn't worry much about the added observers, since, realistically, this
number is relatively small, and adding extra observers is quite cheap (probably
cheaper than another KeyedService).  But, moot given the extra update calls.

[khmel, 2017/04/27 00:18:12]

Acknowledged.

+                                public ExtensionRegistryObserver {
+ public:
+  static ExtensionAppIconService* Create(content::BrowserContext* context);

[Devlin, 2017/04/26 22:41:57]

Why do we need a Create() rather than just using the constructor?

[khmel, 2017/04/27 00:18:12]

This is common practice for services to have CTOR private and call ::Create from
*Factory. Also I see implementations with friend instead of ::Create. Please let
me know what do you prefer. I changed to public CTOR in this updated.

[Devlin, 2017/05/01 14:51:10]

We should only need a Create() method when there's some additional logic to the
creation, like when we might want to stub out the factory for a different one. 
Since that's not the case here, we should be fine.

[khmel, 2017/05/01 20:18:18]

Acknowledged.

+
+  // Convenience function to get the ExtensionAppIconService for a
+  // BrowserContext.
+  static ExtensionAppIconService* Get(content::BrowserContext* context);
+
+  ~ExtensionAppIconService() override;
+
+  // Creates extension app icon for requested app and size. Icon updates are
+  // dispatched via |delegate|.
+  std::unique_ptr<ExtensionAppIcon> CreateIcon(
+      ExtensionAppIconDelegate* delegate,
+      const std::string& app_id,
+      int resource_size_in_dip);
+
+  // KeyedService:
+  void Shutdown() override;
+
+ private:
+  friend class ExtensionAppIcon;
+
+  class Updater;
+  using IconMap = std::multimap<std::string, ExtensionAppIcon*>;

[Devlin, 2017/04/26 22:41:57]

Is there a reason to use a multimap instead of e.g. a std::map<std::string,
std::set>?  It seems like that would make some of the operations easier/clearer.

[khmel, 2017/04/27 00:18:12]

Changed as you suggested.

+
+  // See the Create methods.
+  explicit ExtensionAppIconService(content::BrowserContext* context);
+
+  // Called from ExtensionAppIcon DTOR.
+  void OnIconDestroyed(ExtensionAppIcon* icon);
+
+  // Called from Updater when corresponded app icons need to be updated.
+  void OnAppUpdated(const std::string& app_id);
+
+  // ExtensionRegistryObserver:
+  void OnExtensionLoaded(content::BrowserContext* browser_context,
+                         const Extension* extension) override;
+
+  void OnExtensionUnloaded(content::BrowserContext* browser_context,
+                           const Extension* extension,
+                           UnloadedExtensionInfo::Reason reason) override;
+
+  // Unowned pointer.
+  content::BrowserContext* context_;
+
+#if defined(OS_CHROMEOS)
+  std::unique_ptr<Updater> app_updater_;
+#endif
+
+  IconMap icon_map_;
+
+  base::WeakPtrFactory<ExtensionAppIconService> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionAppIconService);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_APP_ICON_SERVICE_H_