Handle app custom icon via aura::Window property.

Handle app custom icon via aura::Window property.

This does refactoring of custom icon functionality for Chrome extension
app and for ARC app by unifying applying a custom icon. Before each app
class has own logic in window controller that applies the custom icon
for the shelf item. With this new CL custom icon is merged to concept
of window app icon property which is set by widget delegate. Base
AppWindowLauncherController observes changes of native window related
to app icon and applies the new window app icon as shelf icon.
This CL also moves loading of Chrome App icon from extensions
sub-project to chrome/browser (AppWindow->ChromeNativeAppWindowViews).
ChromeNativeAppWindowViews is only one consumer of loaded App icon.
From other side, chrome/browser has reach functionality to handle
Chrome app icons, including ChromeAppIcon.
With this CL ExtensionAppLauncher is de-facto deprecated in
ChromeLauncherController (to be removed in next CLs).

BUG=724292
TEST=Tests + Manually on device, including test apps to validate custom
     icons for Extension and ARC based apps.

Review-Url: https://codereview.chromium.org/2900783003
Cr-Commit-Position: refs/heads/master@{#476761}
Committed: https://chromium.googlesource.com/chromium/src/+/1ddcada88969fcf84e51511060588094ddbd54c8

[khmel, 2017-05-22 21:17:59]

Description was changed from

==========
Handle app custom icon via aura::Window property.

This refactor custom icon functionality for Chrome extension app and
for ARC app by unifying appling a custom icon. Before each app class
has own logic in window controller that applies the custom icon for the
shelf item. With this new CL custom icom is merged to concept of window
app icon which is set by widged delegate. Base
AppWindowLauncherController observes changes of native window related
to app icon and applies the new window app icon as shelf icon.
This CL also moves loading of Chrome App icon from extensions
sub-project to chrome/browser (AppWindow->ChromeNativeAppWindowViews).
ChromeNativeAppWindowViews is only one consumber of loaded App icon.
From other side, chrome/browser has reach functionality to handle
Chrome app icons, including ChromeAppIcon.
With this CL ExtensionAppLauncher is de-facto depricated in
ChromeLauncherController (to be removed in next CLs).

BUG=724292
TEST=Tests + Manually on device, including test apps to validate custom
     icons for Extension and ARC based apps.
==========

to

==========
Handle app custom icon via aura::Window property.

This does refactoring of custom icon functionality for Chrome extension
app and for ARC app by unifying applying a custom icon. Before each app
class has own logic in window controller that applies the custom icon
for the shelf item. With this new CL custom icon is merged to concept
of window app icon property which is set by widget delegate. Base
AppWindowLauncherController observes changes of native window related
to app icon and applies the new window app icon as shelf icon.
This CL also moves loading of Chrome App icon from extensions
sub-project to chrome/browser (AppWindow->ChromeNativeAppWindowViews).
ChromeNativeAppWindowViews is only one consumer of loaded App icon.
From other side, chrome/browser has reach functionality to handle
Chrome app icons, including ChromeAppIcon.
With this CL ExtensionAppLauncher is de-facto deprecated in
ChromeLauncherController (to be removed in next CLs).

BUG=724292
TEST=Tests + Manually on device, including test apps to validate custom
     icons for Extension and ARC based apps.
==========

[khmel, 2017-05-22 21:21:46]

khmel@chromium.org changed reviewers:
+ msw@chromium.org

[khmel, 2017-05-22 21:21:47]

Hi Mike,

PTAL 

Thanks!

[msw, 2017-05-23 00:02:34]

I find this change a bit confusing (mostly AppWindow and
ChromeNativeAppWindowViews); are there active maintainers that might understand
some of this better? Sorry I'm having trouble understanding again :-/

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_icon.cc:94: extension ?
IconsInfo::GetIcons(extension) : ExtensionIconSet(),
nit: can IconsInfo::GetIcons just return |g_empty_icon_set.Get()| for a null
extension, like it does when the IconsInfo manifest data is null?

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:90: void
UpdateLauncherItem();
nit: comment (or inline in OnWindowPropertyChanged), rename UpdateShelfItemIcon?

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:695: AppIconLoader*
ChromeLauncherController::GetAppIconLoaderForApp(
nit: reorder to match decl

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:155: void
UpdateAppItemImage(const std::string& app_id);
nit: rename UpdateLauncherItemImage for parity with above? (Or
UpdateShelfItemImage to reduce old 'Launcher' name usage)

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:64:
// icon = app_window->pp_icon();
fix this

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:257: const
gfx::Image& custom_image = app_window()->custom_app_icon();
Should the resulting icon be cached, if this function is called a lot?

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:260:
app_window()->show_in_shelf()) {
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?

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:409:
app_icon_.reset();
q: is this necessary right before an assignment?

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.h (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:17: class
ChromeAppIcon;
nit: not needed (fwd decl in chrome_app_icon_delegate.h)

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:79: void
EnsureAppIconCreated();
nit: comment.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:97:
std::unique_ptr<extensions::ChromeAppIcon> app_icon_;
nit: comment

https://codereview.chromium.org/2900783003/diff/20001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/2900783003/diff/20001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:511: // Custom icon shown in the task
bar or in shelf.
nit: the shelf is chrome's 'task bar', so you can probably remove "or in shelf",
or change it to "or in the Chrome OS shelf."

[James Cook, 2017-05-23 15:23:50]

jamescook@chromium.org changed reviewers:
+ jamescook@chromium.org

[James Cook, 2017-05-23 15:23:50]

I'm not particularly familiar with this code. stevenjb@ or sky@ would be better.

khmel / msw - do you want me to review anyway?

[khmel, 2017-05-23 16:11:42]

khmel@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, stevenjb@chromium.org

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

Thank you Mike and James for taking a look.

I am adding 
Devlin as extensions owner and Steven as James recommended.

PTAL

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_icon.cc:94: extension ?
IconsInfo::GetIcons(extension) : ExtensionIconSet(),
On 2017/05/23 00:02:28, msw wrote:
> nit: can IconsInfo::GetIcons just return |g_empty_icon_set.Get()| for a null
> extension, like it does when the IconsInfo manifest data is null?

This might be convenient. Done as recommended.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:90: void
UpdateLauncherItem();
On 2017/05/23 00:02:29, msw wrote:
> nit: comment (or inline in OnWindowPropertyChanged), rename
UpdateShelfItemIcon?

Done. Inlining is not very possible because it is called not only on prop change
but also when active window is changed (case multiple windows in one
controller).

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:695: AppIconLoader*
ChromeLauncherController::GetAppIconLoaderForApp(
On 2017/05/23 00:02:29, msw wrote:
> nit: reorder to match decl

Done.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:155: void
UpdateAppItemImage(const std::string& app_id);
On 2017/05/23 00:02:29, msw wrote:
> nit: rename UpdateLauncherItemImage for parity with above? (Or
> UpdateShelfItemImage to reduce old 'Launcher' name usage)

That is nice.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:64:
// icon = app_window->pp_icon();
On 2017/05/23 00:02:29, msw wrote:
> fix this

Done.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:257: const
gfx::Image& custom_image = app_window()->custom_app_icon();
On 2017/05/23 00:02:29, msw wrote:
> Should the resulting icon be cached, if this function is called a lot?

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.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:260:
app_window()->show_in_shelf()) {
On 2017/05/23 00:02:29, msw wrote:
> 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?

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.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:409:
app_icon_.reset();
On 2017/05/23 00:02:29, msw wrote:
> q: is this necessary right before an assignment?

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.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.h (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:17: class
ChromeAppIcon;
On 2017/05/23 00:02:29, msw wrote:
> nit: not needed (fwd decl in chrome_app_icon_delegate.h)

Done.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:79: void
EnsureAppIconCreated();
On 2017/05/23 00:02:30, msw wrote:
> nit: comment.

Done.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:97:
std::unique_ptr<extensions::ChromeAppIcon> app_icon_;
On 2017/05/23 00:02:29, msw wrote:
> nit: comment

Done.

https://codereview.chromium.org/2900783003/diff/20001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/2900783003/diff/20001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:511: // Custom icon shown in the task
bar or in shelf.
On 2017/05/23 00:02:30, msw wrote:
> nit: the shelf is chrome's 'task bar', so you can probably remove "or in
shelf",
> or change it to "or in the Chrome OS shelf."

Done.

[stevenjb, 2017-05-23 17:00:19]

I don't really have my head wrapped around this code these days, so mostly a
couple of questions just to make sure that we do not break
chrome.app.window.setIcon.

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:185:
ui::BaseWindow* last_active_window = GetLastActiveWindow();
Could you add a comment briefly explaining the logic here? What will the
behavior be if different windows have different icons, will the launcher icon
change when activation changes?

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(left):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:80:
AppWindow* app_window) {
I think this is the only actual use of OnAppWindowIconChanged? If so we should
remove it, but I am missing where / how exactly the icon is getting updated when
AppWindow::UpdateAppIcon gets called?

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:70:
icon = gfx::Image(*app_icon);
Maybe put the above in AppWindow as GetAppIcon() instead?

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

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:257: const
gfx::Image& custom_image = app_window()->custom_app_icon();
On 2017/05/23 16:11:44, khmel wrote:
> On 2017/05/23 00:02:29, msw wrote:
> > Should the resulting icon be cached, if this function is called a lot?
> 
> 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.

Good to know, maybe mention that in a comment?

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:260:
app_window()->show_in_shelf()) {
On 2017/05/23 16:11:44, khmel wrote:
> On 2017/05/23 00:02:29, msw wrote:
> > 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?
> 
> 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.

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.

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:409:
app_icon_.reset();
On 2017/05/23 16:11:44, khmel wrote:
> On 2017/05/23 00:02:29, msw wrote:
> > q: is this necessary right before an assignment?
> 
> 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. 

What you have is probably fine with an explanatory comment.

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:191:
set_image_set_by_controller(true);
q: can we get rid of image_set_by_controller? (maybe in a followup)

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

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:260:
app_window()->show_in_shelf()) {
On 2017/05/23 17:17:40, msw wrote:
> On 2017/05/23 16:11:44, khmel wrote:
> > On 2017/05/23 00:02:29, msw wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:409:
app_icon_.reset();
On 2017/05/23 17:17:40, msw wrote:
> On 2017/05/23 16:11:44, khmel wrote:
> > On 2017/05/23 00:02:29, msw wrote:
> > > q: is this necessary right before an assignment?
> > 
> > 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. 
> 
> What you have is probably fine with an explanatory comment.

Done.

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:185:
ui::BaseWindow* last_active_window = GetLastActiveWindow();
On 2017/05/23 17:00:19, stevenjb wrote:
> Could you add a comment briefly explaining the logic here? What will the
> behavior be if different windows have different icons, will the launcher icon
> change when activation changes?

Done.

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:191:
set_image_set_by_controller(true);
On 2017/05/23 17:17:40, msw wrote:
> q: can we get rid of image_set_by_controller? (maybe in a followup)

I think so. We still have a goal to get rid of AppIconLoader. Added todo.

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(left):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:80:
AppWindow* app_window) {
On 2017/05/23 17:00:19, stevenjb wrote:
> I think this is the only actual use of OnAppWindowIconChanged? If so we should
> remove it, but I am missing where / how exactly the icon is getting updated
when
> AppWindow::UpdateAppIcon gets called?

I tested legacy code and found that it did not work as expected. You may read
here in my big comment problem I found with async image loading and updating it
in shelf:
https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...

Now we have reliable mechanism to observe kAppIconKey change.

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:70:
icon = gfx::Image(*app_icon);
On 2017/05/23 17:00:19, stevenjb wrote:
> Maybe put the above in AppWindow as GetAppIcon() instead?

I am not very sure. Actually ChromeNativeAppWindowViews is WidgetDelegate, and
as a delegate it implements:
  gfx::ImageSkia GetWindowAppIcon() override;
  gfx::ImageSkia GetWindowIcon() override;

I don't feel that asking app window back is good idea. Let decide how app icon
looks in one place, here. WDYT?

[msw, 2017-05-23 18:13:13]

looking pretty good, one comment.

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:70:
icon = gfx::Image(*app_icon);
On 2017/05/23 17:35:00, khmel wrote:
> On 2017/05/23 17:00:19, stevenjb wrote:
> > Maybe put the above in AppWindow as GetAppIcon() instead?
> 
> I am not very sure. Actually ChromeNativeAppWindowViews is WidgetDelegate, and
> as a delegate it implements:
>   gfx::ImageSkia GetWindowAppIcon() override;
>   gfx::ImageSkia GetWindowIcon() override;
> 
> I don't feel that asking app window back is good idea. Let decide how app icon
> looks in one place, here. WDYT?

Hmm, it seems like this code should call
ChromeNativeAppWindowViews::GetWindowIcon, and fallback to
ChromeNativeAppWindowViews::GetWindowAppIcon if the former is null/empty. Can we
call those here, perhaps indirectly if needed (eg. via AppWindow)?

[stevenjb, 2017-05-23 18:19:23]

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(left):

https://codereview.chromium.org/2900783003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:80:
AppWindow* app_window) {
On 2017/05/23 17:35:00, khmel wrote:
> On 2017/05/23 17:00:19, stevenjb wrote:
> > I think this is the only actual use of OnAppWindowIconChanged? If so we
should
> > remove it, but I am missing where / how exactly the icon is getting updated
> when
> > AppWindow::UpdateAppIcon gets called?
> 
> I tested legacy code and found that it did not work as expected. You may read
> here in my big comment problem I found with async image loading and updating
it
> in shelf:
>
https://codereview.chromium.org/2900783003/diff/20001/chrome/browser/ui/views...
> 
> Now we have reliable mechanism to observe kAppIconKey change.

OK. I believe that there may have been bugs before, especially WRT async
loading.

I'm still a little fuzzy on what is replacing
AppWindowRegistry::Observer::OnAppWindowIconChanged, and why that isn't removed
since this is the only non test usage.

https://codereview.chromium.org/2900783003/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:188: //
such icon is available and non-empty set is as a shelf item icon. Otherwise
s/set is/set it/

[khmel, 2017-05-24 20:10:11]

Thanks Steven for pointing that OnAppWindowIconChanged is now deprecated.

I  discarded that code and refactored browser tests.
Well, I also found test flakiness :(. Further investigation showed that problem
happens when we set icon in window creation params like this:
chrome.app.window.create(	
  'main.html', { id: "win_with_icon",	
                 type: "shell",	
                 icon: "icon48.png"}

Digging deeper pointed that blink sometimes fails to load resource and this
causes download icon failure. That code is complicated, including resource
loader and outside of this CL.
So I changed test to update icon dynamically. Once blink problem is fixed I
added test coverage for icon in window creation params.

PTAL

https://codereview.chromium.org/2900783003/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:188: //
such icon is available and non-empty set is as a shelf item icon. Otherwise
On 2017/05/23 18:19:22, stevenjb wrote:
> s/set is/set it/

Done.

[Devlin, 2017-05-24 20:24:36]

Steven knows this code a lot better than I, so I'll wait 'til he's signed off.

[stevenjb, 2017-05-25 22:25:20]

I wish I could say I was more confident of my review than I am, but it's been
quite a while since I have been in this code. Hopefully msw@ will catch anything
I missed, but... thanks for taking this on and... lgtm!

https://codereview.chromium.org/2900783003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:276: } else {
nit: no else after return

[Devlin, 2017-05-31 22:00:01]

extensions lgtm

https://codereview.chromium.org/2900783003/diff/100001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_app_window_observer.h (right):

https://codereview.chromium.org/2900783003/diff/100001/chrome/browser/ui/test...
chrome/browser/ui/test/test_app_window_observer.h:43: int expeced_icon_updates_
= 0;
typo: expected

https://codereview.chromium.org/2900783003/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/app_icon/test.js (right):

https://codereview.chromium.org/2900783003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/app_icon/test.js:29: nonShelfWindow =
win;
two-space indentation in functions

https://codereview.chromium.org/2900783003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/app_icon/test.js:44:
shelfWindow.setIcon("icon32.png")
single-quotes in js

https://codereview.chromium.org/2900783003/diff/100001/extensions/common/mani...
File extensions/common/manifest_handlers/icons_handler.cc (right):

https://codereview.chromium.org/2900783003/diff/100001/extensions/common/mani...
extensions/common/manifest_handlers/icons_handler.cc:32: extension
I think I'd rather put the onus of checking |extension| on the callers.

[khmel, 2017-06-01 16:56:51]

khmel@chromium.org changed reviewers:
+ reveman@chromium.org

[khmel, 2017-06-01 16:56:52]

Thanks Devlin and Steven for review.

Adding reveman@

David, Mike,
PTAL

https://codereview.chromium.org/2900783003/diff/100001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_app_window_observer.h (right):

https://codereview.chromium.org/2900783003/diff/100001/chrome/browser/ui/test...
chrome/browser/ui/test/test_app_window_observer.h:43: int expeced_icon_updates_
= 0;
On 2017/05/31 22:00:01, Devlin wrote:
> typo: expected

Done.

https://codereview.chromium.org/2900783003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:276: } else {
On 2017/05/25 22:25:20, stevenjb wrote:
> nit: no else after return

Done.

https://codereview.chromium.org/2900783003/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/app_icon/test.js (right):

https://codereview.chromium.org/2900783003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/app_icon/test.js:29: nonShelfWindow =
win;
On 2017/05/31 22:00:01, Devlin wrote:
> two-space indentation in functions

Done.

https://codereview.chromium.org/2900783003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/app_icon/test.js:44:
shelfWindow.setIcon("icon32.png")
On 2017/05/31 22:00:01, Devlin wrote:
> single-quotes in js

Done.

https://codereview.chromium.org/2900783003/diff/100001/extensions/common/mani...
File extensions/common/manifest_handlers/icons_handler.cc (right):

https://codereview.chromium.org/2900783003/diff/100001/extensions/common/mani...
extensions/common/manifest_handlers/icons_handler.cc:32: extension
On 2017/05/31 22:00:01, Devlin wrote:
> I think I'd rather put the onus of checking |extension| on the callers.

Done.

[reveman, 2017-06-01 18:02:23]

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
File components/exo/shell_surface.h (right):

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
components/exo/shell_surface.h:158: static void SetAppIcon(aura::Window* window,
const gfx::ImageSkia& icon);
Why do we need shell surface API for this? Can we just set the icon properties
directly instead?
https://cs.chromium.org/chromium/src/ui/aura/client/aura_constants.cc?l=68

[khmel, 2017-06-01 18:08:20]

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
File components/exo/shell_surface.h (right):

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
components/exo/shell_surface.h:158: static void SetAppIcon(aura::Window* window,
const gfx::ImageSkia& icon);
On 2017/06/01 18:02:23, reveman wrote:
> Why do we need shell surface API for this? Can we just set the icon properties
> directly instead?
> https://cs.chromium.org/chromium/src/ui/aura/client/aura_constants.cc?l=68

Widget uses WidgetDelegate to resolve the icon (and title). Default delegate
impl returns empty icon. In this case what we set can be removed. If we want
this way we have to prohibit to calling UpdateWindowIcon.

[reveman, 2017-06-01 18:28:42]

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
components/exo/shell_surface.cc:45:
DECLARE_UI_CLASS_PROPERTY_TYPE(exo::ShellSurface*)
Don't add this. Launcher code can get the ShellSurface instance from:

static_cast<ShellSurface*>(widget->widget_delegate()->GetContentsView())

after verifying that ShellSurface::GetMainSurface is non-null.

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
File components/exo/shell_surface.h (right):

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
components/exo/shell_surface.h:157: // Set icon for the window.
// Set icon for the surface.

and the function should not be a static function that takes a window. It's up to
the caller to get a shell surface instance and call this properly.

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
components/exo/shell_surface.h:158: static void SetAppIcon(aura::Window* window,
const gfx::ImageSkia& icon);
On 2017/06/01 at 18:08:20, khmel wrote:
> On 2017/06/01 18:02:23, reveman wrote:
> > Why do we need shell surface API for this? Can we just set the icon
properties
> > directly instead?
> > https://cs.chromium.org/chromium/src/ui/aura/client/aura_constants.cc?l=68
> 
> Widget uses WidgetDelegate to resolve the icon (and title). Default delegate
impl returns empty icon. In this case what we set can be removed. If we want
this way we have to prohibit to calling UpdateWindowIcon.

Not sure this is called today for exo shell surface widgets but keeping an icon
member here in this class might still be the best approach so continuing review
of this patch.

[msw, 2017-06-01 19:47:29]

looks pretty good, mostly comment grammar nits.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:184: //
App window may optionally contain an attached icon we may use for the shelf
optional nit: consider replacing this comment with "Set the shelf item icon from
the kAppIconKey property of the current (or most recently) active window. If
there is no valid icon, ask ChromeLauncherController to update the icon."

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:69:
ChromeLauncherController* owner);
nit: just use ChromeLauncherController::instance() instead of injecting this
value. aside: Technically, CLC doesn't own ShelfItemDelegates, ShelfModel owns
them as ash::ShelfItemDelegates via ShelfModel::SetShelfItemDelegate.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:91: // and
sets the new controller icon based on currently active window.
nit: "the currently"

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:166: void
ArcAppDeferredLauncherController::UpdateApp(const std::string& app_id) {
optional nit: rename UpdateShelfItemIcon or UpdateLAuncherItemImage or similar.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:151: // Updates the
image for a specific shelf item image from app image loader.
nit: remove "image" after "shelf item", also "the app's icon loader":
"Updates the image for a specific shelf item from the app's icon loader."

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:382: // Resolves app
icon image loader for the app.
nit: "the app icon"

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_app_window_observer.h (right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/test...
chrome/browser/ui/test/test_app_window_observer.h:19: class
TestAppWindowObserver : public extensions::AppWindowRegistry::Observer,
nit: comment "A test helper that waits for AppWindow icon property updates."
optionally rename this class to TestAppWindowIconObserver?

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/test...
chrome/browser/ui/test/test_app_window_observer.h:41: content::BrowserContext*
const context_;
nit: add a blank line above

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:403: // To avoid
recursive call reset smart pointer. It will be checked at
nit: "calls, reset the" s/at/in/

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:404: //
OnIconUpdated to determine if this is real update or initial callback on
nit: "is a real", "or the initial"

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.h (right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:74: // Ensures
that Chrome app icon is created.
nit: "the Chrome"

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:92: // Contains
the default Chrome app icon. It is used in case custom icon for
nit: "the custom"

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:93: // extension
app window is not set or as a part of composite image.
"the extension", "set, or"

https://codereview.chromium.org/2900783003/diff/120001/extensions/browser/api...
File extensions/browser/api/app_window/app_window_apitest.cc (right):

https://codereview.chromium.org/2900783003/diff/120001/extensions/browser/api...
extensions/browser/api/app_window/app_window_apitest.cc:51: base::RunLoop
run_loop;
optional nit: base::RunLoop().RunUntilIdle(); and remove curlies {}

[khmel, 2017-06-01 21:55:47]

Thanks for your comments!

PTAL updated version.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:184: //
App window may optionally contain an attached icon we may use for the shelf
On 2017/06/01 19:47:28, msw wrote:
> optional nit: consider replacing this comment with "Set the shelf item icon
from
> the kAppIconKey property of the current (or most recently) active window. If
> there is no valid icon, ask ChromeLauncherController to update the icon."

Thanks!

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:69:
ChromeLauncherController* owner);
On 2017/06/01 19:47:28, msw wrote:
> nit: just use ChromeLauncherController::instance() instead of injecting this
> value. aside: Technically, CLC doesn't own ShelfItemDelegates, ShelfModel owns
> them as ash::ShelfItemDelegates via ShelfModel::SetShelfItemDelegate.

Good point

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:91: // and
sets the new controller icon based on currently active window.
On 2017/06/01 19:47:28, msw wrote:
> nit: "the currently"

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:166: void
ArcAppDeferredLauncherController::UpdateApp(const std::string& app_id) {
On 2017/06/01 19:47:28, msw wrote:
> optional nit: rename UpdateShelfItemIcon or UpdateLAuncherItemImage or
similar.

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:151: // Updates the
image for a specific shelf item image from app image loader.
On 2017/06/01 19:47:28, msw wrote:
> nit: remove "image" after "shelf item", also "the app's icon loader":
> "Updates the image for a specific shelf item from the app's icon loader."

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:382: // Resolves app
icon image loader for the app.
On 2017/06/01 19:47:28, msw wrote:
> nit: "the app icon"

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_app_window_observer.h (right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/test...
chrome/browser/ui/test/test_app_window_observer.h:19: class
TestAppWindowObserver : public extensions::AppWindowRegistry::Observer,
On 2017/06/01 19:47:28, msw wrote:
> nit: comment "A test helper that waits for AppWindow icon property updates."
> optionally rename this class to TestAppWindowIconObserver?

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/test...
chrome/browser/ui/test/test_app_window_observer.h:41: content::BrowserContext*
const context_;
On 2017/06/01 19:47:28, msw wrote:
> nit: add a blank line above

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:403: // To avoid
recursive call reset smart pointer. It will be checked at
On 2017/06/01 19:47:28, msw wrote:
> nit: "calls, reset the" s/at/in/

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:404: //
OnIconUpdated to determine if this is real update or initial callback on
On 2017/06/01 19:47:28, msw wrote:
> nit: "is a real", "or the initial"

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.h (right):

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:74: // Ensures
that Chrome app icon is created.
On 2017/06/01 19:47:29, msw wrote:
> nit: "the Chrome"

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:92: // Contains
the default Chrome app icon. It is used in case custom icon for
On 2017/06/01 19:47:28, msw wrote:
> nit: "the custom"

Done.

https://codereview.chromium.org/2900783003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:93: // extension
app window is not set or as a part of composite image.
On 2017/06/01 19:47:29, msw wrote:
> "the extension", "set, or"

Done.

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
components/exo/shell_surface.cc:45:
DECLARE_UI_CLASS_PROPERTY_TYPE(exo::ShellSurface*)
On 2017/06/01 18:28:42, reveman wrote:
> Don't add this. Launcher code can get the ShellSurface instance from:
> 
> static_cast<ShellSurface*>(widget->widget_delegate()->GetContentsView())
> 
> after verifying that ShellSurface::GetMainSurface is non-null.

Good point, thanks!

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
File components/exo/shell_surface.h (right):

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
components/exo/shell_surface.h:157: // Set icon for the window.
On 2017/06/01 18:28:42, reveman wrote:
> // Set icon for the surface.
> 
> and the function should not be a static function that takes a window. It's up
to
> the caller to get a shell surface instance and call this properly.

Done.

https://codereview.chromium.org/2900783003/diff/120001/components/exo/shell_s...
components/exo/shell_surface.h:158: static void SetAppIcon(aura::Window* window,
const gfx::ImageSkia& icon);
On 2017/06/01 18:28:42, reveman wrote:
> On 2017/06/01 at 18:08:20, khmel wrote:
> > On 2017/06/01 18:02:23, reveman wrote:
> > > Why do we need shell surface API for this? Can we just set the icon
> properties
> > > directly instead?
> > > https://cs.chromium.org/chromium/src/ui/aura/client/aura_constants.cc?l=68
> > 
> > Widget uses WidgetDelegate to resolve the icon (and title). Default delegate
> impl returns empty icon. In this case what we set can be removed. If we want
> this way we have to prohibit to calling UpdateWindowIcon.
> 
> Not sure this is called today for exo shell surface widgets but keeping an
icon
> member here in this class might still be the best approach so continuing
review
> of this patch.

Acknowledged.

https://codereview.chromium.org/2900783003/diff/120001/extensions/browser/api...
File extensions/browser/api/app_window/app_window_apitest.cc (right):

https://codereview.chromium.org/2900783003/diff/120001/extensions/browser/api...
extensions/browser/api/app_window/app_window_apitest.cc:51: base::RunLoop
run_loop;
On 2017/06/01 19:47:29, msw wrote:
> optional nit: base::RunLoop().RunUntilIdle(); and remove curlies {}

Done.

[reveman, 2017-06-01 22:05:36]

components/exo lgtm with nits

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
components/exo/shell_surface.cc:536: widget_->UpdateWindowIcon();
nit: please leave this up to the caller so icon can be set before we have a
widget and unnecessary updates can be avoided

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
File components/exo/shell_surface.h (right):

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
components/exo/shell_surface.h:158: void SetAppIcon(const gfx::ImageSkia& icon);
nit: SetIcon or SetApplicationIcon as we're not using the App abbreviation
above. I prefer SetIcon as it's consistent with SetTitle.

[khmel, 2017-06-01 22:19:28]

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
components/exo/shell_surface.cc:536: widget_->UpdateWindowIcon();
On 2017/06/01 22:05:36, reveman wrote:
> nit: please leave this up to the caller so icon can be set before we have a
> widget and unnecessary updates can be avoided

I did it the same way as SetTitle

  if (widget_)
    widget_->UpdateWindowIcon();

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
File components/exo/shell_surface.h (right):

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
components/exo/shell_surface.h:158: void SetAppIcon(const gfx::ImageSkia& icon);
On 2017/06/01 22:05:36, reveman wrote:
> nit: SetIcon or SetApplicationIcon as we're not using the App abbreviation
> above. I prefer SetIcon as it's consistent with SetTitle.

Done.

[reveman, 2017-06-01 22:26:00]

lgtm

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2900783003/diff/140001/components/exo/shell_s...
components/exo/shell_surface.cc:536: widget_->UpdateWindowIcon();
On 2017/06/01 at 22:19:27, khmel wrote:
> On 2017/06/01 22:05:36, reveman wrote:
> > nit: please leave this up to the caller so icon can be set before we have a
> > widget and unnecessary updates can be avoided
> 
> I did it the same way as SetTitle
> 
>   if (widget_)
>     widget_->UpdateWindowIcon();

Sounds good. Thanks!

[msw, 2017-06-01 23:48:31]

lgtm with a few more/repeated comment nits.

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:144:
// Register the window after a shelf item is created to let controller set
nit: "let the controller"

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:145:
// the shelf item properly.
nit: did you mean "set the shelf icon property"? otherwise, please clarify this
what you mean in this comment.

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:403: // To avoid
recursive call reset smart pointer. It will be checked in
nit: "calls, reset the"

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:404: //
OnIconUpdated to determine if this is real update or the initial callback
nit: "is a real" (or clarify what a 'real' update means)

[khmel, 2017-06-02 01:52:36]

Thank you for review!

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:144:
// Register the window after a shelf item is created to let controller set
On 2017/06/01 23:48:30, msw wrote:
> nit: "let the controller"

Done.

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:145:
// the shelf item properly.
On 2017/06/01 23:48:30, msw wrote:
> nit: did you mean "set the shelf icon property"? otherwise, please clarify
this
> what you mean in this comment.

 icon property sounds better, thanks!

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:403: // To avoid
recursive call reset smart pointer. It will be checked in
On 2017/06/01 23:48:31, msw wrote:
> nit: "calls, reset the"

Done.

https://codereview.chromium.org/2900783003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:404: //
OnIconUpdated to determine if this is real update or the initial callback
On 2017/06/01 23:48:30, msw wrote:
> nit: "is a real" (or clarify what a 'real' update means)

Done.

[khmel, 2017-06-02 01:53:01]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-06-02 01:53:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, stevenjb@chromium.org, msw@chromium.org,
reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2900783003/#ps180001
(title: "rebase + comments update")

[commit-bot: I haz the power, 2017-06-02 01:53:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 02:14:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 02:14:02]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[khmel, 2017-06-02 15:48:39]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-06-02 15:48:40]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
reveman@chromium.org, rdevlin.cronin@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2900783003/#ps200001
(title: "support unit_tests")

[commit-bot: I haz the power, 2017-06-02 15:49:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 16:12:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 16:12:21]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[khmel, 2017-06-02 17:05:00]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-06-02 17:05:01]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
reveman@chromium.org, rdevlin.cronin@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2900783003/#ps220001
(title: "fix mac compile")

[commit-bot: I haz the power, 2017-06-02 17:05:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 19:47:12]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496423100663430,
"parent_rev": "2537fa873c33c1c2597b2f03d7a15d7c88d1da3e", "commit_rev":
"1ddcada88969fcf84e51511060588094ddbd54c8"}

[commit-bot: I haz the power, 2017-06-02 19:47:28]

Description was changed from

==========
Handle app custom icon via aura::Window property.

This does refactoring of custom icon functionality for Chrome extension
app and for ARC app by unifying applying a custom icon. Before each app
class has own logic in window controller that applies the custom icon
for the shelf item. With this new CL custom icon is merged to concept
of window app icon property which is set by widget delegate. Base
AppWindowLauncherController observes changes of native window related
to app icon and applies the new window app icon as shelf icon.
This CL also moves loading of Chrome App icon from extensions
sub-project to chrome/browser (AppWindow->ChromeNativeAppWindowViews).
ChromeNativeAppWindowViews is only one consumer of loaded App icon.
From other side, chrome/browser has reach functionality to handle
Chrome app icons, including ChromeAppIcon.
With this CL ExtensionAppLauncher is de-facto deprecated in
ChromeLauncherController (to be removed in next CLs).

BUG=724292
TEST=Tests + Manually on device, including test apps to validate custom
     icons for Extension and ARC based apps.
==========

to

==========
Handle app custom icon via aura::Window property.

This does refactoring of custom icon functionality for Chrome extension
app and for ARC app by unifying applying a custom icon. Before each app
class has own logic in window controller that applies the custom icon
for the shelf item. With this new CL custom icon is merged to concept
of window app icon property which is set by widget delegate. Base
AppWindowLauncherController observes changes of native window related
to app icon and applies the new window app icon as shelf icon.
This CL also moves loading of Chrome App icon from extensions
sub-project to chrome/browser (AppWindow->ChromeNativeAppWindowViews).
ChromeNativeAppWindowViews is only one consumer of loaded App icon.
From other side, chrome/browser has reach functionality to handle
Chrome app icons, including ChromeAppIcon.
With this CL ExtensionAppLauncher is de-facto deprecated in
ChromeLauncherController (to be removed in next CLs).

BUG=724292
TEST=Tests + Manually on device, including test apps to validate custom
     icons for Extension and ARC based apps.

Review-Url: https://codereview.chromium.org/2900783003
Cr-Commit-Position: refs/heads/master@{#476761}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ddcada88969fcf84e5151106058...
==========

[commit-bot: I haz the power, 2017-06-02 19:47:30]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/1ddcada88969fcf84e5151106058...