Load the actual app icon in Athena.

Load the actual app icon in Athena.

This CL depends on: https://codereview.chromium.org/334053003/

BUG=380421
R=oshima@chromium.org, jamescook@chromium.org
TBR=sheib@chromium.org
TEST=manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278825

[oshima, 2014-06-19 23:06:22]

https://codereview.chromium.org/345773004/diff/1/apps/shell/common/shell_exte...
File apps/shell/common/shell_extensions_client.cc (right):

https://codereview.chromium.org/345773004/diff/1/apps/shell/common/shell_exte...
apps/shell/common/shell_extensions_client.cc:110: (new
extensions::IconsHandler)->Register();
I'll leave this to james

https://codereview.chromium.org/345773004/diff/1/athena/content/content_app_m...
File athena/content/content_app_model_builder.cc (right):

https://codereview.chromium.org/345773004/diff/1/athena/content/content_app_m...
athena/content/content_app_model_builder.cc:69: public
extensions::IconImage::Observer {
is this observer used?

[Jun Mukai, 2014-06-19 23:08:32]

https://codereview.chromium.org/345773004/diff/1/athena/content/content_app_m...
File athena/content/content_app_model_builder.cc (right):

https://codereview.chromium.org/345773004/diff/1/athena/content/content_app_m...
athena/content/content_app_model_builder.cc:69: public
extensions::IconImage::Observer {
On 2014/06/19 23:06:22, oshima wrote:
> is this observer used?

oops, removed. thanks for catching this.

[oshima, 2014-06-19 23:20:09]

lgtm if jamescook approves it.

[James Cook, 2014-06-20 14:48:45]

https://codereview.chromium.org/345773004/diff/20001/apps/shell/common/shell_...
File apps/shell/common/shell_extensions_client.cc (right):

https://codereview.chromium.org/345773004/diff/20001/apps/shell/common/shell_...
apps/shell/common/shell_extensions_client.cc:110: (new
extensions::IconsHandler)->Register();
Can this go in extensions/common/common_manifest_handlers.cc ?

If so it should also be removed from 
chrome/common/extensions/chrome_extensions_client.cc.

https://codereview.chromium.org/345773004/diff/20001/athena/content/content_a...
File athena/content/content_app_model_builder.cc (right):

https://codereview.chromium.org/345773004/diff/20001/athena/content/content_a...
athena/content/content_app_model_builder.cc:79: // TODO(mukai): better default
icon
Not for this CL, but there is an IDR_APP_DEFAULT_ICON you could use.
Unfortunately it lives in chrome/app/theme/theme_resources.grd so it would
require more refactoring.  Or you could copy it for Athena -- it might be OK for
Athena to have a different default icon.

[Jun Mukai, 2014-06-20 16:33:31]

https://codereview.chromium.org/345773004/diff/20001/apps/shell/common/shell_...
File apps/shell/common/shell_extensions_client.cc (right):

https://codereview.chromium.org/345773004/diff/20001/apps/shell/common/shell_...
apps/shell/common/shell_extensions_client.cc:110: (new
extensions::IconsHandler)->Register();
On 2014/06/20 14:48:45, James Cook wrote:
> Can this go in extensions/common/common_manifest_handlers.cc ?
> 
> If so it should also be removed from 
> chrome/common/extensions/chrome_extensions_client.cc.

Done.
I've noticed a few other src/extensions/common manifest handlers are initialized
in chrome_manifest_handlers.cc. Do you know any reasons why they have to be
chrome/?  I suspect this is just a historical reason.

https://codereview.chromium.org/345773004/diff/20001/athena/content/content_a...
File athena/content/content_app_model_builder.cc (right):

https://codereview.chromium.org/345773004/diff/20001/athena/content/content_a...
athena/content/content_app_model_builder.cc:79: // TODO(mukai): better default
icon
On 2014/06/20 14:48:45, James Cook wrote:
> Not for this CL, but there is an IDR_APP_DEFAULT_ICON you could use.
> Unfortunately it lives in chrome/app/theme/theme_resources.grd so it would
> require more refactoring.  Or you could copy it for Athena -- it might be OK
for
> Athena to have a different default icon.

Note that resource_bundle is still not yet initialized in athena.

[James Cook, 2014-06-20 16:36:15]

LGTM

I'm not sure why the other manifest handler registration is still in Chrome. 
"Historical" sounds right. :-)

[Jun Mukai, 2014-06-20 16:47:49]

Adding sheib as TBR for icons manifest handler move.

[Jun Mukai, 2014-06-20 16:47:53]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-20 16:50:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/345773004/40001

[scheib, 2014-06-20 18:13:16]

lgtm, however this is not clearly a TBR situation to me:
http://dev.chromium.org/developers/owners-files and I would prefer the change
description to document why the code move was needed and made.

[Jun Mukai, 2014-06-20 18:29:08]

On 2014/06/20 18:13:16, scheib wrote:
> lgtm, however this is not clearly a TBR situation to me:
> http://dev.chromium.org/developers/owners-files and I would prefer the change
> description to document why the code move was needed and made.

sorry.

[commit-bot: I haz the power, 2014-06-20 20:02:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-20 20:09:48]

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

[commit-bot: I haz the power, 2014-06-20 20:09:49]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[Jun Mukai, 2014-06-20 20:18:20]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-20 20:20:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/345773004/40001

[commit-bot: I haz the power, 2014-06-20 21:24:37]

Change committed as 278825

[xiyuan, 2014-06-20 21:36:04]

https://codereview.chromium.org/345773004/diff/40001/athena/content/content_a...
File athena/content/content_app_model_builder.cc (right):

https://codereview.chromium.org/345773004/diff/40001/athena/content/content_a...
athena/content/content_app_model_builder.cc:82:
icon_image_.image_skia().EnsureRepsForSupportedScales();
nit: This is not necessary. AppListMainView::PreloadIcons would trigger the load
early before UI is shown. After that, paint would trigger the load. Doing it
here just adds unnecessary loading jobs.