chromeos: Fix pixelated icons in app list and launcher (part 3)

chromeos: Fix pixelated icons in app list and launcher (part 3)

- Use extensions::IconImage for HiDPI icons;
- Fix IconImage use default icon for bundled component extension icon;
- Fix ImageLoadingTracker does not properly resize bundled component extension
  icon;
- Get rid of icon cache and work thread for shadow generation. We are using
  the exact icon size (smaller) and only generate shadow for visible ones.
  Thus, it is no longer needed.

BUG=131738, 131739
TEST=Verify launcher and app list have crispy icons on HiDPI.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155939

[xiyuan, 2012-08-21 22:36:19]

Looks like this CL could be related to tony's
https://chromiumcodereview.appspot.com/10861034/, regarding how we handle the
load failure case.

[Aaron Boodman, 2012-08-22 21:23:04]

Can you please say which reviewer should review which bit?

[xiyuan, 2012-08-22 22:22:36]

On 2012/08/22 21:23:04, Aaron Boodman wrote:
> Can you please say which reviewer should review which bit?

How about this:
aa for chrome/browser/extensions/*
pkotwicz and tbarzic for rest of the files + extension_icon_image.*

Temporarily take sky out until we have all agreed on the CL.

[tbarzic, 2012-08-25 01:46:39]

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:185: scoped_ptr<SkBitmap>
bitmap(new SkBitmap);
now I wonder why I didn't do this in the first place :)

http://codereview.chromium.org/10868003/diff/3001/ui/app_list/app_list_item_v...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10868003/diff/3001/ui/app_list/app_list_item_v...
ui/app_list/app_list_item_view.cc:110: // generation does not run on UI thread.
I'm a bit concerned about this part..
I don't think we will ever do drop shadow transformation on FILE thread (we'll
get here only when we get first icon representation, which will probably be a
temp one that we use until the real one is loaded).
well, maybe not never, but that depends on a race between this operation and
image loading :)

[xiyuan, 2012-08-28 17:31:21]

http://codereview.chromium.org/10868003/diff/3001/ui/app_list/app_list_item_v...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10868003/diff/3001/ui/app_list/app_list_item_v...
ui/app_list/app_list_item_view.cc:110: // generation does not run on UI thread.
As we have discussed, the icon cache is keyed by the actual pixels of the image.
So when the real icon is loaded, it has a different key compared with the
default image and we will schedule another job to run this code to generate
shadow for the real icon.

[Aaron Boodman, 2012-08-30 06:09:19]

lgtm

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image.cc (right):

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.cc:119: if
(!ImageLoadingTracker::IsSpecialBundledExtensionId(extension_->id())) {
I hate this hack so much. It's probably a different project, but it would be
good if we could pull knowledge of component extensions out of this class.

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image.h (right):

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.h:58: // Sets given |rep| to
|image_skia_|. This allows observer to set an
Reader of documentation should not need to understand internal details like
|image_skia_|. Is there another way to describe what this does without
referencing internal details?

[pkotwicz, 2012-08-30 16:04:53]

I am unsure if you have. Have you merged with tbarzic's changes in
http://codereview.chromium.org/10861034/ ?

[xiyuan, 2012-08-30 17:01:16]

On 2012/08/30 16:04:53, pkotwicz wrote:
> I am unsure if you have. Have you merged with tbarzic's changes in
> http://codereview.chromium.org/10861034/ ?

I have not merged with Tony's CL and will wait it to land. The merge would be to
get rid of the load failure fallback logic here and use Tony's CL. The rest
would remain mostly the same.

aa, could you take a look at http://codereview.chromium.org/10861034/ as well?
Thanks.

[tbarzic, 2012-08-31 18:57:34]

lgtm

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc (right):

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc:88:
extensions::Extension::GetDefaultIcon(true),
this will return SkBitmap, right?
we should probably change GetDefaultIcon to return ImageSkia instead.

[xiyuan, 2012-08-31 19:24:45]

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc (right):

http://codereview.chromium.org/10868003/diff/3001/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc:88:
extensions::Extension::GetDefaultIcon(true),
On 2012/08/31 18:57:34, tbarzic wrote:
> this will return SkBitmap, right?
> we should probably change GetDefaultIcon to return ImageSkia instead.

Yep, this part will be removed after your default icon CL lands.

[xiyuan, 2012-09-07 16:41:52]

CL rebased on Tony's change. Please take another look. Thanks.

Suggested split:
pkotwicz, tbarzic: extensions::IconImage and how it's used in ExtensionAppItem
                   and LaucherAppIconLoader
aa: files under extensions/... (mostly unchanged since your last review)
+sky: ash/... and ui/app_list/... as I get rid of the icon cache and shadow
worker thread.

The icon cache and shadow work thread seems no longer needed because we use
exact icon size and only generate shadows for visible icons. Before the
change, we generate shadow for all icons during init. 128x128 takes 6ms per icon
and if you have 50 icons, it took 300ms for UI to finish init and show up.
Now shadows are triggered from paint (thanks for the ImageSkia infrastructure)
so we only generate for 16, and 48x48 (1x) takes 1.1ms per icon and 96x96 (2x)
takes 3ms per icon. Overall shadow generation took 17.6ms (1x) and 48ms (2x) and
that seems no longer an issue.

[tbarzic, 2012-09-07 17:13:23]

On 2012/09/07 16:41:52, xiyuan wrote:
> CL rebased on Tony's change. Please take another look. Thanks.
> 
> Suggested split:
> pkotwicz, tbarzic: extensions::IconImage and how it's used in ExtensionAppItem
>                    and LaucherAppIconLoader
> aa: files under extensions/... (mostly unchanged since your last review)
> +sky: ash/... and ui/app_list/... as I get rid of the icon cache and shadow
> worker thread.
> 
> The icon cache and shadow work thread seems no longer needed because we use
> exact icon size and only generate shadows for visible icons. Before the
> change, we generate shadow for all icons during init. 128x128 takes 6ms per
icon
> and if you have 50 icons, it took 300ms for UI to finish init and show up.
> Now shadows are triggered from paint (thanks for the ImageSkia infrastructure)
> so we only generate for 16, and 48x48 (1x) takes 1.1ms per icon and 96x96 (2x)
> takes 3ms per icon. Overall shadow generation took 17.6ms (1x) and 48ms (2x)
and
> that seems no longer an issue.

lgtm

[tbarzic, 2012-09-07 17:14:06]

forgot to publish nits :)

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/extension_app_item.cc:148:
OnExtensionIconImageChanged(icon_.get());
optional:
I think SetIcon(icon_->image_skia()) would make clearer what you're trying to
do.

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc (right):

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc:55: // |map_| takes
ownership if |image|.
s/if/of

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc:58:
OnExtensionIconImageChanged(image);
optional:
host_->SetAppImage(id, image->image_skia());

[pkotwicz, 2012-09-07 19:24:49]

LGTM with nits

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/extensions/...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/extensions/...
chrome/browser/extensions/image_loading_tracker.cc:185: scoped_ptr<SkBitmap>
bitmap(new SkBitmap);
Optional: You can probably use SkBitmap instead of SkBitmap* in all of these
functions. But I am fine if you keep it the way it is now.

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/extensions/...
chrome/browser/extensions/image_loading_tracker.cc:187:
resource_id).ToSkBitmap();
Nit: Use AsBitmap() instead of GetImageNamed()

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/extension_app_item.cc:145:
extension_misc::EXTENSION_ICON_MEDIUM,
Just to confirm, you are changing the size of the fetched resource to decrease
the amount of time it takes to generate shadows?

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc (right):

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc:77: 
Optional: I think you can delete i->first and erase it from the map here.
ImageSkia is refcounted so using the ImageSkia should still be valid after this
function has terminated.

[xiyuan, 2012-09-07 20:05:58]

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/extensions/...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/extensions/...
chrome/browser/extensions/image_loading_tracker.cc:185: scoped_ptr<SkBitmap>
bitmap(new SkBitmap);
On 2012/09/07 19:24:49, pkotwicz wrote:
> Optional: You can probably use SkBitmap instead of SkBitmap* in all of these
> functions. But I am fine if you keep it the way it is now.

Added a TODO and I'll leave that to a dedicated clean up CL.

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/extensions/...
chrome/browser/extensions/image_loading_tracker.cc:187:
resource_id).ToSkBitmap();
On 2012/09/07 19:24:49, pkotwicz wrote:
> Nit: Use AsBitmap() instead of GetImageNamed()

Done.

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/extension_app_item.cc:145:
extension_misc::EXTENSION_ICON_MEDIUM,
On 2012/09/07 19:24:49, pkotwicz wrote:
> Just to confirm, you are changing the size of the fetched resource to decrease
> the amount of time it takes to generate shadows?

This is actually the right size to use. We show 48x48 (aka
EXTENSION_ICON_MEDIUM) in current app list UI. Previously using 128x128 is
because the old UI spec asks for that and later use it to feed 2x UI where we
need 96x96.

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/extension_app_item.cc:148:
OnExtensionIconImageChanged(icon_.get());
On 2012/09/07 17:14:07, tbarzic wrote:
> optional:
> I think SetIcon(icon_->image_skia()) would make clearer what you're trying to
> do.

Done.

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc (right):

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc:55: // |map_| takes
ownership if |image|.
On 2012/09/07 17:14:07, tbarzic wrote:
> s/if/of

Done.

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc:58:
OnExtensionIconImageChanged(image);
On 2012/09/07 17:14:07, tbarzic wrote:
> optional:
> host_->SetAppImage(id, image->image_skia());

Done.

http://codereview.chromium.org/10868003/diff/14002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_app_icon_loader.cc:77: 
On 2012/09/07 19:24:49, pkotwicz wrote:
> Optional: I think you can delete i->first and erase it from the map here.
> ImageSkia is refcounted so using the ImageSkia should still be valid after
this
> function has terminated.

We need to keep IconImage alive so that it could load additional reps when
needed (e.g. when switching monitors).

[sky, 2012-09-10 21:10:03]

LGTM

http://codereview.chromium.org/10868003/diff/16002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

http://codereview.chromium.org/10868003/diff/16002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:96: // Clears the
image for specified id.
the specified id

[commit-bot: I haz the power, 2012-09-10 21:59:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10868003/22004

[commit-bot: I haz the power, 2012-09-10 23:24:43]

Try job failure for 10868003-22004 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-11 01:13:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10868003/23003

[commit-bot: I haz the power, 2012-09-11 03:38:14]

Change committed as 155939