Refactor extension app icon.

chrome/browser/extensions/chrome_app_icon.h

Index: chrome/browser/extensions/chrome_app_icon.h
diff --git a/chrome/browser/extensions/chrome_app_icon.h b/chrome/browser/extensions/chrome_app_icon.h
new file mode 100644
index 0000000000000000000000000000000000000000..6c552c21c571a760ea28d8eb73670d7b3e1d66d4
--- /dev/null
+++ b/chrome/browser/extensions/chrome_app_icon.h
@@ -0,0 +1,85 @@
+// 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_CHROME_APP_ICON_H_
+#define CHROME_BROWSER_EXTENSIONS_CHROME_APP_ICON_H_
+
+#include <memory>
+#include <string>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "extensions/browser/extension_icon_image.h"
+#include "ui/gfx/image/image_skia.h"
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+
+class Extension;
+class ChromeAppIconDelegate;
+
+// This represents how an extension app icon should finally look. As a base,
+// extension icon is used and effects that depend on extension type, state and
+// some external conditions are applied. Resulting image is sent via
+// ChromeAppIconDelegate. Several updates are expected in case extension
+// state or some external conditions are changed.
+class ChromeAppIcon : public IconImage::Observer {

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

I'd be very interested to see if all this refactoring could be encapsulated
within extensions::IconImage and ExtensionAppIconLoader/ChromeAppIconLoader
without adding ChromeAppIcon/ChromeAppIconService/ChromeAppIconService::Updater
/ChromeAppIconServiceFactory/ChromeAppIconDelegate. Perhaps additional pieces
could be added (if needed) across multiple CLs? I worry that the added class
complexity and division of responsibilities and inter-relationships here is
detrimental to the long-term maintainability of this code; I certainly don't
find this code easy to understand or review. I'm likely to defer approval to
other reviewers/owners, as I can't conceptualize the need for the added
complexity here. On the bright side, I do appreciate some of the simplification
of the various ChromeAppIcon* clients.

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

You might miss that but I already described reasons why I did so.

Let me summarize.
We have different using concept. Direct icon and via
Chrome(Extension)AppIconLoader. So that means we cannot encapsulate all
functionality in ChromeAppIconLoader because we have several consumers which
uses ChromeAppIcon directly. Solutions: 1) Move functionality to ChromeAppIcon
and 2) Replace ChromeAppIcon to ChromeAppIconLoader but for one icon. 2) is
de-factor the same as 1 because.
Having everything in ChromeAppIcon is bad because it has to observe extension
registry and handle all callbacks from ExtensionRegistry even most of callbacks
are for other extensions. Assume we have 100 icons. That requires the 100
listeners in extension registry. Now one event is sent for some extension. It
will be delivered to 100 handlers and only one has reason to handle each. Now
Extension Registry is quite busy system and it sends events quite frequently.
Next we have problem with ArcAppListPrefs. Each icon has to observe again,
messages from ArcAppListPrefs. Problems are the same as I described above. Next
problem is not obvious but I faced with it and it was not simple to solve. We
have different life cycles of Extension services and ArcAppLIstPrefs. That means
we have to implement additional observer to listen for ArcAppListPrefs shutdown
event. That was really hack code. That was my one of the my first
implementation. I very reluctantly added this new service. However it helps to
remove all drawback. Yes code is not trivial but alternative is worse.

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

Having a single object to monitor extension changes for all icons seems
reasonable, and sound like a perfect job for the icon loader. I don't think we
should have both a loader *and* a new service with such similar logic and
responsibilities. Could you make AppIconLoader a singleton or
profile-keyed-service and move all the service code/logic into that class? That
should make it easier for clients to get updating icons without owning their own
loader. I'd also like to see extensions::IconImage merged with ChromeAppIcon,
but that seems a little less important. WDYT?

[khmel, 2017/05/10 20:26:22]

1. AppIconLoader is base class, we have ArcAppIconLoader and
ChromeAppIconLoader. 
2. AppIconLoader is per specific icon size.
3. Some codes use icon directly.

These 3 points makes it is impossible to achieve without big refactoring.
I am not stopping Icon work with current CL. I have some tasks for ARC icons to
refactor. Ideally I plan to get rid of AppIconLoader(s) and make everything
using ChromeAppIconService including ARC icons. So my next step looks very
similar to what you suggest (just ChromeAppIconService is AppIconLoader in your
terms).
Beside refactoring this CL fixes several real issues.
Let do it as separate CL if you don't mind.

+ public:
+  using DestroyedCallback = base::OnceCallback<void(ChromeAppIcon*)>;
+
+  ChromeAppIcon(ChromeAppIconDelegate* delegate,
+                content::BrowserContext* browser_context,
+                DestroyedCallback destroyed_callback,
+                const std::string& app_id,
+                int resource_size_in_dip);
+  ~ChromeAppIcon() override;
+
+  // Reloads icon.
+  void Reload();
+
+  // Returns true if icon is loaded and attached to valid extension.
+  bool IsValid() const;
+
+  // Re-applies app effects over the current extension icon and dispatches the
+  // result via |delegate_|.
+  void UpdateIcon();
+
+  // Generates all images for supported scale factors.
+  void EnsureRepsForSupportedScales();

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

It seems like this is only called in tests; if so add "ForTesting". It also
seems like ChromeAppIconLoader::FetchImage does the same thing when initially
loading the image, so I wonder if this is necessary at all?

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

Actually you are right. I thought that once we applied effects we lost reference
to original ImageSkia. However both, RoundedCornersImageSource and
CreateHSLShiftedImage contains reference to original ImageSkia. I tested and it
works well. So it is possible to discard this code. Thanks!

+
+  const gfx::ImageSkia& image_skia() const { return image_skia_; }
+  const std::string& app_id() const { return app_id_; }
+
+ private:
+  const Extension* GetExtension();
+
+  // IconImage::Observer:
+  void OnExtensionIconImageChanged(IconImage* image) override;
+
+  // Unowned pointers.
+  ChromeAppIconDelegate* const delegate_;
+  content::BrowserContext* const browser_context_;
+
+  // Called when this instance of ChromeAppIcon is destroyed.
+  DestroyedCallback destroyed_callback_;
+
+  const std::string app_id_;
+
+  // Contains current icon image. This is static image with applied effects and
+  // it is updated each time when |icon_| is updated.
+  gfx::ImageSkia image_skia_;
+
+  const int resource_size_in_dip_;
+
+  std::unique_ptr<IconImage> icon_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeAppIcon);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_CHROME_APP_ICON_H_