Refactor extension app icon.

chrome/browser/extensions/chrome_app_icon.cc

Index: chrome/browser/extensions/chrome_app_icon.cc
diff --git a/chrome/browser/extensions/chrome_app_icon.cc b/chrome/browser/extensions/chrome_app_icon.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c5dd0515cf08e8135ce8ada452f16110e68c7cac
--- /dev/null
+++ b/chrome/browser/extensions/chrome_app_icon.cc
@@ -0,0 +1,137 @@
+// 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.h"
+
+#include <algorithm>
+
+#include "chrome/browser/extensions/chrome_app_icon_delegate.h"
+#include "chrome/browser/extensions/extension_util.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/manifest_handlers/icons_handler.h"
+#include "ui/gfx/canvas.h"
+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/image/canvas_image_source.h"
+#include "ui/gfx/image/image_skia_operations.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/extensions/gfx_utils.h"
+#endif
+
+namespace extensions {
+
+namespace {
+
+gfx::ImageSkia CreateDisabledIcon(const gfx::ImageSkia& icon) {

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

nit: inline for the one caller.

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

There are several places in code with dup functionality. I keep in mind
possibility to provide helper.

+  const color_utils::HSL shift = {-1, 0, 0.6};
+  return gfx::ImageSkiaOperations::CreateHSLShiftedImage(icon, shift);
+}
+
+// Rounds the corners of a given image.
+class RoundedCornersImageSource : public gfx::CanvasImageSource {

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

Can we avoid subclassing CanvasImageSource and just use a function to generate
an ImageSkia with rounded corners (defined by a mask or premultiplied alpha
values built into the ImageSkia)?

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

Would TODO here be acceptable? That was legacy code and I would prefer to do it
separately.

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

Yes, since you are just moving this code, it's fine as-is, a TODO is optional.

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

TODO added.

+ public:
+  explicit RoundedCornersImageSource(const gfx::ImageSkia& icon)
+      : gfx::CanvasImageSource(icon.size(), false), icon_(icon) {}
+  ~RoundedCornersImageSource() override {}
+
+ private:
+  // gfx::CanvasImageSource overrides:
+  void Draw(gfx::Canvas* canvas) override {
+    // The radius used to round the app icon, based on 2 pixel per 48 pixels
+    // icon size.
+    const int rounding_radius =
+        std::max(static_cast<int>(std::round(2.0 * icon_.width() / 48.0)), 1);

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

int: use std::max<int> and avoid the cast, or use non-integral values

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

Done

+
+    canvas->DrawImageInt(icon_, 0, 0);
+
+    cc::PaintFlags masking_flags;
+    masking_flags.setBlendMode(SkBlendMode::kDstIn);
+    canvas->SaveLayerWithFlags(masking_flags);
+
+    cc::PaintFlags mask_flags;
+    mask_flags.setAntiAlias(true);
+    mask_flags.setColor(SK_ColorWHITE);
+    canvas->DrawRoundRect(gfx::Rect(icon_.width(), icon_.height()),
+                          rounding_radius, mask_flags);
+
+    canvas->Restore();
+  }
+
+  gfx::ImageSkia icon_;
+
+  DISALLOW_COPY_AND_ASSIGN(RoundedCornersImageSource);
+};
+
+}  // namespace
+
+ChromeAppIcon::ChromeAppIcon(ChromeAppIconDelegate* delegate,
+                             content::BrowserContext* context,
+                             DestroyedCallback destroyed_callback,
+                             const std::string& app_id,
+                             int resource_size_in_dip)
+    : delegate_(delegate),
+      context_(context),
+      destroyed_callback_(std::move(destroyed_callback)),
+      app_id_(app_id),
+      resource_size_in_dip_(resource_size_in_dip) {
+  DCHECK(delegate_);
+  DCHECK(context_);
+  DCHECK(!destroyed_callback_.is_null());
+  DCHECK_GE(resource_size_in_dip, 0);
+  Reload();
+}
+
+ChromeAppIcon::~ChromeAppIcon() {
+  std::move(destroyed_callback_).Run(this);

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

Why do you need to move the callback to run it?

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

I was requested to use base::OnceCallback and this is how it is called.

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

Acknowledged.

+}
+
+const Extension* ChromeAppIcon::GetExtension() {
+  const Extension* extension =

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

nit: inline this in the return statement.

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

Done.

+      ExtensionRegistry::Get(context_)->GetInstalledExtension(app_id_);
+  return extension;
+}
+
+bool ChromeAppIcon::IsValid() const {
+  return icon_ && icon_->is_valid();
+}
+
+void ChromeAppIcon::Reload() {
+  const Extension* extension = GetExtension();

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

nit: inline in call below

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

It is used twice below, extension, IconsInfo::GetIcons(extension)

+  icon_ = base::MakeUnique<IconImage>(
+      context_, extension, IconsInfo::GetIcons(extension),
+      resource_size_in_dip_, util::GetDefaultAppIcon(), this);
+  UpdateIcon();
+}
+
+void ChromeAppIcon::UpdateIcon() {
+  DCHECK(icon_);
+
+  image_skia_ = icon_->image_skia();
+#if defined(OS_CHROMEOS)
+  util::MaybeApplyChromeBadge(context_, app_id_, &image_skia_);
+#endif
+
+  const bool enabled = util::IsAppLaunchable(app_id_, context_);

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

nit: inline in conditional

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

Done.

+  if (!enabled)
+    image_skia_ = CreateDisabledIcon(image_skia_);
+
+  const Extension* extension = GetExtension();
+  if (extension && extension->from_bookmark())
+    image_skia_ = gfx::ImageSkia(new RoundedCornersImageSource(image_skia_),
+                                 image_skia_.size());
+
+  delegate_->OnIconUpdated(this);
+}
+
+void ChromeAppIcon::EnsureRepsForSupportedScales() {
+  DCHECK(icon_);
+  icon_->image_skia().EnsureRepsForSupportedScales();
+}
+
+void ChromeAppIcon::OnExtensionIconImageChanged(IconImage* icon) {
+  DCHECK_EQ(icon_.get(), icon);
+  UpdateIcon();
+}
+
+}  // namespace extensions