arc: Restore Chrome badging for apps that have peer in Android apps.

arc: Restore Chrome badging for apps that have peer in Android apps.

This fix issue when running app for such apps does not have Chrome
badging. This CL does not apply AppWindow icon to shelf item if window
does not override its default icon. AppWindow might have its own icon
(sine M54), however if it is not set then app icon is used. From other
side shelf launcher controller has extra logic by applying additional
badging over the icon. 

TEST=Manually
BUG=708830
BUG=b/35258599

Review-Url: https://codereview.chromium.org/2804623004
Cr-Commit-Position: refs/heads/master@{#462661}
Committed: https://chromium.googlesource.com/chromium/src/+/3d8e30ac4d15e9f85e57423bc38eaafd762dd821

[khmel, 2017-04-06 01:49:24]

The CQ bit was checked by khmel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 01:49:49]

Dry run: 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-04-06 01:54:06]

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

[commit-bot: I haz the power, 2017-04-06 01:54:07]

Dry run: 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-04-06 01:55:04]

The CQ bit was checked by khmel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 01:55:33]

Dry run: 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-04-06 03:01:14]

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

[commit-bot: I haz the power, 2017-04-06 03:01:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[khmel, 2017-04-06 19:04:26]

Description was changed from

==========
arc: Restore Chrome badging for apps that have peer in Android apps.

This fix issue when running app for such apps does not have Chrome
badging. This CL does not apply AppWindow to shelf item if window
does not override its default icon.

TEST=Manually
BUG=708830
BUG=b/35258599
==========

to

==========
arc: Restore Chrome badging for apps that have peer in Android apps.

This fix issue when running app for such apps does not have Chrome
badging. This CL does not apply AppWindow icon to shelf item if window
does not override its default icon. AppWindow might have its own icon
(sine M54), however if it is not set then app icon is used. From other
side shelf launcher controller has extra logic by applying additional
badging over the icon. 

TEST=Manually
BUG=708830
BUG=b/35258599
==========

[khmel, 2017-04-06 19:08:59]

khmel@chromium.org changed reviewers:
+ msw@chromium.org, rockot@chromium.org, xiyuan@chromium.org

[khmel, 2017-04-06 19:09:00]

Hi,

PTAL,

Thanks!

https://codereview.chromium.org/2804623004/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/2804623004/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:598: if (window_icon_url_.is_valid()
&& app_icon_image_ &&
this fixes crash when icon is set but not showInShelf

[khmel, 2017-04-06 19:10:36]

khmel@chromium.org changed reviewers:
+ reillyg@chromium.org
- rockot@chromium.org

[khmel, 2017-04-06 19:10:37]

Did not realize that rockot@ is OOO.

reillyg@, PTAL

Thanks!

[msw, 2017-04-06 19:22:30]

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

https://codereview.chromium.org/2804623004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:98:
if (!app_window->window_icon_url().is_valid())
Should this also bail if app_window->app_icon().IsEmpty()?

https://codereview.chromium.org/2804623004/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_icon/test.js (right):

https://codereview.chromium.org/2804623004/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_icon/test.js:23: icon:
"icon48.png",
q: what's the difference between |win.setIcon("...")" and using this |icon:
"..."| init?

[khmel, 2017-04-06 20:10:40]

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

https://codereview.chromium.org/2804623004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:98:
if (!app_window->window_icon_url().is_valid())
On 2017/04/06 19:22:29, msw wrote:
> Should this also bail if app_window->app_icon().IsEmpty()?

makes sense, thanks

https://codereview.chromium.org/2804623004/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_icon/test.js (right):

https://codereview.chromium.org/2804623004/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_icon/test.js:23: icon:
"icon48.png",
On 2017/04/06 19:22:29, msw wrote:
> q: what's the difference between |win.setIcon("...")" and using this |icon:
> "..."| init?

It is equivalent, can be used after creation. Also realized that checking
window_icon_url_ is not enough we also should check app_icon_url_. Introduced
HasCustomIcon.

[msw, 2017-04-06 20:23:34]

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

https://codereview.chromium.org/2804623004/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:98:
if (!app_window->HasCustomIcon() || app_window->app_icon().IsEmpty())
Should HasCustomIcon also check !app_window->app_icon().IsEmpty()?

https://codereview.chromium.org/2804623004/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:193:
if (!app_icon.IsEmpty()) {
This check is now redundant with the one on line 191.

[Reilly Grant (use Gerrit), 2017-04-06 20:32:21]

//extensions lgtm

[khmel, 2017-04-06 20:39:15]

Thank you Reilly for review!

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

https://codereview.chromium.org/2804623004/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:98:
if (!app_window->HasCustomIcon() || app_window->app_icon().IsEmpty())
On 2017/04/06 20:23:34, msw wrote:
> Should HasCustomIcon also check !app_window->app_icon().IsEmpty()?

Not sure, that means custom icon is set but might be not loaded yet. I added
comment. Actually I don't have very strong opinion what is better so if you see
that is better I can change the meaning.

https://codereview.chromium.org/2804623004/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:193:
if (!app_icon.IsEmpty()) {
On 2017/04/06 20:23:34, msw wrote:
> This check is now redundant with the one on line 191.

Done.

[msw, 2017-04-06 20:48:23]

lgtm

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

https://codereview.chromium.org/2804623004/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:98:
if (!app_window->HasCustomIcon() || app_window->app_icon().IsEmpty())
On 2017/04/06 20:39:15, khmel wrote:
> On 2017/04/06 20:23:34, msw wrote:
> > Should HasCustomIcon also check !app_window->app_icon().IsEmpty()?
> 
> Not sure, that means custom icon is set but might be not loaded yet. I added
> comment. Actually I don't have very strong opinion what is better so if you
see
> that is better I can change the meaning.

What you have is fine for now.

[khmel, 2017-04-06 20:51:21]

The CQ bit was checked by khmel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 20:52:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[khmel, 2017-04-06 22:08:45]

Patchset #6 (id:100001) has been deleted

[xiyuan, 2017-04-06 22:12:43]

lgtm

[khmel, 2017-04-06 22:13:11]

Thank you for review!

[khmel, 2017-04-06 22:13:11]

Thank you for review!

[khmel, 2017-04-06 22:13:16]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-04-06 22:13:17]

The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2804623004/#ps80001
(title: "minor updates")

[commit-bot: I haz the power, 2017-04-06 22:14:48]

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-04-06 22:45:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491516796787380,
"parent_rev": "b8456f4c1ab4122fc65d85b973a39637b9ad2146", "commit_rev":
"3d8e30ac4d15e9f85e57423bc38eaafd762dd821"}

[commit-bot: I haz the power, 2017-04-06 22:46:21]

Description was changed from

==========
arc: Restore Chrome badging for apps that have peer in Android apps.

This fix issue when running app for such apps does not have Chrome
badging. This CL does not apply AppWindow icon to shelf item if window
does not override its default icon. AppWindow might have its own icon
(sine M54), however if it is not set then app icon is used. From other
side shelf launcher controller has extra logic by applying additional
badging over the icon. 

TEST=Manually
BUG=708830
BUG=b/35258599
==========

to

==========
arc: Restore Chrome badging for apps that have peer in Android apps.

This fix issue when running app for such apps does not have Chrome
badging. This CL does not apply AppWindow icon to shelf item if window
does not override its default icon. AppWindow might have its own icon
(sine M54), however if it is not set then app icon is used. From other
side shelf launcher controller has extra logic by applying additional
badging over the icon. 

TEST=Manually
BUG=708830
BUG=b/35258599

Review-Url: https://codereview.chromium.org/2804623004
Cr-Commit-Position: refs/heads/master@{#462661}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d8e30ac4d15e9f85e57423bc38e...
==========

[commit-bot: I haz the power, 2017-04-06 22:46:22]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3d8e30ac4d15e9f85e57423bc38e...