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..22093ff47122b0450d7257653f36ac6aa20bd27f
--- /dev/null
+++ b/chrome/browser/extensions/chrome_app_icon.h
@@ -0,0 +1,83 @@
+// 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;
+class IconImage;

[msw, 2017/05/03 01:17:38]

nit: not needed; defined in extension_icon_image.h

[khmel, 2017/05/03 02:15:22]

Done.

+
+// 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/03 01:17:38]

If each ChromeAppIcon is owned by the transient UI displaying the icon (app list
items, app list search results, app info dialog header... each of which might be
requesting a different size_in_dip anyway), and then destroyed when that
transient UI goes away, is it worthwhile to have a centralized cache at all? How
often is the icon actually re-used between these, instead of destroyed and then
reloaded over and over? Perhaps it would make sense to just refine AppIconLoader
instead of adding the new service? Also, why doesn't the shelf use these icons?

[khmel, 2017/05/03 02:15:22]

Having cache is actually good idea. I have this idea in mind. Once we have
centralized service we can used it as a base for this. It can be task to do.
However don't want to complicate this already huge CL.
Actually does. Shelf uses ExtensionAppIconLoader (renamed here to
ChromeAppIconLoader)

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

Sorry, I don't think my comment was clear. I think this code is strange, because
(AFAICT) it only "caches" the icons in the service when they are alive and owned
and being shown by (mostly) temporary UI. The icons aren't owned by the service,
and they are created each time some UI needs to show them (ie. app list), and
then destroyed as soon as that UI is hidden. Is that correct? Even if the
service could cache the icons, I don't think that would be a good idea, since
that just seems like a waste of memory. Can you measure and describe the
benefits to the centralized "caching", instead of just loading the icons and
adornments for each bit of UI as needed? My guess is that there's a simpler
solution that would only lose marginal efficiency (re-loading an app icon for
the app list that's already pinned on the shelf).

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

Yes, this is correct. Service does not own icon. It just tracks created icon and
updates them once needed. Once icon is not longer used in UI it gets destroyed
and service no longer tracks it.
My idea is similar what you suggest here. To have something like ref-counter.
if icon is used in multiple places. However it is just idea and I don't plan to
do this in nearest time.

+ public:
+  using DestroyedCallback = base::OnceCallback<void(ChromeAppIcon*)>;
+
+  ChromeAppIcon(ChromeAppIconDelegate* delegate,
+                content::BrowserContext* 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.

[msw, 2017/05/03 01:17:38]

What does it mean for the icon to be attached to a valid extension? I don't see
how that's checked in the impl.

[khmel, 2017/05/03 02:15:22]

When extension is disabled, IconImage discards this extension and cannot longer
perform updates.

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

So the icon becomes invalid? Please clarify the comment to mention the extension
being disabled or similar? "attached" is odd

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

In recent revision we creates icon_ at CTOR. That means we can drop 'is loaded'.
Tried to re-phrase for better understanding.

+  bool IsValid() const;
+
+  void UpdateIcon();

[msw, 2017/05/03 01:17:38]

nit: comment; maybe allude to how UpdateIcon differs from Reload?

[khmel, 2017/05/03 02:15:21]

Done.

+
+  // Generates all images for supported scale factors.
+  void EnsureRepsForSupportedScales();

[msw, 2017/05/03 01:17:38]

nit: remove this, let the one non-test caller (ChromeAppIconLoader::FetchImage)
do image_skia().EnsureRepsForSupportedScales() instead.

[khmel, 2017/05/03 02:15:22]

image_skia() from this class is static image with applied effects. It does not
refer to icon_. EnsureRepsForSupportedScales on this image_skia won't trigger
loading actual images.

+
+  const gfx::ImageSkia& image_skia() const { return image_skia_; }
+  const std::string& app_id() const { return app_id_; }
+  content::BrowserContext* context() { return context_; }

[msw, 2017/05/03 01:17:38]

Remove this if it's not used anywhere, otherwise rename browser_context()

[khmel, 2017/05/03 02:15:22]

Done.

+
+ private:
+  const Extension* GetExtension();
+
+  // IconImage::Observer:
+  void OnExtensionIconImageChanged(IconImage* image) override;
+
+  // Unowned pointers.
+  ChromeAppIconDelegate* const delegate_;
+  content::BrowserContext* const context_;

[msw, 2017/05/03 01:17:38]

nit: browser_context_

[khmel, 2017/05/03 02:15:22]

Done.

+
+  DestroyedCallback destroyed_callback_;

[msw, 2017/05/03 01:17:38]

nit: comment, describe when this is called (when the ChromeAppIcon is destroyed?
when the IconImage is destroyed?). Would it be sufficient to instead vend
WeakPtr<ChromeAppIcon>s and store those in the ChromeAppIconService::icon_map_? 

[khmel, 2017/05/03 02:15:22]

Somebody needs to clean the icon from icon_map_, right? so we have to notify
service somehow. Initially I had here delegate and using callback was preference
of previous reviewer.

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

I suppose that's fine. Holding weak pointers also seems acceptable.

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

Acknowledged.

+
+  const std::string app_id_;
+
+  const int resource_size_in_dip_;

[msw, 2017/05/03 01:17:38]

Would it make sense to just expose the underlying value in IconImage, and remove
this redundant value?

[khmel, 2017/05/03 02:15:22]

resource_size_in_dip_ is actually used to create/recreate IconImage.

+
+  // Contains current icon image.
+  gfx::ImageSkia image_skia_;
+
+  std::unique_ptr<IconImage> icon_;

[msw, 2017/05/03 01:17:38]

How does this IconImage |icon_|, which contains an ImageSkia, relate to the
ImageSkia |image_skia_| above? Are they the same? If so, maybe remove
|image_skia_| and just add an accessor for icon_->image_skia() if needed.
Otherwise, maybe explain in comments here how they are different.

[khmel, 2017/05/03 02:15:22]

Done in comment

+
+  DISALLOW_COPY_AND_ASSIGN(ChromeAppIcon);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_CHROME_APP_ICON_H_