arc: Set custom icon in shelf for ARC apps.

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

+// 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_UI_ASH_LAUNCHER_ARC_APP_WINDOW_H_
+#define CHROME_BROWSER_UI_ASH_LAUNCHER_ARC_APP_WINDOW_H_
+
+#include <string>
+#include <vector>
+
+#include "ash/public/cpp/shelf_types.h"
+#include "base/macros.h"
+#include "chrome/browser/image_decoder.h"
+#include "chrome/browser/ui/ash/launcher/arc_app_shelf_id.h"
+#include "ui/base/base_window.h"
+#include "ui/gfx/image/image_skia.h"
+
+class ArcAppWindowLauncherController;
+class ArcAppWindowLauncherItemController;
+
+namespace views {
+class Widget;
+}
+
+// A ui::BaseWindow for a chromeos launcher to control ARC applications.
+class ArcAppWindow : public ui::BaseWindow, public ImageDecoder::ImageRequest {

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

Is this done by "git cl format"? If not, might be nice to put each parent class
in its own line.

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

Hmm, I always put this to separate line. So this is "git cl format" 	
trick. I validated once again and yes, formatter decides that this is better.

[xiyuan, 2017/05/18 17:33:30]

Acknowledged.

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

It's too bad that we can't use and extend an existing ui::BaseWindow subclass,
like NativeAppWindow (for extensions). Or simply let the [Item]Controllers deal
with views::Widgets and aura::Windows.

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

Probably, but that is outside of context of this CL. IIRC AppWindow for
extension has long history and was already sub-classed from ui::BaseWindow. Arc
launcher item just inherited this concept. 

+ public:
+  enum class FullScreenMode {

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

Remove this and use a bool set to false by default, or use an existing enum,
like ash::wm::WindowStateType and the existing ui::BaseWindow::IsFullscreen().
Avoid adding duplicate types/functions that someone will need to combine later.

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

This is legacy code and I don't touch this functionality in this CL. Let do it
separately.

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

Add a TODO, this is technical debt.

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

Done.

+    NOT_DEFINED,  // Fullscreen mode was not defined.
+    ACTIVE,       // Fullscreen is activated for an app.
+    NON_ACTIVE,   // Fullscreen was not activated for an app.
+  };
+
+  ArcAppWindow(int task_id,
+               const arc::ArcAppShelfId& app_shelf_id,
+               views::Widget* widget,
+               ArcAppWindowLauncherController* owner);
+
+  ~ArcAppWindow() override;
+
+  static void DisableSafeIconDecodingForTesting();

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

Add comments for all non-obvious (non-accessor/setter) functions. (ie. a reader
should understand why we'd disable safe icon decoding for testing)

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

Done.

+
+  void SetController(ArcAppWindowLauncherItemController* controller);
+
+  void ResetController();

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

Remove this, let users call SetController(nullptr) as needed.

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

Done.

+
+  void SetFullscreenMode(FullScreenMode mode);
+
+  // Sets optional window title and icon.
+  void SetDescription(const std::string& title,

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

Should this be two separate functions?

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

They are set together. Imho we can handle it together as well.

+                      const std::vector<uint8_t>& unsafe_icon_data_png);

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

nit: "unsafe" needs a comment.

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

Done

+
+  FullScreenMode fullscreen_mode() const { return fullscreen_mode_; }
+
+  int task_id() const { return task_id_; }
+
+  const arc::ArcAppShelfId& app_shelf_id() const { return app_shelf_id_; }
+
+  const ash::ShelfID& shelf_id() const { return shelf_id_; }
+
+  void set_shelf_id(const ash::ShelfID& shelf_id) { shelf_id_ = shelf_id; }
+
+  views::Widget* widget() const { return widget_; }
+
+  ArcAppWindowLauncherItemController* controller() { return controller_; }

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

nit: const?

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

Done. However in one of my previous CL reviewer told me if we return
non-constant member (that time it was also pointer) we should not set const.

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

Ah, good point; feel free to keep this non-const.

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

Done.

+
+  const gfx::ImageSkia& image_skia() { return image_skia_; }

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

nit: const?

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

Done.

+
+  // ui::BaseWindow:
+  bool IsActive() const override;
+  bool IsMaximized() const override;
+  bool IsMinimized() const override;
+  bool IsFullscreen() const override;
+  gfx::NativeWindow GetNativeWindow() const override;
+  gfx::Rect GetRestoredBounds() const override;
+  ui::WindowShowState GetRestoredState() const override;
+  gfx::Rect GetBounds() const override;
+  void Show() override;
+  void ShowInactive() override;
+  void Hide() override;
+  void Close() override;
+  void Activate() override;
+  void Deactivate() override;
+  void Maximize() override;
+  void Minimize() override;
+  void Restore() override;
+  void SetBounds(const gfx::Rect& bounds) override;
+  void FlashFrame(bool flash) override;
+  bool IsAlwaysOnTop() const override;
+  void SetAlwaysOnTop(bool always_on_top) override;
+
+ private:
+  // Resets custom icon if it was previously set. If current window is an active

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

nit: maybe just "  // Resets the icon and updates |controller_|'s active icon as
needed." (if you need to keep this function, hopefully not)

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

I described in other comments that user may want to reset previously set custom
icon and this is valid case (icon in Android TaskDescription is optional value).
Comment updated.

+  // window in context of controller then updates controller icon.
+  void ResetIcon();
+
+  // ImageDecoder::ImageRequest:
+  void OnImageDecoded(const SkBitmap& decoded_image) override;
+  void OnDecodeImageFailed() override;
+
+  const int task_id_;

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

Please comment on all members.

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

Done.

+  const arc::ArcAppShelfId app_shelf_id_;
+  ash::ShelfID shelf_id_;
+  FullScreenMode fullscreen_mode_ = FullScreenMode::NOT_DEFINED;
+  gfx::ImageSkia image_skia_;

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

nit: Rename this and the accessor to convey the purpose of this image_skia (ie.
icon)

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

Done.

+  // Unowned pointers
+  views::Widget* const widget_;
+  ArcAppWindowLauncherController* owner_;

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

nit: ArcAppWindowLauncherController* const owner_;

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

Done.

+  ArcAppWindowLauncherItemController* controller_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcAppWindow);
+};
+
+#endif  // CHROME_BROWSER_UI_ASH_LAUNCHER_ARC_APP_WINDOW_H_