Note: I'm using a fizz with an external 30" monitor.

@nverne is this something you're able to look at for M69?

No. I'm out for the next two weeks. Reassigning.

In case we need the asset again, it's located here
https://drive.google.com/open?id=19GbhNCcnmeWAM9Sbup7JKmtM0kEVxs1o&authuser=sgabriel@google.com

why RVG ?  the asset is public, including having been committed to the public hterm source repo.

Looks like this only affects the icon when the terminal is running. I think what's happening is AppWindowLauncherItemController::UpdateShelfItemIcon sets the shelf icon to the icon on the aura window, which is set by Browser::GetCurrentPageIcon to the favicon, which is a 16x16 image.

+oshima/khmel for thoughts.

So I can't seem to get the crosh favicon to not be 16x16 (e.g. adding 32/48/128 sizes to the manifest), possibly due to being a v1 app. However for this case it would be preferable to just to have the ChromeLauncherController determine the icon normally (see AppWindowLauncherItemController::UpdateShelfItemIcon() for context) instead of the icon set on the aura window. It seems weird to me that we would use different sources for an app in pinned state and non-pinned state. Is there some reason for the difference here and can we make these use the same icon?

I'm looking into this now.

Yes, this is because it's v1 app. Pinned state has no app runnig, in normal scenario, pinned state uses cached image, but crostini terminal uses
hardcoded one and that's why this is different.

In any case, I'll update so that i'll force to use the one provided.
Ideally, if it's v1 app, the icon should be on the extension side. That way,
you don't have to update two places if we ever want to change the icon.

the icon is specified in the extension manifest already ...

V1 apps (re-)uses the window icon, which is designed to be shown at the corner of
the window frame. I think Chrome shrink the icon so that it fits to the frame size (or favicon), then shelf scales it up and that's why it looks blurry.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27e7096754d53abeaabd5ec509a9efbccb3c4290

commit 27e7096754d53abeaabd5ec509a9efbccb3c4290
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Thu Aug 30 01:25:24 2018

Add a way to override a window/app icon in BrowserView

aura::client::kWindowIconKey is used to set the image created by GetWindowIcon:

https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?rcl=e034d1631b4e9a5348403b2ab910caaa9794b7e0&l=130

Using this for trusted extension is a bit riskly because it will make it impossible
to update the icon once its set.

Instead, this CL adds an explicit way to override these icons for special cases
like Crostini Terminal/Settings.

This is used by Settings and Crostini Terminal extension.

Bug:  869388 
Test: manual. settings/crostini termina icons are not blurry.
Change-Id: Ie557a6b8c5b3fb37412c7970070a44ae9685bf85
Reviewed-on: https://chromium-review.googlesource.com/1192122
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587367}
[modify] https://crrev.com/27e7096754d53abeaabd5ec509a9efbccb3c4290/chrome/browser/chromeos/crostini/crostini_util.cc
[modify] https://crrev.com/27e7096754d53abeaabd5ec509a9efbccb3c4290/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/27e7096754d53abeaabd5ec509a9efbccb3c4290/chrome/browser/ui/ash/launcher/settings_window_observer.cc
[add] https://crrev.com/27e7096754d53abeaabd5ec509a9efbccb3c4290/chrome/browser/ui/ash/window_properties.cc
[add] https://crrev.com/27e7096754d53abeaabd5ec509a9efbccb3c4290/chrome/browser/ui/ash/window_properties.h
[modify] https://crrev.com/27e7096754d53abeaabd5ec509a9efbccb3c4290/chrome/browser/ui/settings_window_manager_chromeos.cc
[modify] https://crrev.com/27e7096754d53abeaabd5ec509a9efbccb3c4290/chrome/browser/ui/views/frame/browser_view.cc

[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.

i don't think we need to bother with a merge here.  just let it be resolved in M70.

agreed

How difficult would this be to merge? The Terminal icon is a very visible part of Crostini, and it's launching to stable with M69. Showing new users a blurry icon won't leave a great impression.

This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Has this been verified on ToT? Looks good?

I tested on 70.0.3538.0/11041.0.0 eve

Deleted: Screenshot 2018-09-06 at 12.41.55 PM.png 1.2 MB

	Screenshot 2018-09-06 at 12.41.55 PM.png 1.2 MB View Download

Merge approved, M69.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44c82cfbda5a6903c2f54233a63ebd7e6f78b67c

commit 44c82cfbda5a6903c2f54233a63ebd7e6f78b67c
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Fri Sep 07 22:04:38 2018

Add a way to override a window/app icon in BrowserView

aura::client::kWindowIconKey is used to set the image created by GetWindowIcon:

https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?rcl=e034d1631b4e9a5348403b2ab910caaa9794b7e0&l=130

Using this for trusted extension is a bit riskly because it will make it impossible
to update the icon once its set.

Instead, this CL adds an explicit way to override these icons for special cases
like Crostini Terminal/Settings.

This is used by Settings and Crostini Terminal extension.

Bug:  869388 
Test: manual. settings/crostini termina icons are not blurry.
Change-Id: Ieac4283d89f2fc89c4a2ceec193fd983ca4cf9e8
Reviewed-on: https://chromium-review.googlesource.com/1204113
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#908}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/44c82cfbda5a6903c2f54233a63ebd7e6f78b67c/chrome/browser/chromeos/crostini/crostini_util.cc
[modify] https://crrev.com/44c82cfbda5a6903c2f54233a63ebd7e6f78b67c/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/44c82cfbda5a6903c2f54233a63ebd7e6f78b67c/chrome/browser/ui/ash/launcher/settings_window_observer.cc
[add] https://crrev.com/44c82cfbda5a6903c2f54233a63ebd7e6f78b67c/chrome/browser/ui/ash/window_properties.cc
[add] https://crrev.com/44c82cfbda5a6903c2f54233a63ebd7e6f78b67c/chrome/browser/ui/ash/window_properties.h
[modify] https://crrev.com/44c82cfbda5a6903c2f54233a63ebd7e6f78b67c/chrome/browser/ui/settings_window_manager_chromeos.cc
[modify] https://crrev.com/44c82cfbda5a6903c2f54233a63ebd7e6f78b67c/chrome/browser/ui/views/frame/browser_view.cc

I just started having this issue after the update to 71.0.3578.27 today.  I had the PCManFM icon go funky a few days ago, but never the terminal icon like now.

Should I start a new issue, or would you like to re-open this one?

file a new bug please.  this was fixed in R69.