Handle app custom icon via aura::Window property.

chrome/browser/ui/views/apps/chrome_native_app_window_views.cc

Index: chrome/browser/ui/views/apps/chrome_native_app_window_views.cc
diff --git a/chrome/browser/ui/views/apps/chrome_native_app_window_views.cc b/chrome/browser/ui/views/apps/chrome_native_app_window_views.cc
index a3d48e9b5f50bb135452f819c78e43821fab8358..b4f811941360be1520ed77ee3766d304b94c7af6 100644
--- a/chrome/browser/ui/views/apps/chrome_native_app_window_views.cc
+++ b/chrome/browser/ui/views/apps/chrome_native_app_window_views.cc
@@ -13,11 +13,16 @@
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
+#include "chrome/browser/extensions/chrome_app_icon.h"
+#include "chrome/browser/extensions/chrome_app_icon_service.h"
+#include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
 #include "components/favicon/content/content_favicon_driver.h"
 #include "components/zoom/page_zoom.h"
 #include "components/zoom/zoom_controller.h"
+#include "extensions/browser/app_window/app_delegate.h"
+#include "ui/gfx/image/image_skia_operations.h"
 #include "ui/views/controls/webview/webview.h"
 #include "ui/views/widget/widget.h"
 
@@ -249,11 +254,35 @@ bool ChromeNativeAppWindowViews::IsAlwaysOnTop() const {
 // views::WidgetDelegate implementation.
 
 gfx::ImageSkia ChromeNativeAppWindowViews::GetWindowAppIcon() {
-  gfx::Image app_icon = app_window()->app_icon();
-  if (app_icon.IsEmpty())
-    return GetWindowIcon();
-  else
-    return *app_icon.ToImageSkia();
+  const gfx::Image& custom_image = app_window()->custom_app_icon();

[msw, 2017/05/23 00:02:29]

Should the resulting icon be cached, if this function is called a lot?

[khmel, 2017/05/23 16:11:44]

Actually it is cached when a widget handles UpdateIcon. It reads icons from
WidgetDelegate and sets it into aura::client::kAppIconKey property. This code is
called only in case icon is changed.

[msw, 2017/05/23 17:17:40]

Good to know, maybe mention that in a comment?

+
+  if (app_window()->app_icon_url().is_valid() &&
+      app_window()->show_in_shelf()) {

[msw, 2017/05/23 00:02:29]

Why does show_in_shelf matter here? It didn't in AppWindow::UpdateAppIcon. Does
this prevent windows that shouldn't be in the shelf from having an icon shown in
the window's own titlebar?

[khmel, 2017/05/23 16:11:44]

This logic came from app_window.cc (with small refactoring not to use 2 urls). I
checked implementation history but I am not so sure that logic was correct. It
computes some image 128x128 size and applies default app icon as a badge. This
is strange because app icon based on available constants from AppDelegate is 32
or 48. That image was used to return from here as window app icon (see left size
please). From my understanding this more window icon, not window app icon. In
this CL I kept logic as is. Besides I found one more bug with that impl. Custom
icon was loaded async and there were a race condition when app icon was applied
in shelf first and did not react once custom icon was actually loaded. This CL
also fixes this issue and shelf icon is updated once custom icon is loaded.

[msw, 2017/05/23 17:17:40]

Hmm, it still seems odd to only load the app icon for windows with shelf items.
I thought the app icon might sometimes be used in the window frame if there is
no window icon, but maybe that's not the case. I did find this webrtc code that
tries using the app icon as a fallback for the window icon:
https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/window_icon_...
My thought is that we should load the app icon even if the app window doesn't
want to be shown in the shelf, otherwise, this probably deserves a code comment.

[khmel, 2017/05/23 17:35:00]

Base on this change history, Devlin might know more about it.

+    EnsureAppIconCreated();
+    gfx::Image base_image =
+        !custom_image.IsEmpty()
+            ? custom_image
+            : gfx::Image(extensions::util::GetDefaultAppIcon());
+    // Scale the icon to EXTENSION_ICON_LARGE.
+    const int large_icon_size = extension_misc::EXTENSION_ICON_LARGE;
+    if (base_image.Width() != large_icon_size ||
+        base_image.Height() != large_icon_size) {
+      gfx::ImageSkia resized_image =
+          gfx::ImageSkiaOperations::CreateResizedImage(
+              base_image.AsImageSkia(), skia::ImageOperations::RESIZE_BEST,
+              gfx::Size(large_icon_size, large_icon_size));
+      return gfx::ImageSkiaOperations::CreateIconWithBadge(
+          resized_image, app_icon_->image_skia());
+    } else {
+      return gfx::ImageSkiaOperations::CreateIconWithBadge(
+          base_image.AsImageSkia(), app_icon_->image_skia());
+    }
+  }
+
+  if (!custom_image.IsEmpty())
+    return *custom_image.ToImageSkia();
+  EnsureAppIconCreated();
+  return app_icon_->image_skia();
 }
 
 gfx::ImageSkia ChromeNativeAppWindowViews::GetWindowIcon() {
@@ -372,3 +401,22 @@ void ChromeNativeAppWindowViews::InitializeWindow(
       extensions::ExtensionKeybindingRegistry::PLATFORM_APPS_ONLY,
       NULL));
 }
+
+void ChromeNativeAppWindowViews::EnsureAppIconCreated() {
+  if (app_icon_ && app_icon_->IsValid())
+    return;
+
+  app_icon_.reset();

[msw, 2017/05/23 00:02:29]

q: is this necessary right before an assignment?

[khmel, 2017/05/23 16:11:44]

Note sure how to resolve this better. This is to avoid recursive call
GetWindowAppIcon. GetWindowAppIcon calls EnsureAppIconCreated. On Chrome  Icon
creation we call OnIconUpdated. Line 418 is to protect from recursive call. This
icon can also be recreated if was marked as invalid. 

[msw, 2017/05/23 17:17:40]

What you have is probably fine with an explanatory comment.

[khmel, 2017/05/23 17:35:00]

Done.

+  app_icon_ =
+      extensions::ChromeAppIconService::Get(app_window()->browser_context())
+          ->CreateIcon(this, app_window()->extension_id(),
+                       app_window()->app_delegate()->PreferredIconSize());
+}
+
+void ChromeNativeAppWindowViews::OnIconUpdated(
+    extensions::ChromeAppIcon* icon) {
+  if (!app_icon_)
+    return;
+  DCHECK_EQ(app_icon_.get(), icon);
+  UpdateWindowIcon();
+}