Refactor extension app icon.

Refactor extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ChromeAppIcon is introduced that applies all necessary effects based on current environment and app type. ChromeAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ChromeAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.

TBR=yoshiki@chromium.org

Review-Url: https://codereview.chromium.org/2819413003
Cr-Commit-Position: refs/heads/master@{#471035}
Committed: https://chromium.googlesource.com/chromium/src/+/8c1f6622f0279ee6062496672a7b83317b579486

[khmel, 2017-04-18 17:15:46]

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

[khmel, 2017-04-18 17:15:46]

Hi Xiyuan,

May I ask you to perform high level review in order to make sure that suggested
design is acceptable? If it approved I will add few more tests.

Thanks!

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:144: bool uninstalled) {}
This is to fix badging in case we opts out ARC. Badging is currently implemented
tracking onPackage event but Removed even was not issued on ARC opt out. This
extends this interface and issue OnPackageRemoved events with uninstalled =
false in case ARC is opted out

[xiyuan, 2017-04-18 20:43:01]

The approach looks fine with me.

Not sure what is the best home for ExtensionAppIcon and friends though. They are
not core extensions code and has dependency in ui/ash. So maybe instead of
browser/extensions, we put them in browser/ui/extensinos ? Please check with
rdevlin.cronin@ (he is the owner of both dir) before you make changes (I don't
want to waste your time).

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon.cc:143: // We own this object. Do
nothing.
How about just get rid of it?

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon.h:26: class ExtensionAppIcon :
public extensions::IconImage::Observer {
Document the class

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_delegate.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_delegate.h:18: virtual void
UpdateIcon(ExtensionAppIcon* icon,
Document the method. 

  // Invoked when ExtensionAppIcon is updated.

And this makes me feel the function should probably be called OnIconUpdated.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_delegate.h:19: const
gfx::ImageSkia& image) = 0;
nit: Shouldn't |image| be part of |icon|? i.e. expose |image| from |icon|.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.cc:35:
base::WeakPtr<ExtensionAppIconService> host_;
WeakPtr is not necessary since |host_| owns |this|.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.h:24: class
ExtensionAppIconService : public KeyedService {
Document the class

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.h:34:
std::unique_ptr<ExtensionAppIcon> CreateIcon(
doc ?

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.h:39: void
OnIconDestroyed(ExtensionAppIcon* icon);
doc ?

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.h:51: void
OnAppUpdated(const std::string& app_id);
doc ?

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_service_factory.cc (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service_factory.cc:28:
BrowserContextDependencyManager::GetInstance()) {}
DependsOn(ExtensionRegistryFactory::GetInstance());

since we sort of depend on it (ExtensionAppIcon uses ExtensionRegistry).

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_service_factory.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service_factory.h:16: class
ExtensionAppIconServiceFactory
doc ?

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:696:
GetPackagesFromPrefs(false, false);
nit: doc the booleans ?

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1306: return
GetPackagesFromPrefs(true, true);
nit: doc the booleans

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:144: bool uninstalled) {}
On 2017/04/18 17:15:46, khmel wrote:
> This is to fix badging in case we opts out ARC. Badging is currently
implemented
> tracking onPackage event but Removed even was not issued on ARC opt out. This
> extends this interface and issue OnPackageRemoved events with uninstalled =
> false in case ARC is opted out

Can you also mention this in the comment? i.e. when OnPackageRemoved is called
with uninstalled = false.

[khmel, 2017-04-18 20:45:25]

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

[khmel, 2017-04-18 20:45:25]

Thank you Xiyuan for your comments!

As recommended, adding rdevlin.cronin@ for top level review.

Hi Devlin,
Could you please make high level review of this?
PTAL
Thanks

[khmel, 2017-04-20 00:13:38]

Hi Xiyuan,

I addressed your comments + provided required unit tests.

I postponed moving ExtensionAppIcon and "friends"@ :) once I have not heard from
Devlin yet. Anyway this should be mechanical change and won't change logic. I
can do it anytime.

So now it close to final version.
PTAL

Devlin, gentle ping.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon.cc:143: // We own this object. Do
nothing.
On 2017/04/18 20:43:00, xiyuan wrote:
> How about just get rid of it?

True, this is optional callback.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon.h:26: class ExtensionAppIcon :
public extensions::IconImage::Observer {
On 2017/04/18 20:43:00, xiyuan wrote:
> Document the class

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_delegate.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_delegate.h:18: virtual void
UpdateIcon(ExtensionAppIcon* icon,
On 2017/04/18 20:43:00, xiyuan wrote:
> Document the method. 
> 
>   // Invoked when ExtensionAppIcon is updated.
> 
> And this makes me feel the function should probably be called OnIconUpdated.

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_delegate.h:19: const
gfx::ImageSkia& image) = 0;
On 2017/04/18 20:43:00, xiyuan wrote:
> nit: Shouldn't |image| be part of |icon|? i.e. expose |image| from |icon|.

Makes sense. this gives more flexibility in using.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.cc:35:
base::WeakPtr<ExtensionAppIconService> host_;
On 2017/04/18 20:43:00, xiyuan wrote:
> WeakPtr is not necessary since |host_| owns |this|.

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.h:24: class
ExtensionAppIconService : public KeyedService {
On 2017/04/18 20:43:00, xiyuan wrote:
> Document the class

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.h:34:
std::unique_ptr<ExtensionAppIcon> CreateIcon(
On 2017/04/18 20:43:00, xiyuan wrote:
> doc ?

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.h:39: void
OnIconDestroyed(ExtensionAppIcon* icon);
On 2017/04/18 20:43:00, xiyuan wrote:
> doc ?

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service.h:51: void
OnAppUpdated(const std::string& app_id);
On 2017/04/18 20:43:00, xiyuan wrote:
> doc ?

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_service_factory.cc (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service_factory.cc:28:
BrowserContextDependencyManager::GetInstance()) {}
On 2017/04/18 20:43:01, xiyuan wrote:
> DependsOn(ExtensionRegistryFactory::GetInstance());
> 
> since we sort of depend on it (ExtensionAppIcon uses ExtensionRegistry).

Thanks for catching this! actually I was seeking the way adding the dependency.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_app_icon_service_factory.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_app_icon_service_factory.h:16: class
ExtensionAppIconServiceFactory
On 2017/04/18 20:43:01, xiyuan wrote:
> doc ?

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:696:
GetPackagesFromPrefs(false, false);
On 2017/04/18 20:43:01, xiyuan wrote:
> nit: doc the booleans ?

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1306: return
GetPackagesFromPrefs(true, true);
On 2017/04/18 20:43:01, xiyuan wrote:
> nit: doc the booleans

Done.

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2819413003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:144: bool uninstalled) {}
On 2017/04/18 20:43:01, xiyuan wrote:
> On 2017/04/18 17:15:46, khmel wrote:
> > This is to fix badging in case we opts out ARC. Badging is currently
> implemented
> > tracking onPackage event but Removed even was not issued on ARC opt out.
This
> > extends this interface and issue OnPackageRemoved events with uninstalled =
> > false in case ARC is opted out
> 
> Can you also mention this in the comment? i.e. when OnPackageRemoved is called
> with uninstalled = false.

Done.

https://codereview.chromium.org/2819413003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_extension_app_updater.cc (left):

https://codereview.chromium.org/2819413003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_extension_app_updater.cc:84: void
LauncherExtensionAppUpdater::UpdateHostedApps() {
Well, this code was not used. I had temptation to use it from
OnPackageListInitialRefreshed but it also contained bug 
if ((*it)->is_hosted_app())
Actually we handle not only hosted app.
So it was easier to remove dead code and just pass package from the list on
initial refresh.

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

https://codereview.chromium.org/2819413003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_extension_app_updater.cc:61: void
LauncherExtensionAppUpdater::OnPackageListInitialRefreshed() {
This is reason why badge may not be applied also.

[khmel, 2017-04-20 01:09:28]

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

[commit-bot: I haz the power, 2017-04-20 01:10:03]

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-20 01:25:30]

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

[commit-bot: I haz the power, 2017-04-20 01:25:33]

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

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

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

[commit-bot: I haz the power, 2017-04-20 15:39:10]

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

[xiyuan, 2017-04-20 16:36:32]

Mostly nits.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_service.cc:64:
registry->AddObserver(this);
nit: get rid of |registry| and inline ExtensionRegistry::Get() here.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_service.cc:74:
registry->RemoveObserver(this);
nit: ditto

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_service.cc:88: void
ExtensionAppIconService::OnExtensionLoaded(
nit: insert an empty line before

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_service.h:61: void
OnIconDestroyed(ExtensionAppIcon* icon);
Can we base::Bind this as a OnIconDestroyedClosure and pass it to
ExtensionAppIcon? This way, we could get rid of the friend class and
ExtensionAppIcon does not need to know ExtensionAppIconService.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:56:
icon_updated_callback_.Reset();
nit: We can avoid this call by making |icon_updated_callback_| a
base::OnceClosure. And invoke it std::move(icon_updated_callback_).Run() would
auto reset it.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:131: bool
AreEqual(const gfx::ImageSkia& image1, const gfx::ImageSkia& image2) {
You can use gfx::test::AreImagesEqual for this.

e.g.
https://cs.chromium.org/chromium/src/ash/system/user/tray_user_unittest.cc?rc...

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:183:
EXPECT_TRUE(reference_icon.image_skia().bitmap());
nit: avoid bitmap() since it is going way. Use GetRepresentation(1.0f) instead.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:257:
EXPECT_TRUE(reference_icon.image_skia().bitmap());
nit: GetRepresentation(1.0f)

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:258: while
(IsBlankImage(reference_icon.image_skia()))
Why using a while loop instead of WaitIconUpdated ?

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:288:
EXPECT_TRUE(reference_icon.image_skia().bitmap());
nit: GetRepresentation(1.0f)

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:302:
EXPECT_EQ(icon_update_count_before_badging + 1,
nit: Other tests are using a GetCountAndReset that returns the current count and
resets it to 0 on return so that we can EXPECT_EQ on delta.

[commit-bot: I haz the power, 2017-04-20 17:05:27]

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

[commit-bot: I haz the power, 2017-04-20 17:05:28]

Dry run: This issue passed the CQ dry run.

[khmel, 2017-04-20 17:46:19]

Thanks for your comments, PTAL updated version

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_service.cc:64:
registry->AddObserver(this);
On 2017/04/20 16:36:31, xiyuan wrote:
> nit: get rid of |registry| and inline ExtensionRegistry::Get() here.

Done.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_service.cc:74:
registry->RemoveObserver(this);
On 2017/04/20 16:36:31, xiyuan wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_service.cc:88: void
ExtensionAppIconService::OnExtensionLoaded(
On 2017/04/20 16:36:31, xiyuan wrote:
> nit: insert an empty line before

Done.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_service.h:61: void
OnIconDestroyed(ExtensionAppIcon* icon);
On 2017/04/20 16:36:31, xiyuan wrote:
> Can we base::Bind this as a OnIconDestroyedClosure and pass it to
> ExtensionAppIcon? This way, we could get rid of the friend class and
> ExtensionAppIcon does not need to know ExtensionAppIconService.

That is nice, thanks

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:56:
icon_updated_callback_.Reset();
On 2017/04/20 16:36:32, xiyuan wrote:
> nit: We can avoid this call by making |icon_updated_callback_| a
> base::OnceClosure. And invoke it std::move(icon_updated_callback_).Run() would
> auto reset it.

I also found base::ResetAndReturn approach. Would it look well here?

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:131: bool
AreEqual(const gfx::ImageSkia& image1, const gfx::ImageSkia& image2) {
On 2017/04/20 16:36:32, xiyuan wrote:
> You can use gfx::test::AreImagesEqual for this.
> 
> e.g.
>
https://cs.chromium.org/chromium/src/ash/system/user/tray_user_unittest.cc?rc...

Done.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:183:
EXPECT_TRUE(reference_icon.image_skia().bitmap());
On 2017/04/20 16:36:32, xiyuan wrote:
> nit: avoid bitmap() since it is going way. Use GetRepresentation(1.0f)
instead.

Done.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:257:
EXPECT_TRUE(reference_icon.image_skia().bitmap());
On 2017/04/20 16:36:31, xiyuan wrote:
> nit: GetRepresentation(1.0f)

Done.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:258: while
(IsBlankImage(reference_icon.image_skia()))
On 2017/04/20 16:36:31, xiyuan wrote:
> Why using a while loop instead of WaitIconUpdated ?

Hmmm...., you are right.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:288:
EXPECT_TRUE(reference_icon.image_skia().bitmap());
On 2017/04/20 16:36:31, xiyuan wrote:
> nit: GetRepresentation(1.0f)

Done.

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:302:
EXPECT_EQ(icon_update_count_before_badging + 1,
On 2017/04/20 16:36:32, xiyuan wrote:
> nit: Other tests are using a GetCountAndReset that returns the current count
and
> resets it to 0 on return so that we can EXPECT_EQ on delta.

Done.

[xiyuan, 2017-04-20 17:52:46]

lgtm

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon_unittest.cc:56:
icon_updated_callback_.Reset();
On 2017/04/20 17:46:18, khmel wrote:
> On 2017/04/20 16:36:32, xiyuan wrote:
> > nit: We can avoid this call by making |icon_updated_callback_| a
> > base::OnceClosure. And invoke it std::move(icon_updated_callback_).Run()
would
> > auto reset it.
> 
> I also found base::ResetAndReturn approach. Would it look well here?

That works.

https://codereview.chromium.org/2819413003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon.cc:10: #include
"chrome/browser/extensions/extension_app_icon_service.h"
remove ?

https://codereview.chromium.org/2819413003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon.h:33: using Closure =
base::Callback<void(ExtensionAppIcon*)>;
nit: Closure -> CloseCallback (or DestroyCallback).

Technically, this is not a closure since we need to run it with an arg. And
Closure is easy to be confused with base::Closure.

[khmel, 2017-04-20 18:01:11]

Thank you Xiyuan!

Adding msw@ (c/b/ui/views  c/b/ui/ash)

PTAL

https://codereview.chromium.org/2819413003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon.cc:10: #include
"chrome/browser/extensions/extension_app_icon_service.h"
On 2017/04/20 17:52:46, xiyuan wrote:
> remove ?

Done.

https://codereview.chromium.org/2819413003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_app_icon.h:33: using Closure =
base::Callback<void(ExtensionAppIcon*)>;
On 2017/04/20 17:52:46, xiyuan wrote:
> nit: Closure -> CloseCallback (or DestroyCallback).
> 
> Technically, this is not a closure since we need to run it with an arg. And
> Closure is easy to be confused with base::Closure. 

Done.

[khmel, 2017-04-20 18:01:34]

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

[khmel, 2017-04-20 18:01:34]

Hi Mike,

PTAL

Thanks

[msw, 2017-04-20 18:49:23]

I'm hoping Devlin or someone else more familiar with extension icon details will
take a look at this first; it's sad to see so much new machinery for just
getting an icon. This CL doesn't seem like a refactoring to me...

[xiyuan, 2017-04-20 18:53:50]

On 2017/04/20 18:49:23, msw wrote:
> I'm hoping Devlin or someone else more familiar with extension icon details
will
> take a look at this first; it's sad to see so much new machinery for just
> getting an icon. This CL doesn't seem like a refactoring to me...

The intention is to consolidate the app icon creating from multiple sources
(icons in applist grid + search results, icons in launcher/shelf etc) into one
ExtensionAppIcon class. So that we don't need to implement similar things
multiple times (badging, greying) and sometime forget to update all places.

[Devlin, 2017-04-20 20:17:41]

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:30: class
ExtensionAppIconService : public KeyedService,
Do we even need this class?  Is there a reason the icons couldn't observe the
registry directly for extension loaded/unloaded calls?

[khmel, 2017-04-20 20:21:53]

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:30: class
ExtensionAppIconService : public KeyedService,
On 2017/04/20 20:17:41, Devlin wrote:
> Do we even need this class?  Is there a reason the icons couldn't observe the
> registry directly for extension loaded/unloaded calls?

This would requires tons on listeners and very inefficient form memory and
performance point of view. (Each icon has to observe callbacks for all extension
events (not only personal event). Also each icon has to create
LauncherExtensionAppUpdater (name and location of this class is bad but this is
out of scope current CL)). Instead one service is created that receives
callbacks and dispatches to corresponded icons.

[khmel, 2017-04-26 20:22:12]

Hi Devlin,

Would you find a chance to take a look?

Thanks!

[Devlin, 2017-04-26 22:41:58]

Sorry for the delay!

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:26: // This represents how an
extension icon should finally look. As a base,
This is really only used for app icons, right?  Extension icons (such as those
shown in context menus, the toolbar, the extensions page, etc) are all
different.  We should probably say "app icon" here.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:31: class ExtensionAppIcon :
public extensions::IconImage::Observer {
Similarly, maybe this whole class should just be ChromeAppIcon?

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:31: class ExtensionAppIcon :
public extensions::IconImage::Observer {
no need for extensions:: prefix inside the extensions namespace (this whole
file)

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:33: using DestroyCallback =
base::Callback<void(ExtensionAppIcon*)>;
nit: DestroyedCallback, or OnDestroyedCallback.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:33: using DestroyCallback =
base::Callback<void(ExtensionAppIcon*)>;
Can this be a OnceCallback?

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:42: void Reload();
Add a function comment

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:49: void
EnsureRepsForSupportedScales();
Add a function comment

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.cc:64:
ExtensionRegistry::Get(context_)->AddObserver(this);
Prefer a ScopedObserver

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:30: class
ExtensionAppIconService : public KeyedService,
On 2017/04/20 20:21:53, khmel wrote:
> On 2017/04/20 20:17:41, Devlin wrote:
> > Do we even need this class?  Is there a reason the icons couldn't observe
the
> > registry directly for extension loaded/unloaded calls?
> 
> This would requires tons on listeners and very inefficient form memory and
> performance point of view. (Each icon has to observe callbacks for all
extension
> events (not only personal event). Also each icon has to create
> LauncherExtensionAppUpdater (name and location of this class is bad but this
is
> out of scope current CL)). Instead one service is created that receives
> callbacks and dispatches to corresponded icons.

Ah, I missed that call to UpdateIcon (and only saw the ones from the OnUnload). 
I wouldn't worry much about the added observers, since, realistically, this
number is relatively small, and adding extra observers is quite cheap (probably
cheaper than another KeyedService).  But, moot given the extra update calls.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:33: static
ExtensionAppIconService* Create(content::BrowserContext* context);
Why do we need a Create() rather than just using the constructor?

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:55: using IconMap =
std::multimap<std::string, ExtensionAppIcon*>;
Is there a reason to use a multimap instead of e.g. a std::map<std::string,
std::set>?  It seems like that would make some of the operations easier/clearer.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service_factory.cc (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service_factory.cc:42: // This
matches the logic in ExtensionSyncServiceFactory, which uses the
What's the pertinence of ExtensionSyncServiceFactory?

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_unittest.cc:170: EXPECT_EQ(1,
reference_icon.icon_update_count());
Testing the update count seems to be relying a bit too much on the
implementation of the code, rather than the behavior.  Could we instead just
test that the icon returned is accurate under the different circumstances?

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

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:174:
AppIconLoader* acon_loader = owner_->GetAppIconLoaderForApp(app_id);
typo: s/acon/icon

[khmel, 2017-04-27 00:18:12]

Thank you for comments.

Please find updated version

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:26: // This represents how an
extension icon should finally look. As a base,
On 2017/04/26 22:41:57, Devlin wrote:
> This is really only used for app icons, right?  Extension icons (such as those
> shown in context menus, the toolbar, the extensions page, etc) are all
> different.  We should probably say "app icon" here.

Make sense, updated to extension app icon.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:31: class ExtensionAppIcon :
public extensions::IconImage::Observer {
On 2017/04/26 22:41:57, Devlin wrote:
> Similarly, maybe this whole class should just be ChromeAppIcon?

this would cause a confusion with ARC apps which have different loading.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:31: class ExtensionAppIcon :
public extensions::IconImage::Observer {
On 2017/04/26 22:41:57, Devlin wrote:
> no need for extensions:: prefix inside the extensions namespace (this whole
> file)

Done.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:33: using DestroyCallback =
base::Callback<void(ExtensionAppIcon*)>;
On 2017/04/26 22:41:57, Devlin wrote:
> Can this be a OnceCallback?

Done.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:33: using DestroyCallback =
base::Callback<void(ExtensionAppIcon*)>;
On 2017/04/26 22:41:57, Devlin wrote:
> nit: DestroyedCallback, or OnDestroyedCallback.

Done.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:42: void Reload();
On 2017/04/26 22:41:57, Devlin wrote:
> Add a function comment

Done.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:49: void
EnsureRepsForSupportedScales();
On 2017/04/26 22:41:57, Devlin wrote:
> Add a function comment

Done.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.cc:64:
ExtensionRegistry::Get(context_)->AddObserver(this);
On 2017/04/26 22:41:57, Devlin wrote:
> Prefer a ScopedObserver

Done.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:30: class
ExtensionAppIconService : public KeyedService,
On 2017/04/26 22:41:57, Devlin wrote:
> On 2017/04/20 20:21:53, khmel wrote:
> > On 2017/04/20 20:17:41, Devlin wrote:
> > > Do we even need this class?  Is there a reason the icons couldn't observe
> the
> > > registry directly for extension loaded/unloaded calls?
> > 
> > This would requires tons on listeners and very inefficient form memory and
> > performance point of view. (Each icon has to observe callbacks for all
> extension
> > events (not only personal event). Also each icon has to create
> > LauncherExtensionAppUpdater (name and location of this class is bad but this
> is
> > out of scope current CL)). Instead one service is created that receives
> > callbacks and dispatches to corresponded icons.
> 
> Ah, I missed that call to UpdateIcon (and only saw the ones from the
OnUnload). 
> I wouldn't worry much about the added observers, since, realistically, this
> number is relatively small, and adding extra observers is quite cheap
(probably
> cheaper than another KeyedService).  But, moot given the extra update calls.

Acknowledged.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:33: static
ExtensionAppIconService* Create(content::BrowserContext* context);
On 2017/04/26 22:41:57, Devlin wrote:
> Why do we need a Create() rather than just using the constructor?

This is common practice for services to have CTOR private and call ::Create from
*Factory. Also I see implementations with friend instead of ::Create. Please let
me know what do you prefer. I changed to public CTOR in this updated.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:55: using IconMap =
std::multimap<std::string, ExtensionAppIcon*>;
On 2017/04/26 22:41:57, Devlin wrote:
> Is there a reason to use a multimap instead of e.g. a std::map<std::string,
> std::set>?  It seems like that would make some of the operations
easier/clearer.

Changed as you suggested.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service_factory.cc (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service_factory.cc:42: // This
matches the logic in ExtensionSyncServiceFactory, which uses the
On 2017/04/26 22:41:57, Devlin wrote:
> What's the pertinence of ExtensionSyncServiceFactory?

Removed. it seems it is redundant, came from copy/paste.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_unittest.cc:170: EXPECT_EQ(1,
reference_icon.icon_update_count());
On 2017/04/26 22:41:57, Devlin wrote:
> Testing the update count seems to be relying a bit too much on the
> implementation of the code, rather than the behavior.  Could we instead just
> test that the icon returned is accurate under the different circumstances?

My idea was to test that we don't have redundant updates also. From this point
of view, this code validates that we have only one update (which is expected)
and not more.

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

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:174:
AppIconLoader* acon_loader = owner_->GetAppIconLoaderForApp(app_id);
On 2017/04/26 22:41:57, Devlin wrote:
> typo: s/acon/icon

Done.

[Devlin, 2017-05-01 14:51:11]

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:31: class ExtensionAppIcon :
public extensions::IconImage::Observer {
On 2017/04/27 00:18:12, khmel wrote:
> On 2017/04/26 22:41:57, Devlin wrote:
> > Similarly, maybe this whole class should just be ChromeAppIcon?
> 
> this would cause a confusion with ARC apps which have different loading.

"Chrome Apps" are separate from ARC apps, and I haven't heard or seen references
to ARC/android apps as chrome apps in the code.  On the other hand, extensions
and apps are different, and an "extension app" isn't a thing. :)  Additionally,
given the file location, I think the overlap with ARC is sufficiently low that
differentiating between extensions is more important.  I'm open to alternative
suggestions, but I think "ExtensionApp" is a bit too unusual.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:33: static
ExtensionAppIconService* Create(content::BrowserContext* context);
On 2017/04/27 00:18:12, khmel wrote:
> On 2017/04/26 22:41:57, Devlin wrote:
> > Why do we need a Create() rather than just using the constructor?
> 
> This is common practice for services to have CTOR private and call ::Create
from
> *Factory. Also I see implementations with friend instead of ::Create. Please
let
> me know what do you prefer. I changed to public CTOR in this updated.

We should only need a Create() method when there's some additional logic to the
creation, like when we might want to stub out the factory for a different one. 
Since that's not the case here, we should be fine.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.cc:89: const extensions::Extension*
ExtensionAppIcon::GetExtension() {
Remember, no need for extensions:: prefix in the extensions namespace.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.cc:68:
observer_.Remove(ExtensionRegistry::Get(context_));
Given this depends on ExtensionRegistry(), we should just be able to let this
destruct and have the observer automatically remove itself.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.cc:110: NOTREACHED();
Why not a DCHECK?  The NOTREACHED(); return; pattern should only be used in
cases like IPCs where there's a chance that a compromised process could send
arbitrary values.  If we don't expect it to ever be the case, we shouldn't use
this pattern.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:35: // See the Create
methods.
Obsolete?

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:55: friend class
ExtensionAppIcon;
Is this needed?

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_unittest.cc:174: // Defualt blank
image must be provided without an update.
nit: typo - "Default"

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_unittest.cc:277: while
(!AreEqual(reference_icon.image_skia(), app_list_item->icon()))
Can we do this in a bit more of a deterministic way?  e.g., if you want to keep
the behavior that we know exactly when an icon should be updated, can't we wait
for the update?

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/search/extension_app_result.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/extension_app_result.cc:8: #include
"chrome/browser/chromeos/extensions/gfx_utils.h"
needed?

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_extension_app_updater.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_extension_app_updater.cc:64: for (auto&
package_name : package_names)
nit: const auto&

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h:13: // TODO
(khmel), this is not Launcher class. Consider moving this to proper
nit: s/TODO (khmel), /TODO(khmel):

[khmel, 2017-05-01 18:50:07]

Patchset #9 (id:160001) has been deleted

[khmel, 2017-05-01 19:36:44]

Patchset #10 (id:200001) has been deleted

[khmel, 2017-05-01 20:18:19]

Thanks Devlin for comments

PTAL updated version

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.h:31: class ExtensionAppIcon :
public extensions::IconImage::Observer {
On 2017/05/01 14:51:10, Devlin wrote:
> On 2017/04/27 00:18:12, khmel wrote:
> > On 2017/04/26 22:41:57, Devlin wrote:
> > > Similarly, maybe this whole class should just be ChromeAppIcon?
> > 
> > this would cause a confusion with ARC apps which have different loading.
> 
> "Chrome Apps" are separate from ARC apps, and I haven't heard or seen
references
> to ARC/android apps as chrome apps in the code.  On the other hand, extensions
> and apps are different, and an "extension app" isn't a thing. :) 
Additionally,
> given the file location, I think the overlap with ARC is sufficiently low that
> differentiating between extensions is more important.  I'm open to alternative
> suggestions, but I think "ExtensionApp" is a bit too unusual.

Renamed to ChromeAppIcon.

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:33: static
ExtensionAppIconService* Create(content::BrowserContext* context);
On 2017/05/01 14:51:10, Devlin wrote:
> On 2017/04/27 00:18:12, khmel wrote:
> > On 2017/04/26 22:41:57, Devlin wrote:
> > > Why do we need a Create() rather than just using the constructor?
> > 
> > This is common practice for services to have CTOR private and call ::Create
> from
> > *Factory. Also I see implementations with friend instead of ::Create. Please
> let
> > me know what do you prefer. I changed to public CTOR in this updated.
> 
> We should only need a Create() method when there's some additional logic to
the
> creation, like when we might want to stub out the factory for a different one.

> Since that's not the case here, we should be fine.

Acknowledged.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon.cc:89: const extensions::Extension*
ExtensionAppIcon::GetExtension() {
On 2017/05/01 14:51:11, Devlin wrote:
> Remember, no need for extensions:: prefix in the extensions namespace.

Thanks, overlooked this.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.cc:68:
observer_.Remove(ExtensionRegistry::Get(context_));
On 2017/05/01 14:51:11, Devlin wrote:
> Given this depends on ExtensionRegistry(), we should just be able to let this
> destruct and have the observer automatically remove itself.

Done.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.cc:110: NOTREACHED();
On 2017/05/01 14:51:11, Devlin wrote:
> Why not a DCHECK?  The NOTREACHED(); return; pattern should only be used in
> cases like IPCs where there's a chance that a compromised process could send
> arbitrary values.  If we don't expect it to ever be the case, we shouldn't use
> this pattern.

Changed to DCHECK

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:35: // See the Create
methods.
On 2017/05/01 14:51:11, Devlin wrote:
> Obsolete?

Done.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_service.h:55: friend class
ExtensionAppIcon;
On 2017/05/01 14:51:11, Devlin wrote:
> Is this needed?

Done.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_unittest.cc:174: // Defualt blank
image must be provided without an update.
On 2017/05/01 14:51:11, Devlin wrote:
> nit: typo - "Default"

Done.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_app_icon_unittest.cc:277: while
(!AreEqual(reference_icon.image_skia(), app_list_item->icon()))
On 2017/05/01 14:51:11, Devlin wrote:
> Can we do this in a bit more of a deterministic way?  e.g., if you want to
keep
> the behavior that we know exactly when an icon should be updated, can't we
wait
> for the update?

Done.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/search/extension_app_result.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/extension_app_result.cc:8: #include
"chrome/browser/chromeos/extensions/gfx_utils.h"
On 2017/05/01 14:51:11, Devlin wrote:
> needed?

nice catch

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_extension_app_updater.cc (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_extension_app_updater.cc:64: for (auto&
package_name : package_names)
On 2017/05/01 14:51:11, Devlin wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h (right):

https://codereview.chromium.org/2819413003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h:13: // TODO
(khmel), this is not Launcher class. Consider moving this to proper
On 2017/05/01 14:51:11, Devlin wrote:
> nit: s/TODO (khmel), /TODO(khmel): 

Done.

[Devlin, 2017-05-01 21:40:56]

extensions lgtm.

I'm not familiar with the buildbot files, but if that's an intentional change,
please mention it in the CL description.

Also, please update the CL description to use ChromeAppIcon instead of
ExtensionAppIcon.

https://codereview.chromium.org/2819413003/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:44: std::max((int)std::round(2.0 *
icon_.width() / 48.0), 1);
nit: prefer static_cast<int> (sorry I didn't see this earlier)

https://codereview.chromium.org/2819413003/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2819413003/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:653: for (auto package_name
: package_names_to_remove) {
const auto&?

https://codereview.chromium.org/2819413003/diff/220001/testing/buildbot/chrom...
File testing/buildbot/chromium.android.json (right):

https://codereview.chromium.org/2819413003/diff/220001/testing/buildbot/chrom...
testing/buildbot/chromium.android.json:10: "merge": {
What are these changes?

[khmel, 2017-05-01 22:33:13]

Description was changed from

==========
Refactore extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ExtensionAppIcon is introduced that
applies all neceesarry effects based on current environment and app
type. ExtensionAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ExtensionAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.
==========

to

==========
Refactore extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ChromeAppIcon is introduced that applies all
necessary effects based on current environment and app type.
ChromeAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ChromeAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.
==========

[khmel, 2017-05-01 22:33:47]

Description was changed from

==========
Refactore extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ChromeAppIcon is introduced that applies all
necessary effects based on current environment and app type.
ChromeAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ChromeAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.
==========

to

==========
Refactor extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ChromeAppIcon is introduced that applies all
necessary effects based on current environment and app type.
ChromeAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ChromeAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.
==========

[khmel, 2017-05-01 22:41:34]

Patchset #11 (id:240001) has been deleted

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

Patchset #11 (id:260001) has been deleted

[khmel, 2017-05-02 01:03:52]

Thank you Devlin for review!

Mike, PTAL
Thanks

https://codereview.chromium.org/2819413003/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:44: std::max((int)std::round(2.0 *
icon_.width() / 48.0), 1);
On 2017/05/01 21:40:56, Devlin wrote:
> nit: prefer static_cast<int> (sorry I didn't see this earlier)

np, it is big cl. And thanks for catching this.

https://codereview.chromium.org/2819413003/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2819413003/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:653: for (auto package_name
: package_names_to_remove) {
On 2017/05/01 21:40:56, Devlin wrote:
> const auto&?

Done.

https://codereview.chromium.org/2819413003/diff/220001/testing/buildbot/chrom...
File testing/buildbot/chromium.android.json (right):

https://codereview.chromium.org/2819413003/diff/220001/testing/buildbot/chrom...
testing/buildbot/chromium.android.json:10: "merge": {
On 2017/05/01 21:40:56, Devlin wrote:
> What are these changes?

Sorry, this comes from git flows, Rebase fixed this.

[khmel, 2017-05-02 01:04:16]

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

[commit-bot: I haz the power, 2017-05-02 01:04:40]

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-05-02 02:16:02]

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

[commit-bot: I haz the power, 2017-05-02 02:16:03]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-03 01:17:39]

I didn't get through the whole CL, but wanted to send what comments and
questions I had. I haven't wrapped my head around this completely, it's rather
complex. Perhaps someone that previously worked on app icons, like tbarzic@
should also take a look?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/gfx_utils.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/gfx_utils.cc:97: // Used in unit tests.
Could unit tests just use one of the above entries?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/gfx_utils.cc:98: {"fake.package.name1",
"emfkafnhnpcmabnnkckkchdilgeoekbo"},
The tests don't actually ever mention "fake.package.name1", is this mapping
needed? If so, perhaps explain why it's needed and actually check for
"fake.package.name1" in the tests?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_system_provider/notification_manager.cc
(right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/file_system_provider/notification_manager.cc:58:
icon_loader_(
optional nit/aside: icon_loader_ need not be a std::unique_ptr

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/file_system/request_file_system_notification.cc
(right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/request_file_system_notification.cc:106:
: icon_loader_(
optional nit/aside: icon_loader_ need not be a std::unique_ptr

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:26: gfx::ImageSkia
CreateDisabledIcon(const gfx::ImageSkia& icon) {
nit: inline for the one caller.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:32: class RoundedCornersImageSource
: public gfx::CanvasImageSource {
Can we avoid subclassing CanvasImageSource and just use a function to generate
an ImageSkia with rounded corners (defined by a mask or premultiplied alpha
values built into the ImageSkia)?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:44:
std::max(static_cast<int>(std::round(2.0 * icon_.width() / 48.0)), 1);
int: use std::max<int> and avoid the cast, or use non-integral values

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:86:
std::move(destroyed_callback_).Run(this);
Why do you need to move the callback to run it?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:90: const Extension* extension =
nit: inline this in the return statement.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:100: const Extension* extension =
GetExtension();
nit: inline in call below

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:115: const bool enabled =
util::IsAppLaunchable(app_id_, context_);
nit: inline in conditional

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:24: class IconImage;
nit: not needed; defined in extension_icon_image.h

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:31: class ChromeAppIcon : public
IconImage::Observer {
If each ChromeAppIcon is owned by the transient UI displaying the icon (app list
items, app list search results, app info dialog header... each of which might be
requesting a different size_in_dip anyway), and then destroyed when that
transient UI goes away, is it worthwhile to have a centralized cache at all? How
often is the icon actually re-used between these, instead of destroyed and then
reloaded over and over? Perhaps it would make sense to just refine AppIconLoader
instead of adding the new service? Also, why doesn't the shelf use these icons?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:45: // Returns true if icon is
loaded and attached to valid extension.
What does it mean for the icon to be attached to a valid extension? I don't see
how that's checked in the impl.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:48: void UpdateIcon();
nit: comment; maybe allude to how UpdateIcon differs from Reload?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:51: void
EnsureRepsForSupportedScales();
nit: remove this, let the one non-test caller (ChromeAppIconLoader::FetchImage)
do image_skia().EnsureRepsForSupportedScales() instead.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:55: content::BrowserContext*
context() { return context_; }
Remove this if it's not used anywhere, otherwise rename browser_context()

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:65: content::BrowserContext* const
context_;
nit: browser_context_

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:67: DestroyedCallback
destroyed_callback_;
nit: comment, describe when this is called (when the ChromeAppIcon is destroyed?
when the IconImage is destroyed?). Would it be sufficient to instead vend
WeakPtr<ChromeAppIcon>s and store those in the ChromeAppIconService::icon_map_?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:71: const int resource_size_in_dip_;
Would it make sense to just expose the underlying value in IconImage, and remove
this redundant value?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:76: std::unique_ptr<IconImage>
icon_;
How does this IconImage |icon_|, which contains an ImageSkia, relate to the
ImageSkia |image_skia_| above? Are they the same? If so, maybe remove
|image_skia_| and just add an accessor for icon_->image_skia() if needed.
Otherwise, maybe explain in comments here how they are different.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_delegate.h (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_delegate.h:14: // Invoked when
ChromeAppIcon is updated. "icon->image_skia()" contains
nit: use vertical bars instead of quotes

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_delegate.h:15: // the current icon
image.
Does current mean the post-update current, or does it mean the prior-to-update
current? Perhaps say "new"/"updated" or "old" instead of current.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_loader.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.cc:54:
ChromeAppIconService::Get(profile())->CreateIcon(this, id, icon_size());
IT seems like the service does most of the actual loading, should
ChromeAppIconService inherit from AppIconLoader and then we could just remove
this class?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.cc:76:
delegate()->OnAppImageUpdated(icon->app_id(), icon->image_skia());
It's unfortunate that we have to go through two delegates here (this and
delegate())

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_loader.h (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.h:21: // ImageLoader to load
images.
What is ImageLoader? I don't see that referenced here or in the cc file.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.h:25: int icon_size,
nit: icon_size_in_dips?

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.h:36: using
ExtensionIDToImageMap =
nit: s/Image/ChromeAppIcon/? Let's avoid mixing the names where possible.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.h:42: // Maps from extension id
to IconImage pointer.
nit: s/IconImage pointer/ChromeAppIcon/?

[khmel, 2017-05-03 02:15:23]

Thank you Mike for comments. Please see my responses and updated version.

>> Perhaps someone that previously worked on app icons, like >> tbarzic@
>> should also take a look?
IIUC Xiyuan has recent experience and did a lot of review in icons and
especially in ExtensionAppIconLoader and in code caused by ARC refactoring.

Anyway if you know someone you want to add please add.
Thanks!

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/gfx_utils.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/gfx_utils.cc:97: // Used in unit tests.
On 2017/05/03 01:17:37, msw wrote:
> Could unit tests just use one of the above entries?

ARC test package name is defined here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_t...

emfkafnhnpcmabnnkckkchdilgeoekbo is an extension in chrome/test.

Technically it is possible to do but this will require additional code change,
including existing unit tests were extension id are predefined and number of
tests is also predefined. Actually that was my intention initially to do like
this. But changes were big enough.
If you insist on removing this I can do it in separate issue in order not to
grow this cl.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/gfx_utils.cc:98: {"fake.package.name1",
"emfkafnhnpcmabnnkckkchdilgeoekbo"},
Replied above.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_system_provider/notification_manager.cc
(right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/file_system_provider/notification_manager.cc:58:
icon_loader_(
On 2017/05/03 01:17:37, msw wrote:
> optional nit/aside: icon_loader_ need not be a std::unique_ptr

If you don't mind I won't change. This change is just mechanical change due
renaming ExtensionAppIconLoader to ChromeAppIconLoader.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/file_system/request_file_system_notification.cc
(right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/request_file_system_notification.cc:106:
: icon_loader_(
On 2017/05/03 01:17:37, msw wrote:
> optional nit/aside: icon_loader_ need not be a std::unique_ptr

The same as previous comment. This is mechanical change due renaming.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:26: gfx::ImageSkia
CreateDisabledIcon(const gfx::ImageSkia& icon) {
On 2017/05/03 01:17:37, msw wrote:
> nit: inline for the one caller.

There are several places in code with dup functionality. I keep in mind
possibility to provide helper.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:32: class RoundedCornersImageSource
: public gfx::CanvasImageSource {
On 2017/05/03 01:17:38, msw wrote:
> Can we avoid subclassing CanvasImageSource and just use a function to generate
> an ImageSkia with rounded corners (defined by a mask or premultiplied alpha
> values built into the ImageSkia)?

Would TODO here be acceptable? That was legacy code and I would prefer to do it
separately.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:44:
std::max(static_cast<int>(std::round(2.0 * icon_.width() / 48.0)), 1);
On 2017/05/03 01:17:38, msw wrote:
> int: use std::max<int> and avoid the cast, or use non-integral values

Done

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:86:
std::move(destroyed_callback_).Run(this);
On 2017/05/03 01:17:38, msw wrote:
> Why do you need to move the callback to run it?

I was requested to use base::OnceCallback and this is how it is called.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:90: const Extension* extension =
On 2017/05/03 01:17:38, msw wrote:
> nit: inline this in the return statement.

Done.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:100: const Extension* extension =
GetExtension();
On 2017/05/03 01:17:38, msw wrote:
> nit: inline in call below

It is used twice below, extension, IconsInfo::GetIcons(extension)

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:115: const bool enabled =
util::IsAppLaunchable(app_id_, context_);
On 2017/05/03 01:17:37, msw wrote:
> nit: inline in conditional

Done.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:24: class IconImage;
On 2017/05/03 01:17:38, msw wrote:
> nit: not needed; defined in extension_icon_image.h

Done.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:31: class ChromeAppIcon : public
IconImage::Observer {
On 2017/05/03 01:17:38, msw wrote:
> If each ChromeAppIcon is owned by the transient UI displaying the icon (app
list
> items, app list search results, app info dialog header... each of which might
be
> requesting a different size_in_dip anyway), and then destroyed when that
> transient UI goes away, is it worthwhile to have a centralized cache at all?
How
> often is the icon actually re-used between these, instead of destroyed and
then
> reloaded over and over? Perhaps it would make sense to just refine
AppIconLoader
> instead of adding the new service? Also, why doesn't the shelf use these
icons?

Having cache is actually good idea. I have this idea in mind. Once we have
centralized service we can used it as a base for this. It can be task to do.
However don't want to complicate this already huge CL.

>> Also, why doesn't the shelf use these icons?
Actually does. Shelf uses ExtensionAppIconLoader (renamed here to
ChromeAppIconLoader)

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:45: // Returns true if icon is
loaded and attached to valid extension.
On 2017/05/03 01:17:38, msw wrote:
> What does it mean for the icon to be attached to a valid extension? I don't
see
> how that's checked in the impl.

When extension is disabled, IconImage discards this extension and cannot longer
perform updates.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:48: void UpdateIcon();
On 2017/05/03 01:17:38, msw wrote:
> nit: comment; maybe allude to how UpdateIcon differs from Reload?

Done.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:51: void
EnsureRepsForSupportedScales();
On 2017/05/03 01:17:38, msw wrote:
> nit: remove this, let the one non-test caller
(ChromeAppIconLoader::FetchImage)
> do image_skia().EnsureRepsForSupportedScales() instead.

image_skia() from this class is static image with applied effects. It does not
refer to icon_. EnsureRepsForSupportedScales on this image_skia won't trigger
loading actual images.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:55: content::BrowserContext*
context() { return context_; }
On 2017/05/03 01:17:38, msw wrote:
> Remove this if it's not used anywhere, otherwise rename browser_context()

Done.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:65: content::BrowserContext* const
context_;
On 2017/05/03 01:17:38, msw wrote:
> nit: browser_context_

Done.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:67: DestroyedCallback
destroyed_callback_;
On 2017/05/03 01:17:38, msw wrote:
> nit: comment, describe when this is called (when the ChromeAppIcon is
destroyed?
> when the IconImage is destroyed?). Would it be sufficient to instead vend
> WeakPtr<ChromeAppIcon>s and store those in the
ChromeAppIconService::icon_map_? 

Somebody needs to clean the icon from icon_map_, right? so we have to notify
service somehow. Initially I had here delegate and using callback was preference
of previous reviewer.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:71: const int resource_size_in_dip_;
On 2017/05/03 01:17:38, msw wrote:
> Would it make sense to just expose the underlying value in IconImage, and
remove
> this redundant value?

resource_size_in_dip_ is actually used to create/recreate IconImage.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:76: std::unique_ptr<IconImage>
icon_;
On 2017/05/03 01:17:38, msw wrote:
> How does this IconImage |icon_|, which contains an ImageSkia, relate to the
> ImageSkia |image_skia_| above? Are they the same? If so, maybe remove
> |image_skia_| and just add an accessor for icon_->image_skia() if needed.
> Otherwise, maybe explain in comments here how they are different.
Done in comment

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_delegate.h (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_delegate.h:14: // Invoked when
ChromeAppIcon is updated. "icon->image_skia()" contains
On 2017/05/03 01:17:39, msw wrote:
> nit: use vertical bars instead of quotes

Done

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_delegate.h:15: // the current icon
image.
On 2017/05/03 01:17:39, msw wrote:
> Does current mean the post-update current, or does it mean the prior-to-update
> current? Perhaps say "new"/"updated" or "old" instead of current.

Done.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_loader.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.cc:54:
ChromeAppIconService::Get(profile())->CreateIcon(this, id, icon_size());
On 2017/05/03 01:17:39, msw wrote:
> IT seems like the service does most of the actual loading, should
> ChromeAppIconService inherit from AppIconLoader and then we could just remove
> this class?

We still have ArcAppIconLoader which differs from this class conceptually and
base AppIconLoader to provide independent interface.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.cc:76:
delegate()->OnAppImageUpdated(icon->app_id(), icon->image_skia());
On 2017/05/03 01:17:39, msw wrote:
> It's unfortunate that we have to go through two delegates here (this and
> delegate())

This is legacy implementation and ArcAppIconLoader uses different scheme. Bug
agree, looks kind of redundant.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_loader.h (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.h:21: // ImageLoader to load
images.
On 2017/05/03 01:17:39, msw wrote:
> What is ImageLoader? I don't see that referenced here or in the cc file.

Good catch, left from previous impl.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.h:25: int icon_size,
On 2017/05/03 01:17:39, msw wrote:
> nit: icon_size_in_dips?

Done.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.h:36: using
ExtensionIDToImageMap =
On 2017/05/03 01:17:39, msw wrote:
> nit: s/Image/ChromeAppIcon/? Let's avoid mixing the names where possible.

Done

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_loader.h:42: // Maps from extension id
to IconImage pointer.
On 2017/05/03 01:17:39, msw wrote:
> nit: s/IconImage pointer/ChromeAppIcon/?

Done.

[khmel, 2017-05-05 16:37:28]

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

[commit-bot: I haz the power, 2017-05-05 16:37:50]

Dry run: CQ is trying da patch.

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

[khmel, 2017-05-05 16:38:12]

Rebased code.

Mike, gentle ping.
Thanks!

[commit-bot: I haz the power, 2017-05-05 17:55:23]

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

[commit-bot: I haz the power, 2017-05-05 17:55:24]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-08 19:59:54]

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/gfx_utils.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/gfx_utils.cc:97: // Used in unit tests.
On 2017/05/03 02:15:21, khmel wrote:
> On 2017/05/03 01:17:37, msw wrote:
> > Could unit tests just use one of the above entries?
> 
> ARC test package name is defined here:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_t...
> 
> emfkafnhnpcmabnnkckkchdilgeoekbo is an extension in chrome/test.
> 
> Technically it is possible to do but this will require additional code change,
> including existing unit tests were extension id are predefined and number of
> tests is also predefined. Actually that was my intention initially to do like
> this. But changes were big enough.
> If you insist on removing this I can do it in separate issue in order not to
> grow this cl.
> 

This is fine, at least for now, especially if the alternatives are more
complex...

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_system_provider/notification_manager.cc
(right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/file_system_provider/notification_manager.cc:58:
icon_loader_(
On 2017/05/03 02:15:21, khmel wrote:
> On 2017/05/03 01:17:37, msw wrote:
> > optional nit/aside: icon_loader_ need not be a std::unique_ptr
> 
> If you don't mind I won't change. This change is just mechanical change due
> renaming ExtensionAppIconLoader to ChromeAppIconLoader.

That's fine, it's tangential to your change.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:32: class RoundedCornersImageSource
: public gfx::CanvasImageSource {
On 2017/05/03 02:15:21, khmel wrote:
> On 2017/05/03 01:17:38, msw wrote:
> > Can we avoid subclassing CanvasImageSource and just use a function to
generate
> > an ImageSkia with rounded corners (defined by a mask or premultiplied alpha
> > values built into the ImageSkia)?
> 
> Would TODO here be acceptable? That was legacy code and I would prefer to do
it
> separately.

Yes, since you are just moving this code, it's fine as-is, a TODO is optional.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:86:
std::move(destroyed_callback_).Run(this);
On 2017/05/03 02:15:21, khmel wrote:
> On 2017/05/03 01:17:38, msw wrote:
> > Why do you need to move the callback to run it?
> 
> I was requested to use base::OnceCallback and this is how it is called.

Acknowledged.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:31: class ChromeAppIcon : public
IconImage::Observer {
On 2017/05/03 02:15:22, khmel wrote:
> On 2017/05/03 01:17:38, msw wrote:
> > If each ChromeAppIcon is owned by the transient UI displaying the icon (app
> list
> > items, app list search results, app info dialog header... each of which
might
> be
> > requesting a different size_in_dip anyway), and then destroyed when that
> > transient UI goes away, is it worthwhile to have a centralized cache at all?
> How
> > often is the icon actually re-used between these, instead of destroyed and
> then
> > reloaded over and over? Perhaps it would make sense to just refine
> AppIconLoader
> > instead of adding the new service? Also, why doesn't the shelf use these
> icons?
> 
> Having cache is actually good idea. I have this idea in mind. Once we have
> centralized service we can used it as a base for this. It can be task to do.
> However don't want to complicate this already huge CL.
> 
> >> Also, why doesn't the shelf use these icons?
> Actually does. Shelf uses ExtensionAppIconLoader (renamed here to
> ChromeAppIconLoader)

Sorry, I don't think my comment was clear. I think this code is strange, because
(AFAICT) it only "caches" the icons in the service when they are alive and owned
and being shown by (mostly) temporary UI. The icons aren't owned by the service,
and they are created each time some UI needs to show them (ie. app list), and
then destroyed as soon as that UI is hidden. Is that correct? Even if the
service could cache the icons, I don't think that would be a good idea, since
that just seems like a waste of memory. Can you measure and describe the
benefits to the centralized "caching", instead of just loading the icons and
adornments for each bit of UI as needed? My guess is that there's a simpler
solution that would only lose marginal efficiency (re-loading an app icon for
the app list that's already pinned on the shelf).

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:45: // Returns true if icon is
loaded and attached to valid extension.
On 2017/05/03 02:15:22, khmel wrote:
> On 2017/05/03 01:17:38, msw wrote:
> > What does it mean for the icon to be attached to a valid extension? I don't
> see
> > how that's checked in the impl.
> 
> When extension is disabled, IconImage discards this extension and cannot
longer
> perform updates.

So the icon becomes invalid? Please clarify the comment to mention the extension
being disabled or similar? "attached" is odd

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:67: DestroyedCallback
destroyed_callback_;
On 2017/05/03 02:15:22, khmel wrote:
> On 2017/05/03 01:17:38, msw wrote:
> > nit: comment, describe when this is called (when the ChromeAppIcon is
> destroyed?
> > when the IconImage is destroyed?). Would it be sufficient to instead vend
> > WeakPtr<ChromeAppIcon>s and store those in the
> ChromeAppIconService::icon_map_? 
> 
> Somebody needs to clean the icon from icon_map_, right? so we have to notify
> service somehow. Initially I had here delegate and using callback was
preference
> of previous reviewer.

I suppose that's fine. Holding weak pointers also seems acceptable.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:93: void ChromeAppIcon::Reload() {
nit: function definition order should match declaration order.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:30: class ChromeAppIcon : public
IconImage::Observer {
I'd be very interested to see if all this refactoring could be encapsulated
within extensions::IconImage and ExtensionAppIconLoader/ChromeAppIconLoader
without adding ChromeAppIcon/ChromeAppIconService/ChromeAppIconService::Updater
/ChromeAppIconServiceFactory/ChromeAppIconDelegate. Perhaps additional pieces
could be added (if needed) across multiple CLs? I worry that the added class
complexity and division of responsibilities and inter-relationships here is
detrimental to the long-term maintainability of this code; I certainly don't
find this code easy to understand or review. I'm likely to defer approval to
other reviewers/owners, as I can't conceptualize the need for the added
complexity here. On the bright side, I do appreciate some of the simplification
of the various ChromeAppIcon* clients.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:52: void
EnsureRepsForSupportedScales();
It seems like this is only called in tests; if so add "ForTesting". It also
seems like ChromeAppIconLoader::FetchImage does the same thing when initially
loading the image, so I wonder if this is necessary at all?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_delegate.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_delegate.h:12: class
ChromeAppIconDelegate {
Why is this needed? Why not just pass the AppIconLoaderDelegate from
ChromeAppIconLoader to ChromeAppIcon and have that call OnAppImageUpdated
directly, instead of introducing an intermediary delegate interface?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.cc:21: class
ChromeAppIconService::Updater : public LauncherAppUpdater::Delegate {
Add a class comment if you keep this around.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.cc:30: void
OnAppUpdated(content::BrowserContext* browser_context,
Is this not already accomplished by ChromeAppIconService's
ExtensionRegistryObserver functionality? When is an app updated without
notifying extension registry observers? If this is needed in addition to that,
why not simply make ChromeAppIconService also inherit from this interface,
rather than creating a separate class to

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.cc:38:
LauncherExtensionAppUpdater extension_app_updater_;
What is this member used for? It's instantiated but not accessed, and that class
has no comments. It seems to be an ExtensionRegistryObserver (like
ChromeAppIconService), so I assume it's filling in some gaps there (which should
probably just be implemented directly on ChromeAppIconService). It's also an
ArcAppListPrefs::Observer, which I assume is observing arc app state changes
similar to the extension app state changes (perhaps that too should be folded
into ChromeAppIconService).

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:32: class
ChromeAppIconService : public KeyedService,
It would be very nice if this functionality (updating icon with extension state
changes) could be folded into ChromeAppIconLoader (or vice versa?), instead of
adding another class and splitting up the functionality.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:54: using IconMap =
std::map<std::string, std::set<ChromeAppIcon*>>;
nit: comment on why each app would have a set of icons, rather than just one
icon per app.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:65: 
nit: remove blank line

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:74: std::unique_ptr<Updater>
app_updater_;
nit: add a comment

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service_factory.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service_factory.cc:30:
DependsOn(ExtensionRegistryFactory::GetInstance());
Does this also depend on some similar arc component?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:62: void WaitIconUpdated()
{
nit: WaitForIconUpdates

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:74: if
(!icon_updated_callback_.is_null())
nit: combine nested ifs

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:111: bool
IsBlankImage(const gfx::ImageSkia& image) {
Can you just call image->isNull() || image->bitmap()->drawsNothing() instead?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:130: bool
IsGrayscaleImage(const gfx::ImageSkia& image) {
Can you just check the bitmap's SkColorType instead?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:146: bool AreEqual(const
gfx::ImageSkia& image1, const gfx::ImageSkia& image2) {
nit: all callers are accessing the ImageSkia within an existing gfx::Image for
the first argument, could you perhaps take a gfx::Image for the first argument
to avoid reconstructing one locally here?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:170: void
WaitIconUpdated(const T& item) {
nit: WaitForIconUpdates

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:176: #endif
nit: blank line before and // defined(OS_CHROMEOS)

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:211:
EXPECT_TRUE(reference_icon.image_skia().HasRepresentation(1.0f));
nit: remove this if it's redundant with line 206

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:331:
TEST_F(ChromeAppIconWithModelTest, ChromeBadging) {
Should this just be a ChromeAppIconTest? I don't see any use of the
ChromeAppIconWithModelTest members.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:360: #endif
nit: // defined(OS_CHROMEOS), optionally put a blank line above this and after
256

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:143: // OnPackageRemoved is
called for each active package with |uninstalled| is
nit: "with |uninstalled| set", (no "is")

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:314:
std::vector<std::string> GetPackagesFromPrefs(bool check_arc_alive,
nit: document |check_arc_alive| in the function comment.

[khmel, 2017-05-09 00:05:06]

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/gfx_utils.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/gfx_utils.cc:97: // Used in unit tests.
On 2017/05/08 19:59:53, msw wrote:
> On 2017/05/03 02:15:21, khmel wrote:
> > On 2017/05/03 01:17:37, msw wrote:
> > > Could unit tests just use one of the above entries?
> > 
> > ARC test package name is defined here:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_t...
> > 
> > emfkafnhnpcmabnnkckkchdilgeoekbo is an extension in chrome/test.
> > 
> > Technically it is possible to do but this will require additional code
change,
> > including existing unit tests were extension id are predefined and number of
> > tests is also predefined. Actually that was my intention initially to do
like
> > this. But changes were big enough.
> > If you insist on removing this I can do it in separate issue in order not to
> > grow this cl.
> > 
> 
> This is fine, at least for now, especially if the alternatives are more
> complex...

Acknowledged.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_system_provider/notification_manager.cc
(right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/file_system_provider/notification_manager.cc:58:
icon_loader_(
On 2017/05/08 19:59:53, msw wrote:
> On 2017/05/03 02:15:21, khmel wrote:
> > On 2017/05/03 01:17:37, msw wrote:
> > > optional nit/aside: icon_loader_ need not be a std::unique_ptr
> > 
> > If you don't mind I won't change. This change is just mechanical change due
> > renaming ExtensionAppIconLoader to ChromeAppIconLoader.
> 
> That's fine, it's tangential to your change.

Acknowledged.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:32: class RoundedCornersImageSource
: public gfx::CanvasImageSource {
On 2017/05/08 19:59:53, msw wrote:
> On 2017/05/03 02:15:21, khmel wrote:
> > On 2017/05/03 01:17:38, msw wrote:
> > > Can we avoid subclassing CanvasImageSource and just use a function to
> generate
> > > an ImageSkia with rounded corners (defined by a mask or premultiplied
alpha
> > > values built into the ImageSkia)?
> > 
> > Would TODO here be acceptable? That was legacy code and I would prefer to do
> it
> > separately.
> 
> Yes, since you are just moving this code, it's fine as-is, a TODO is optional.

TODO added.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:31: class ChromeAppIcon : public
IconImage::Observer {
On 2017/05/08 19:59:53, msw wrote:
> On 2017/05/03 02:15:22, khmel wrote:
> > On 2017/05/03 01:17:38, msw wrote:
> > > If each ChromeAppIcon is owned by the transient UI displaying the icon
(app
> > list
> > > items, app list search results, app info dialog header... each of which
> might
> > be
> > > requesting a different size_in_dip anyway), and then destroyed when that
> > > transient UI goes away, is it worthwhile to have a centralized cache at
all?
> > How
> > > often is the icon actually re-used between these, instead of destroyed and
> > then
> > > reloaded over and over? Perhaps it would make sense to just refine
> > AppIconLoader
> > > instead of adding the new service? Also, why doesn't the shelf use these
> > icons?
> > 
> > Having cache is actually good idea. I have this idea in mind. Once we have
> > centralized service we can used it as a base for this. It can be task to do.
> > However don't want to complicate this already huge CL.
> > 
> > >> Also, why doesn't the shelf use these icons?
> > Actually does. Shelf uses ExtensionAppIconLoader (renamed here to
> > ChromeAppIconLoader)
> 
> Sorry, I don't think my comment was clear. I think this code is strange,
because
> (AFAICT) it only "caches" the icons in the service when they are alive and
owned
> and being shown by (mostly) temporary UI. The icons aren't owned by the
service,
> and they are created each time some UI needs to show them (ie. app list), and
> then destroyed as soon as that UI is hidden. Is that correct? 
Yes, this is correct. Service does not own icon. It just tracks created icon and
updates them once needed. Once icon is not longer used in UI it gets destroyed
and service no longer tracks it.

> Even if the
> service could cache the icons, I don't think that would be a good idea, since
> that just seems like a waste of memory. Can you measure and describe the
> benefits to the centralized "caching", instead of just loading the icons and
> adornments for each bit of UI as needed? My guess is that there's a simpler
> solution that would only lose marginal efficiency (re-loading an app icon for
> the app list that's already pinned on the shelf).
My idea is similar what you suggest here. To have something like ref-counter.
if icon is used in multiple places. However it is just idea and I don't plan to
do this in nearest time.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:45: // Returns true if icon is
loaded and attached to valid extension.
On 2017/05/08 19:59:53, msw wrote:
> On 2017/05/03 02:15:22, khmel wrote:
> > On 2017/05/03 01:17:38, msw wrote:
> > > What does it mean for the icon to be attached to a valid extension? I
don't
> > see
> > > how that's checked in the impl.
> > 
> > When extension is disabled, IconImage discards this extension and cannot
> longer
> > perform updates.
> 
> So the icon becomes invalid? Please clarify the comment to mention the
extension
> being disabled or similar? "attached" is odd

In recent revision we creates icon_ at CTOR. That means we can drop 'is loaded'.
Tried to re-phrase for better understanding.

https://codereview.chromium.org/2819413003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:67: DestroyedCallback
destroyed_callback_;
On 2017/05/08 19:59:53, msw wrote:
> On 2017/05/03 02:15:22, khmel wrote:
> > On 2017/05/03 01:17:38, msw wrote:
> > > nit: comment, describe when this is called (when the ChromeAppIcon is
> > destroyed?
> > > when the IconImage is destroyed?). Would it be sufficient to instead vend
> > > WeakPtr<ChromeAppIcon>s and store those in the
> > ChromeAppIconService::icon_map_? 
> > 
> > Somebody needs to clean the icon from icon_map_, right? so we have to notify
> > service somehow. Initially I had here delegate and using callback was
> preference
> > of previous reviewer.
> 
> I suppose that's fine. Holding weak pointers also seems acceptable.

Acknowledged.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.cc:93: void ChromeAppIcon::Reload() {
On 2017/05/08 19:59:53, msw wrote:
> nit: function definition order should match declaration order.

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:30: class ChromeAppIcon : public
IconImage::Observer {
On 2017/05/08 19:59:53, msw wrote:
> I'd be very interested to see if all this refactoring could be encapsulated
> within extensions::IconImage and ExtensionAppIconLoader/ChromeAppIconLoader
> without adding
ChromeAppIcon/ChromeAppIconService/ChromeAppIconService::Updater
> /ChromeAppIconServiceFactory/ChromeAppIconDelegate. Perhaps additional pieces
> could be added (if needed) across multiple CLs? I worry that the added class
> complexity and division of responsibilities and inter-relationships here is
> detrimental to the long-term maintainability of this code; I certainly don't
> find this code easy to understand or review. I'm likely to defer approval to
> other reviewers/owners, as I can't conceptualize the need for the added
> complexity here. On the bright side, I do appreciate some of the
simplification
> of the various ChromeAppIcon* clients.

You might miss that but I already described reasons why I did so.

Let me summarize.
We have different using concept. Direct icon and via
Chrome(Extension)AppIconLoader. So that means we cannot encapsulate all
functionality in ChromeAppIconLoader because we have several consumers which
uses ChromeAppIcon directly. Solutions: 1) Move functionality to ChromeAppIcon
and 2) Replace ChromeAppIcon to ChromeAppIconLoader but for one icon. 2) is
de-factor the same as 1 because.
Having everything in ChromeAppIcon is bad because it has to observe extension
registry and handle all callbacks from ExtensionRegistry even most of callbacks
are for other extensions. Assume we have 100 icons. That requires the 100
listeners in extension registry. Now one event is sent for some extension. It
will be delivered to 100 handlers and only one has reason to handle each. Now
Extension Registry is quite busy system and it sends events quite frequently.
Next we have problem with ArcAppListPrefs. Each icon has to observe again,
messages from ArcAppListPrefs. Problems are the same as I described above. Next
problem is not obvious but I faced with it and it was not simple to solve. We
have different life cycles of Extension services and ArcAppLIstPrefs. That means
we have to implement additional observer to listen for ArcAppListPrefs shutdown
event. That was really hack code. That was my one of the my first
implementation. I very reluctantly added this new service. However it helps to
remove all drawback. Yes code is not trivial but alternative is worse.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:52: void
EnsureRepsForSupportedScales();
Actually you are right. I thought that once we applied effects we lost reference
to original ImageSkia. However both, RoundedCornersImageSource and
CreateHSLShiftedImage contains reference to original ImageSkia. I tested and it
works well. So it is possible to discard this code. Thanks!

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_delegate.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_delegate.h:12: class
ChromeAppIconDelegate {
On 2017/05/08 19:59:53, msw wrote:
> Why is this needed? Why not just pass the AppIconLoaderDelegate from
> ChromeAppIconLoader to ChromeAppIcon and have that call OnAppImageUpdated
> directly, instead of introducing an intermediary delegate interface?

AppIconLoaderDelegate is only once case how to use it. There are consumers that
use ChromeAppIcon directly. ChromeAppIconLoader is helper class for some but not
for all use cases.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.cc:21: class
ChromeAppIconService::Updater : public LauncherAppUpdater::Delegate {
On 2017/05/08 19:59:53, msw wrote:
> Add a class comment if you keep this around.

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.cc:30: void
OnAppUpdated(content::BrowserContext* browser_context,
On 2017/05/08 19:59:53, msw wrote:
> Is this not already accomplished by ChromeAppIconService's
> ExtensionRegistryObserver functionality? When is an app updated without
> notifying extension registry observers? If this is needed in addition to that,
> why not simply make ChromeAppIconService also inherit from this interface,
> rather than creating a separate class to 

No, LauncherExtensionAppUpdater in ChromeOS additionally listens for ARC app
events. Some ARC app may influence how extension based app looks like
(additional badging for Chrome app in case ARC has app that duplicates Chrome
app, for example Hangouts)
And this ChromeAppIconService::Updater is valid only for ChromeOS whenever 
ChromeAppIconService works on all platforms

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.cc:38:
LauncherExtensionAppUpdater extension_app_updater_;
On 2017/05/08 19:59:53, msw wrote:
> What is this member used for? It's instantiated but not accessed, and that
class
> has no comments. It seems to be an ExtensionRegistryObserver (like
> ChromeAppIconService), so I assume it's filling in some gaps there (which
should
> probably just be implemented directly on ChromeAppIconService). It's also an
> ArcAppListPrefs::Observer, which I assume is observing arc app state changes
> similar to the extension app state changes (perhaps that too should be folded
> into ChromeAppIconService).


It used like: extension_app_updater_(this, context) 
Where this is LauncherAppUpdater::Delegate

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:32: class
ChromeAppIconService : public KeyedService,
On 2017/05/08 19:59:53, msw wrote:
> It would be very nice if this functionality (updating icon with extension
state
> changes) could be folded into ChromeAppIconLoader (or vice versa?), instead of
> adding another class and splitting up the functionality.

I described in another comments. ChromeAppIconLoader is optional class. Please
read more details at another comment

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:54: using IconMap =
std::map<std::string, std::set<ChromeAppIcon*>>;
On 2017/05/08 19:59:53, msw wrote:
> nit: comment on why each app would have a set of icons, rather than just one
> icon per app.

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:65: 
On 2017/05/08 19:59:54, msw wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:74: std::unique_ptr<Updater>
app_updater_;
On 2017/05/08 19:59:53, msw wrote:
> nit: add a comment

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service_factory.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service_factory.cc:30:
DependsOn(ExtensionRegistryFactory::GetInstance());
On 2017/05/08 19:59:54, msw wrote:
> Does this also depend on some similar arc component?

No, ChromeAppIconService may work without ArcAppListPrefs (actually
ArcAppListPrefs is optional  and does not exist in many cases, for example
secondary user profile)

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:62: void WaitIconUpdated()
{
On 2017/05/08 19:59:54, msw wrote:
> nit: WaitForIconUpdates

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:74: if
(!icon_updated_callback_.is_null())
On 2017/05/08 19:59:54, msw wrote:
> nit: combine nested ifs

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:111: bool
IsBlankImage(const gfx::ImageSkia& image) {
On 2017/05/08 19:59:54, msw wrote:
> Can you just call image->isNull() || image->bitmap()->drawsNothing() instead?

drawsNothing validates width/heights <= 0 but not if image is filled by empty
pixels (RGBA = 0) as for default Chrome app icon image.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:130: bool
IsGrayscaleImage(const gfx::ImageSkia& image) {
On 2017/05/08 19:59:54, msw wrote:
> Can you just check the bitmap's SkColorType instead?
IIUC CreateHSLShiftedImage does not change color type.
Also it has check
  DCHECK(bitmap.colorType() == kN32_SkColorType);

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:146: bool AreEqual(const
gfx::ImageSkia& image1, const gfx::ImageSkia& image2) {
On 2017/05/08 19:59:54, msw wrote:
> nit: all callers are accessing the ImageSkia within an existing gfx::Image for
> the first argument, could you perhaps take a gfx::Image for the first argument
> to avoid reconstructing one locally here?

TestAppIcon reference_icon is not gfx::ImageSkia. Or I did not get you?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:170: void
WaitIconUpdated(const T& item) {
On 2017/05/08 19:59:54, msw wrote:
> nit: WaitForIconUpdates

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:176: #endif
On 2017/05/08 19:59:54, msw wrote:
> nit: blank line before and // defined(OS_CHROMEOS)

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:211:
EXPECT_TRUE(reference_icon.image_skia().HasRepresentation(1.0f));
On 2017/05/08 19:59:54, msw wrote:
> nit: remove this if it's redundant with line 206

206 has EXPECT_FALSE here EXPECT_TRUE

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:331:
TEST_F(ChromeAppIconWithModelTest, ChromeBadging) {
On 2017/05/08 19:59:54, msw wrote:
> Should this just be a ChromeAppIconTest? I don't see any use of the
> ChromeAppIconWithModelTest members.

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:360: #endif
On 2017/05/08 19:59:54, msw wrote:
> nit: // defined(OS_CHROMEOS), optionally put a blank line above this and after
> 256

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:143: // OnPackageRemoved is
called for each active package with |uninstalled| is
On 2017/05/08 19:59:54, msw wrote:
> nit: "with |uninstalled| set", (no "is")

Done.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:314:
std::vector<std::string> GetPackagesFromPrefs(bool check_arc_alive,
On 2017/05/08 19:59:54, msw wrote:
> nit: document |check_arc_alive| in the function comment.

Done.

[msw, 2017-05-10 19:05:22]

Thanks for your patience with my feedback.
Please reconsider merging the service and loader.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:30: class ChromeAppIcon : public
IconImage::Observer {
On 2017/05/09 00:05:04, khmel wrote:
> On 2017/05/08 19:59:53, msw wrote:
> > I'd be very interested to see if all this refactoring could be encapsulated
> > within extensions::IconImage and ExtensionAppIconLoader/ChromeAppIconLoader
> > without adding
> ChromeAppIcon/ChromeAppIconService/ChromeAppIconService::Updater
> > /ChromeAppIconServiceFactory/ChromeAppIconDelegate. Perhaps additional
pieces
> > could be added (if needed) across multiple CLs? I worry that the added class
> > complexity and division of responsibilities and inter-relationships here is
> > detrimental to the long-term maintainability of this code; I certainly don't
> > find this code easy to understand or review. I'm likely to defer approval to
> > other reviewers/owners, as I can't conceptualize the need for the added
> > complexity here. On the bright side, I do appreciate some of the
> simplification
> > of the various ChromeAppIcon* clients.
> 
> You might miss that but I already described reasons why I did so.
> 
> Let me summarize.
> We have different using concept. Direct icon and via
> Chrome(Extension)AppIconLoader. So that means we cannot encapsulate all
> functionality in ChromeAppIconLoader because we have several consumers which
> uses ChromeAppIcon directly. Solutions: 1) Move functionality to ChromeAppIcon
> and 2) Replace ChromeAppIcon to ChromeAppIconLoader but for one icon. 2) is
> de-factor the same as 1 because.
> Having everything in ChromeAppIcon is bad because it has to observe extension
> registry and handle all callbacks from ExtensionRegistry even most of
callbacks
> are for other extensions. Assume we have 100 icons. That requires the 100
> listeners in extension registry. Now one event is sent for some extension. It
> will be delivered to 100 handlers and only one has reason to handle each. Now
> Extension Registry is quite busy system and it sends events quite frequently.
> Next we have problem with ArcAppListPrefs. Each icon has to observe again,
> messages from ArcAppListPrefs. Problems are the same as I described above.
Next
> problem is not obvious but I faced with it and it was not simple to solve. We
> have different life cycles of Extension services and ArcAppLIstPrefs. That
means
> we have to implement additional observer to listen for ArcAppListPrefs
shutdown
> event. That was really hack code. That was my one of the my first
> implementation. I very reluctantly added this new service. However it helps to
> remove all drawback. Yes code is not trivial but alternative is worse.

Having a single object to monitor extension changes for all icons seems
reasonable, and sound like a perfect job for the icon loader. I don't think we
should have both a loader *and* a new service with such similar logic and
responsibilities. Could you make AppIconLoader a singleton or
profile-keyed-service and move all the service code/logic into that class? That
should make it easier for clients to get updating icons without owning their own
loader. I'd also like to see extensions::IconImage merged with ChromeAppIcon,
but that seems a little less important. WDYT?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_delegate.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_delegate.h:12: class
ChromeAppIconDelegate {
On 2017/05/09 00:05:04, khmel wrote:
> On 2017/05/08 19:59:53, msw wrote:
> > Why is this needed? Why not just pass the AppIconLoaderDelegate from
> > ChromeAppIconLoader to ChromeAppIcon and have that call OnAppImageUpdated
> > directly, instead of introducing an intermediary delegate interface?
> 
> AppIconLoaderDelegate is only once case how to use it. There are consumers
that
> use ChromeAppIcon directly. ChromeAppIconLoader is helper class for some but
not
> for all use cases.

Right, but it's *so* similar (just one On[Icon|AppImage]Updated function), why
not just re-use the AppIconLoaderDelegate interface, even if you have to
maintain separate icon/loader and icon/service clients? The ExtensionAppItem,
ExtensionAppResult, and AppInfoHeaderPanel overrides for
AppIconLoaderDelegate::OnAppImageUpdated could even include a comment that says
"This is called by ChromeAppIconService when the ChromeAppIcon is updated, even
though this class doesn't directly use a ChromeAppIconLoader." (or similar) if
you feel that is necessary.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.cc:30: void
OnAppUpdated(content::BrowserContext* browser_context,
On 2017/05/09 00:05:04, khmel wrote:
> On 2017/05/08 19:59:53, msw wrote:
> > Is this not already accomplished by ChromeAppIconService's
> > ExtensionRegistryObserver functionality? When is an app updated without
> > notifying extension registry observers? If this is needed in addition to
that,
> > why not simply make ChromeAppIconService also inherit from this interface,
> > rather than creating a separate class to 
> 
> No, LauncherExtensionAppUpdater in ChromeOS additionally listens for ARC app
> events. Some ARC app may influence how extension based app looks like
> (additional badging for Chrome app in case ARC has app that duplicates Chrome
> app, for example Hangouts)
> And this ChromeAppIconService::Updater is valid only for ChromeOS whenever 
> ChromeAppIconService works on all platforms

Why not simply make ChromeAppIconService inherit from
LauncherAppUpdater::Delegate in a #if defined(OS_CHROMEOS) block?

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:146: bool AreEqual(const
gfx::ImageSkia& image1, const gfx::ImageSkia& image2) {
On 2017/05/09 00:05:05, khmel wrote:
> On 2017/05/08 19:59:54, msw wrote:
> > nit: all callers are accessing the ImageSkia within an existing gfx::Image
for
> > the first argument, could you perhaps take a gfx::Image for the first
argument
> > to avoid reconstructing one locally here?
> 
> TestAppIcon reference_icon is not gfx::ImageSkia. Or I did not get you?

Ah, nevermind.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:211:
EXPECT_TRUE(reference_icon.image_skia().HasRepresentation(1.0f));
On 2017/05/09 00:05:05, khmel wrote:
> On 2017/05/08 19:59:54, msw wrote:
> > nit: remove this if it's redundant with line 206
> 
> 206 has EXPECT_FALSE here EXPECT_TRUE

Oops, sorry about that!

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:55: // System may have
multiple icons for the same app id with the same of
nit: "same or" and "dimensions". (or just "multiple icons for the same app id
with different dimensions").

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:73: if
((icon_update_count_ == icon_update_count_expected_) &&
nit: extra parens not needed here or on the next line

[khmel, 2017-05-10 20:26:23]

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon.h:30: class ChromeAppIcon : public
IconImage::Observer {
On 2017/05/10 19:05:21, msw wrote:
> On 2017/05/09 00:05:04, khmel wrote:
> > On 2017/05/08 19:59:53, msw wrote:
> > > I'd be very interested to see if all this refactoring could be
encapsulated
> > > within extensions::IconImage and
ExtensionAppIconLoader/ChromeAppIconLoader
> > > without adding
> > ChromeAppIcon/ChromeAppIconService/ChromeAppIconService::Updater
> > > /ChromeAppIconServiceFactory/ChromeAppIconDelegate. Perhaps additional
> pieces
> > > could be added (if needed) across multiple CLs? I worry that the added
class
> > > complexity and division of responsibilities and inter-relationships here
is
> > > detrimental to the long-term maintainability of this code; I certainly
don't
> > > find this code easy to understand or review. I'm likely to defer approval
to
> > > other reviewers/owners, as I can't conceptualize the need for the added
> > > complexity here. On the bright side, I do appreciate some of the
> > simplification
> > > of the various ChromeAppIcon* clients.
> > 
> > You might miss that but I already described reasons why I did so.
> > 
> > Let me summarize.
> > We have different using concept. Direct icon and via
> > Chrome(Extension)AppIconLoader. So that means we cannot encapsulate all
> > functionality in ChromeAppIconLoader because we have several consumers which
> > uses ChromeAppIcon directly. Solutions: 1) Move functionality to
ChromeAppIcon
> > and 2) Replace ChromeAppIcon to ChromeAppIconLoader but for one icon. 2) is
> > de-factor the same as 1 because.
> > Having everything in ChromeAppIcon is bad because it has to observe
extension
> > registry and handle all callbacks from ExtensionRegistry even most of
> callbacks
> > are for other extensions. Assume we have 100 icons. That requires the 100
> > listeners in extension registry. Now one event is sent for some extension.
It
> > will be delivered to 100 handlers and only one has reason to handle each.
Now
> > Extension Registry is quite busy system and it sends events quite
frequently.
> > Next we have problem with ArcAppListPrefs. Each icon has to observe again,
> > messages from ArcAppListPrefs. Problems are the same as I described above.
> Next
> > problem is not obvious but I faced with it and it was not simple to solve.
We
> > have different life cycles of Extension services and ArcAppLIstPrefs. That
> means
> > we have to implement additional observer to listen for ArcAppListPrefs
> shutdown
> > event. That was really hack code. That was my one of the my first
> > implementation. I very reluctantly added this new service. However it helps
to
> > remove all drawback. Yes code is not trivial but alternative is worse.
> 
> Having a single object to monitor extension changes for all icons seems
> reasonable, and sound like a perfect job for the icon loader. I don't think we
> should have both a loader *and* a new service with such similar logic and
> responsibilities. Could you make AppIconLoader a singleton or
> profile-keyed-service and move all the service code/logic into that class?
That
> should make it easier for clients to get updating icons without owning their
own
> loader. I'd also like to see extensions::IconImage merged with ChromeAppIcon,
> but that seems a little less important. WDYT?

1. AppIconLoader is base class, we have ArcAppIconLoader and
ChromeAppIconLoader. 
2. AppIconLoader is per specific icon size.
3. Some codes use icon directly.

These 3 points makes it is impossible to achieve without big refactoring.
I am not stopping Icon work with current CL. I have some tasks for ARC icons to
refactor. Ideally I plan to get rid of AppIconLoader(s) and make everything
using ChromeAppIconService including ARC icons. So my next step looks very
similar to what you suggest (just ChromeAppIconService is AppIconLoader in your
terms).
Beside refactoring this CL fixes several real issues.

>> I'd also like to see extensions::IconImage merged with ChromeAppIcon,
Let do it as separate CL if you don't mind.

[khmel, 2017-05-11 02:13:48]

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_delegate.h (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_delegate.h:12: class
ChromeAppIconDelegate {
On 2017/05/10 19:05:21, msw wrote:
> On 2017/05/09 00:05:04, khmel wrote:
> > On 2017/05/08 19:59:53, msw wrote:
> > > Why is this needed? Why not just pass the AppIconLoaderDelegate from
> > > ChromeAppIconLoader to ChromeAppIcon and have that call OnAppImageUpdated
> > > directly, instead of introducing an intermediary delegate interface?
> > 
> > AppIconLoaderDelegate is only once case how to use it. There are consumers
> that
> > use ChromeAppIcon directly. ChromeAppIconLoader is helper class for some but
> not
> > for all use cases.
> 
> Right, but it's *so* similar (just one On[Icon|AppImage]Updated function), why
> not just re-use the AppIconLoaderDelegate interface, even if you have to
> maintain separate icon/loader and icon/service clients? The ExtensionAppItem,
> ExtensionAppResult, and AppInfoHeaderPanel overrides for
> AppIconLoaderDelegate::OnAppImageUpdated could even include a comment that
says
> "This is called by ChromeAppIconService when the ChromeAppIcon is updated,
even
> though this class doesn't directly use a ChromeAppIconLoader." (or similar) if
> you feel that is necessary.

As agreed, I will remove AppIconloader and from this point of view 
AppIconloaderDelegate will go away also. So IIUC it is better to leave this
file.

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service.cc (right):

https://codereview.chromium.org/2819413003/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.cc:30: void
OnAppUpdated(content::BrowserContext* browser_context,
On 2017/05/10 19:05:21, msw wrote:
> On 2017/05/09 00:05:04, khmel wrote:
> > On 2017/05/08 19:59:53, msw wrote:
> > > Is this not already accomplished by ChromeAppIconService's
> > > ExtensionRegistryObserver functionality? When is an app updated without
> > > notifying extension registry observers? If this is needed in addition to
> that,
> > > why not simply make ChromeAppIconService also inherit from this interface,
> > > rather than creating a separate class to 
> > 
> > No, LauncherExtensionAppUpdater in ChromeOS additionally listens for ARC app
> > events. Some ARC app may influence how extension based app looks like
> > (additional badging for Chrome app in case ARC has app that duplicates
Chrome
> > app, for example Hangouts)
> > And this ChromeAppIconService::Updater is valid only for ChromeOS whenever 
> > ChromeAppIconService works on all platforms
> 
> Why not simply make ChromeAppIconService inherit from
> LauncherAppUpdater::Delegate in a #if defined(OS_CHROMEOS) block?

I did not do this because introduce heavy logic in header. I don't have strong
preference so let do as you suggest.

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_service.h (right):

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:55: // System may have
multiple icons for the same app id with the same of
On 2017/05/10 19:05:21, msw wrote:
> nit: "same or" and "dimensions". (or just "multiple icons for the same app id
> with different dimensions").

Thanks for nice wording.

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_service.h:55: // System may have
multiple icons for the same app id with the same of
On 2017/05/10 19:05:21, msw wrote:
> nit: "same or" and "dimensions". (or just "multiple icons for the same app id
> with different dimensions").

Thanks for nice wording.

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:73: if
((icon_update_count_ == icon_update_count_expected_) &&
On 2017/05/10 19:05:21, msw wrote:
> nit: extra parens not needed here or on the next line

Done.

[khmel, 2017-05-11 16:46:36]

Hi Mike,

As we chatted offline AppIconLoader is subject to be merged to
ChromeAppIconService in next refactoring. Other your comments were addressed.

I rebased and retested with latest build.

PTAL

[msw, 2017-05-11 17:00:31]

shrug, lgtm

https://codereview.chromium.org/2819413003/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:73: if
((icon_update_count_ == icon_update_count_expected_) &&
I meant that the parentheses "()" are not needed around the equality statement
on this line, nor around the condition on the next line. The curly braces "{}"
are needed for multi-line conditionals. This should look like:
if (icon_update_count_ == icon_update_count_expected_ &&
    !icon_updated_callback_.is_null()) {
  base::ResetAndReturn(&icon_updated_callback_).Run();
}

[khmel, 2017-05-11 17:46:58]

Description was changed from

==========
Refactor extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ChromeAppIcon is introduced that applies all
necessary effects based on current environment and app type.
ChromeAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ChromeAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.
==========

to

==========
Refactor extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ChromeAppIcon is introduced that applies all
necessary effects based on current environment and app type.
ChromeAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ChromeAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.

TBR=yoshiki@chromium.org
==========

[khmel, 2017-05-11 17:47:46]

Thank you for review!

Added yoshiki as TBR for trivial change due rename in
chrome/browser/notifications/notifier_source.*

https://codereview.chromium.org/2819413003/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_app_icon_unittest.cc (right):

https://codereview.chromium.org/2819413003/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/chrome_app_icon_unittest.cc:73: if
((icon_update_count_ == icon_update_count_expected_) &&
On 2017/05/11 17:00:31, msw wrote:
> I meant that the parentheses "()" are not needed around the equality statement
> on this line, nor around the condition on the next line. The curly braces "{}"
> are needed for multi-line conditionals. This should look like:
> if (icon_update_count_ == icon_update_count_expected_ &&
>     !icon_updated_callback_.is_null()) {
>   base::ResetAndReturn(&icon_updated_callback_).Run();
> }

Sorry, misunderstood you.

[khmel, 2017-05-11 17:48:06]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-05-11 17:48:07]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rdevlin.cronin@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2819413003/#ps400001
(title: "nit")

[commit-bot: I haz the power, 2017-05-11 17:49:52]

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-05-11 19:14:55]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1494524886633900,
"parent_rev": "29a9efd8a6d120ebb9752d4e9166fc9447df4789", "commit_rev":
"d65b50265fed9520358bd48ee117ba7767a6eae1"}

[commit-bot: I haz the power, 2017-05-11 19:15:16]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1494524886633900,
"parent_rev": "d348b44325689e6d2ecf5ba9896ed26ec68ee97d", "commit_rev":
"8c1f6622f0279ee6062496672a7b83317b579486"}

[commit-bot: I haz the power, 2017-05-11 19:15:34]

Description was changed from

==========
Refactor extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ChromeAppIcon is introduced that applies all
necessary effects based on current environment and app type.
ChromeAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ChromeAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.

TBR=yoshiki@chromium.org
==========

to

==========
Refactor extension app icon.

We have several independent places where we decided how extension icon
is represented. We have separate logic for the shelf, recent view, app
list and app info icons. Icon look depends on type of app and some
external conditions. This leads to situation that extension App icon
looks differently in different places. Yet another problem is non-stable
applying Chrome badging in case corresponded ARC app is installed or
uninstalled.

Solution is to provide centrilized place where app icon is processed
and all consumers should use. ChromeAppIcon is introduced that applies all
necessary effects based on current environment and app type.
ChromeAppIconService is introduced to refer all created icons,
monitor environment changes and updating the icon. Resulting icon
image is sent to consumer via ChromeAppIconDelegate interface.

BUG=712729
BUG=703366
BUG=b/37418449
Test=Manually on device. Existing tests passed.

TBR=yoshiki@chromium.org

Review-Url: https://codereview.chromium.org/2819413003
Cr-Commit-Position: refs/heads/master@{#471035}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c1f6622f0279ee6062496672a7b...
==========

[commit-bot: I haz the power, 2017-05-11 19:15:37]

Committed patchset #17 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/8c1f6622f0279ee6062496672a7b...