Refactor extension app icon.

chrome/browser/extensions/chrome_app_icon_service.cc

Index: chrome/browser/extensions/chrome_app_icon_service.cc
diff --git a/chrome/browser/extensions/chrome_app_icon_service.cc b/chrome/browser/extensions/chrome_app_icon_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3c9ee48ca8ce49abc9ade1683f19b6d0ee7fd49f
--- /dev/null
+++ b/chrome/browser/extensions/chrome_app_icon_service.cc
@@ -0,0 +1,113 @@
+// 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.
+
+#include "chrome/browser/extensions/chrome_app_icon_service.h"
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/extensions/chrome_app_icon.h"
+#include "chrome/browser/extensions/chrome_app_icon_service_factory.h"
+#include "extensions/browser/extension_registry.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h"
+#endif
+
+namespace extensions {
+
+#if defined(OS_CHROMEOS)
+
+class ChromeAppIconService::Updater : public LauncherAppUpdater::Delegate {

[msw, 2017/05/08 19:59:53]

Add a class comment if you keep this around.

[khmel, 2017/05/09 00:05:04]

Done.

+ public:
+  Updater(ChromeAppIconService* host, content::BrowserContext* context)
+      : host_(host), extension_app_updater_(this, context) {}
+
+  ~Updater() override = default;
+
+ private:
+  // LauncherAppUpdater::Delegate:
+  void OnAppUpdated(content::BrowserContext* browser_context,

[msw, 2017/05/08 19:59:53]

Is this not already accomplished by ChromeAppIconService's
ExtensionRegistryObserver functionality? When is an app updated without
notifying extension registry observers? If this is needed in addition to that,
why not simply make ChromeAppIconService also inherit from this interface,
rather than creating a separate class to 

[khmel, 2017/05/09 00:05:04]

No, LauncherExtensionAppUpdater in ChromeOS additionally listens for ARC app
events. Some ARC app may influence how extension based app looks like
(additional badging for Chrome app in case ARC has app that duplicates Chrome
app, for example Hangouts)
And this ChromeAppIconService::Updater is valid only for ChromeOS whenever 
ChromeAppIconService works on all platforms

[msw, 2017/05/10 19:05:21]

Why not simply make ChromeAppIconService inherit from
LauncherAppUpdater::Delegate in a #if defined(OS_CHROMEOS) block?

[khmel, 2017/05/11 02:13:48]

I did not do this because introduce heavy logic in header. I don't have strong
preference so let do as you suggest.

+                    const std::string& app_id) override {
+    host_->OnAppUpdated(app_id);
+  }
+
+  // Unowned pointer.
+  ChromeAppIconService* const host_;
+
+  LauncherExtensionAppUpdater extension_app_updater_;

[msw, 2017/05/08 19:59:53]

What is this member used for? It's instantiated but not accessed, and that class
has no comments. It seems to be an ExtensionRegistryObserver (like
ChromeAppIconService), so I assume it's filling in some gaps there (which should
probably just be implemented directly on ChromeAppIconService). It's also an
ArcAppListPrefs::Observer, which I assume is observing arc app state changes
similar to the extension app state changes (perhaps that too should be folded
into ChromeAppIconService).

[khmel, 2017/05/09 00:05:04]

It used like: extension_app_updater_(this, context) 
Where this is LauncherAppUpdater::Delegate

+
+  DISALLOW_COPY_AND_ASSIGN(Updater);
+};
+
+#endif
+
+// static
+ChromeAppIconService* ChromeAppIconService::Get(
+    content::BrowserContext* context) {
+  return ChromeAppIconServiceFactory::GetInstance()->GetForBrowserContext(
+      context);
+}
+
+ChromeAppIconService::ChromeAppIconService(content::BrowserContext* context)
+    : context_(context), observer_(this), weak_ptr_factory_(this) {
+#if defined(OS_CHROMEOS)
+  app_updater_ = base::MakeUnique<Updater>(this, context);
+#endif
+
+  observer_.Add(ExtensionRegistry::Get(context_));
+}
+
+ChromeAppIconService::~ChromeAppIconService() = default;
+
+void ChromeAppIconService::Shutdown() {
+#if defined(OS_CHROMEOS)
+  app_updater_.reset();
+#endif
+}
+
+std::unique_ptr<ChromeAppIcon> ChromeAppIconService::CreateIcon(
+    ChromeAppIconDelegate* delegate,
+    const std::string& app_id,
+    int resource_size_in_dip) {
+  std::unique_ptr<ChromeAppIcon> icon = base::MakeUnique<ChromeAppIcon>(
+      delegate, context_,
+      base::Bind(&ChromeAppIconService::OnIconDestroyed,
+                 weak_ptr_factory_.GetWeakPtr()),
+      app_id, resource_size_in_dip);
+
+  icon_map_[icon->app_id()].insert(icon.get());
+  return icon;
+}
+
+void ChromeAppIconService::OnExtensionLoaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension) {
+  OnAppUpdated(extension->id());
+}
+
+void ChromeAppIconService::OnExtensionUnloaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    UnloadedExtensionReason reason) {
+  OnAppUpdated(extension->id());
+}
+
+void ChromeAppIconService::OnAppUpdated(const std::string& app_id) {
+  IconMap::const_iterator it = icon_map_.find(app_id);
+  if (it == icon_map_.end())
+    return;
+  for (auto* icon : it->second)
+    icon->UpdateIcon();
+}
+
+void ChromeAppIconService::OnIconDestroyed(ChromeAppIcon* icon) {
+  DCHECK(icon);
+  IconMap::iterator it = icon_map_.find(icon->app_id());
+  DCHECK(it != icon_map_.end());
+  it->second.erase(icon);
+  if (it->second.empty())
+    icon_map_.erase(it);
+}
+
+}  // namespace extensions