arc: Set custom icon in shelf for ARC apps.

chrome/browser/ui/ash/launcher/arc_app_window.cc

Index: chrome/browser/ui/ash/launcher/arc_app_window.cc
diff --git a/chrome/browser/ui/ash/launcher/arc_app_window.cc b/chrome/browser/ui/ash/launcher/arc_app_window.cc
new file mode 100644
index 0000000000000000000000000000000000000000..433fb668a394069e14686a49a3b77d50eaf199c2
--- /dev/null
+++ b/chrome/browser/ui/ash/launcher/arc_app_window.cc
@@ -0,0 +1,217 @@
+// 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/ui/ash/launcher/arc_app_window.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
+#include "chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h"
+#include "chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.h"
+#include "components/exo/shell_surface.h"
+#include "extensions/common/constants.h"
+#include "ui/aura/window.h"
+#include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/image/image_skia_operations.h"
+#include "ui/gfx/image/image_skia_source.h"
+#include "ui/views/widget/widget.h"
+
+namespace {
+
+// Set to true for unit tests to avoid IPC icon decoding.

[msw, 2017/05/18 18:26:48]

nit: explain why that's useful.

[khmel, 2017/05/18 20:27:50]

Done.

+bool disable_safe_icon_decoding = false;

[msw, 2017/05/18 18:26:48]

nit: append _for_testing to the identifier itself

[khmel, 2017/05/18 20:27:51]

Done.

+
+// Source of the custom icon of ARC app window. It takes reference bitmap and
+// resizes to required scale factor on demand.
+class CustomIconSource : public gfx::ImageSkiaSource {

[xiyuan, 2017/05/18 16:22:49]

Can we get rid of this class and do something like the following ?

image_skia_ = gfx::ImageSkiaOperations::CreateResizedImage(
    gfx::ImageSkia(gfx::ImageSkiaRep(decoded_image, 1.0f),
    gfx::Size(extension_misc::EXTENSION_ICON_SMALL,
              extension_misc::EXTENSION_ICON_SMALL));

Think that should give us the same resized icon.

[khmel, 2017/05/18 17:29:34]

Yes, if required scale is missing then 1.0 is taken and scaled. Done

[msw, 2017/05/18 22:03:38]

Nice!

[khmel, 2017/05/18 22:40:02]

Acknowledged.

+ public:
+  explicit CustomIconSource(const SkBitmap& base_image)
+      : base_image_(base_image) {}
+  ~CustomIconSource() override = default;
+
+ private:
+  // gfx::ImageSkiaSource overrides:
+  gfx::ImageSkiaRep GetImageForScale(float scale) override {
+    const gfx::Size target_pixel_size =
+        gfx::ScaleToCeiledSize(gfx::Size(extension_misc::EXTENSION_ICON_SMALL,
+                                         extension_misc::EXTENSION_ICON_SMALL),
+                               scale);
+
+    const SkBitmap resized = skia::ImageOperations::Resize(
+        base_image_, skia::ImageOperations::RESIZE_BEST,
+        target_pixel_size.width(), target_pixel_size.height());
+    return gfx::ImageSkiaRep(resized, scale);
+  }
+
+  const SkBitmap base_image_;
+
+  DISALLOW_COPY_AND_ASSIGN(CustomIconSource);
+};
+
+}  // namespace
+
+ArcAppWindow::ArcAppWindow(int task_id,
+                           const arc::ArcAppShelfId& app_shelf_id,
+                           views::Widget* widget,
+                           ArcAppWindowLauncherController* owner)
+    : task_id_(task_id),
+      app_shelf_id_(app_shelf_id),
+      widget_(widget),
+      owner_(owner) {}
+
+ArcAppWindow::~ArcAppWindow() {
+  ImageDecoder::Cancel(this);
+}
+
+// static
+void ArcAppWindow::DisableSafeIconDecodingForTesting() {
+  disable_safe_icon_decoding = true;
+}
+
+void ArcAppWindow::SetController(
+    ArcAppWindowLauncherItemController* controller) {
+  DCHECK(!controller_ && controller);
+  controller_ = controller;

[xiyuan, 2017/05/18 16:22:49]

Do we need to ensure that the launcher icon is updated in |controller_|?

[khmel, 2017/05/18 17:29:34]

Practically no, because AddWindow is called the same time as controller set.
However it may make sense to add here call as well.

+}
+
+void ArcAppWindow::ResetController() {
+  controller_ = nullptr;
+}
+
+void ArcAppWindow::SetFullscreenMode(FullScreenMode mode) {
+  DCHECK(mode != FullScreenMode::NOT_DEFINED);
+  fullscreen_mode_ = mode;
+}
+
+void ArcAppWindow::SetDescription(
+    const std::string& title,
+    const std::vector<uint8_t>& unsafe_icon_data_png) {
+  if (title.length())

[msw, 2017/05/18 18:26:48]

nit !empty?

[khmel, 2017/05/18 20:27:50]

Done.

+    GetNativeWindow()->SetTitle(base::string16(base::UTF8ToUTF16(title)));

[msw, 2017/05/18 18:26:48]

nit: base::string16 is probably not needed (adds a copy)

[khmel, 2017/05/18 20:27:51]

Done.

+  ImageDecoder::Cancel(this);
+  if (unsafe_icon_data_png.empty()) {
+    ResetIcon();

[msw, 2017/05/18 18:26:48]

Can we just continue using the current icon until a new one is successfully
decoded? It would be nice to avoid unnecessary work/code and avoid blank icons
flashing during decode, etc.

[khmel, 2017/05/18 20:27:51]

In this case we pass empty data which means custom icon is reset. ResetIcon just
discard (if it was previously set) custom icon. No blank icon should appear:
 * If we set custom icon then we try to decode it first and on decode completed
we update actual icon. During the decoding default or previous custom icon is
used. 
 * If we reset custom icon we immediately discards inline previous custom icon
and default icon is used. Also no blank icon.

[msw, 2017/05/18 22:03:38]

So you're trying to use the app/activity icon instead of the window icon in this
case? That might make sense, but it would be much clearer if both this class and
the extension version dealt directly with the app and window icon properties,
and then the AppWindowLauncher[Item]Controller handled window properties
changing in a common way.

[khmel, 2017/05/18 22:40:02]

This makes sense but sounds as refactoring, quite big as I predict. I think let
do it as separate CL. In meantime it is good to have everything in one style.

[msw, 2017/05/18 22:50:11]

I look forward to seeing the cleanup change for http://crbug.com/724292.
It's *bad* to duplicate old complex code and increase technical debt.

+    return;
+  }
+
+  if (disable_safe_icon_decoding) {
+    SkBitmap bitmap;
+    if (gfx::PNGCodec::Decode(&unsafe_icon_data_png[0],
+                              unsafe_icon_data_png.size(), &bitmap)) {
+      OnImageDecoded(bitmap);
+    } else {
+      OnDecodeImageFailed();
+    }
+  } else {
+    ImageDecoder::Start(this, unsafe_icon_data_png);
+  }
+}
+
+bool ArcAppWindow::IsActive() const {
+  return widget_->IsActive() && owner_->active_task_id() == task_id_;

[msw, 2017/05/18 18:26:48]

Do we need to maintain two separate notions of Active? When are they ever in
conflict? Should we instead just DCHECK that one is in sync with the other? (if
they are indeed changed together synchronously).

[khmel, 2017/05/18 20:27:50]

Activating ARC app is not straightforward. It may be activated from Android side
or by clicking on corresponded Chrome window. It took a time to stabilize focus
and active state of ARC windows. Anyway this code is legacy and I don't modify
it here. If we want to reconsider how it work let do it in separate task.

[msw, 2017/05/18 22:03:39]

It would be nice to clean this up later, this is technical debt.

[khmel, 2017/05/18 22:40:02]

Acknowledged.

+}
+
+bool ArcAppWindow::IsMaximized() const {
+  NOTREACHED();
+  return false;
+}
+
+bool ArcAppWindow::IsMinimized() const {
+  NOTREACHED();
+  return false;
+}
+
+bool ArcAppWindow::IsFullscreen() const {

[msw, 2017/05/18 18:26:48]

Shouldn't this tie into your fullscreen setting? I guess it doesn't matter if
it's never called, but with all the NOTREACHED()s here, I wonder why we need
this ui::BaseWindow in the first place (and not just use views::Widgets and/or
aura::Windows).

[khmel, 2017/05/18 20:27:50]

Similar to previous comment. I would prefer not to experiment with legacy code
in this CL. Let do as separate task.

[msw, 2017/05/18 22:03:38]

Acknowledged.

+  NOTREACHED();
+  return false;
+}
+
+gfx::NativeWindow ArcAppWindow::GetNativeWindow() const {
+  return widget_->GetNativeWindow();
+}
+
+gfx::Rect ArcAppWindow::GetRestoredBounds() const {
+  NOTREACHED();
+  return gfx::Rect();
+}
+
+ui::WindowShowState ArcAppWindow::GetRestoredState() const {
+  NOTREACHED();
+  return ui::SHOW_STATE_NORMAL;
+}
+
+gfx::Rect ArcAppWindow::GetBounds() const {
+  NOTREACHED();
+  return gfx::Rect();
+}
+
+void ArcAppWindow::Show() {
+  widget_->Show();
+}
+
+void ArcAppWindow::ShowInactive() {
+  NOTREACHED();
+}
+
+void ArcAppWindow::Hide() {
+  NOTREACHED();
+}
+
+void ArcAppWindow::Close() {
+  arc::CloseTask(task_id_);
+}
+
+void ArcAppWindow::Activate() {
+  widget_->Activate();
+}
+
+void ArcAppWindow::Deactivate() {
+  NOTREACHED();
+}
+
+void ArcAppWindow::Maximize() {
+  NOTREACHED();
+}
+
+void ArcAppWindow::Minimize() {
+  widget_->Minimize();
+}
+
+void ArcAppWindow::Restore() {
+  NOTREACHED();
+}
+
+void ArcAppWindow::SetBounds(const gfx::Rect& bounds) {
+  NOTREACHED();
+}
+
+void ArcAppWindow::FlashFrame(bool flash) {
+  NOTREACHED();
+}
+
+bool ArcAppWindow::IsAlwaysOnTop() const {
+  NOTREACHED();
+  return false;
+}
+
+void ArcAppWindow::SetAlwaysOnTop(bool always_on_top) {
+  NOTREACHED();
+}
+
+void ArcAppWindow::ResetIcon() {

[msw, 2017/05/18 18:26:48]

Is this actually useful? Can we just use the current icon until it changes? (see
other comments). It seems like the worst case scenario is that an app window had
a valid icon, and somehow changed it, and that decode fails, so we continue
showing the old icon... it seems worth simplifying the code if we just don't
handle odd corner cases like that.

[khmel, 2017/05/18 20:27:50]

I explained in previous comment. App may pass null as task bitmap. In this case
icon_png_data is empty and we reset the icon to default activity icon.

+  if (image_skia_.isNull())
+    return;
+  image_skia_ = gfx::ImageSkia();
+  controller()->OnWindowChanged(this);

[xiyuan, 2017/05/18 16:22:48]

Do we need to worry about |controller_| is nullptr? ResetIcon() does not seems
to be guaranteed to have a controller.

[khmel, 2017/05/18 17:29:34]

Yes, it can happen, especially in multi-profile case. Done

+}
+
+void ArcAppWindow::OnImageDecoded(const SkBitmap& decoded_image) {
+  image_skia_ = gfx::ImageSkia(new CustomIconSource(decoded_image),

[msw, 2017/05/18 18:26:48]

It seems like this calls shouldn't store a separate icon, but instead set the
aura::client::kAppIconKey or kWindowIconKey property on the underlying
aura::Window. Generally, it's better to re-use existing infrastructure and
patterns instead of adding new ones that work differently to accomplish a
similar goal.

[khmel, 2017/05/18 20:27:50]

Hmm. I did similar as extension app window works. https://cs.chromium.org does
not work currently. You may take a look:
chromium/src/extensions/browser/app_window/app_window.h
  const gfx::Image& app_icon() const { return app_icon_; }

[msw, 2017/05/18 22:03:38]

We should consolidate common behavior on the reusable
AppWindowLauncher[Item]Controller base classes, instead of duplicating one-off
behavior between the extension/arc subclasses.

[khmel, 2017/05/18 22:40:02]

As above, I suggest to clean up this as separate CL.

+                               gfx::Size(extension_misc::EXTENSION_ICON_SMALL,
+                                         extension_misc::EXTENSION_ICON_SMALL));
+  controller()->OnWindowChanged(this);

[xiyuan, 2017/05/18 16:22:48]

same here.

[khmel, 2017/05/18 17:29:34]

Done.

+}
+
+void ArcAppWindow::OnDecodeImageFailed() {
+  ResetIcon();

[msw, 2017/05/18 18:26:48]

Why should the controller be notified when the icon decoding fails? Can it just
continue using the existing (perhaps empty) icon and not need to be notified?

[khmel, 2017/05/18 20:27:50]

We probably may continue using old icon. And this should be exceptional case,
once we were able to compress icon to png on ARC side.
Removed this one

+}