arc: Prevent overwriting of the Chrome Play Store icon.

arc: Prevent overwriting of the Chrome  Play Store icon.

This implement correct access to ARC icon from notification manager and
from ARC kiosk handler. Incorrect access to the Play Store icon caused
request to extract this app icon from ARC side and which finally
overwrote Chrome Play Store icon, which was not expected.

TEST=Manually, Ensure that Play Store notification icon (in 
     notification settings) correctly loaded with Chrome version of
     Play Store. Also checked arc.app icon cache to confirm that
     Android version of Play Store icon did not exist there.
BUG=719118
Review-Url: https://codereview.chromium.org/2865813002
Cr-Commit-Position: refs/heads/master@{#470058}
Committed: https://chromium.googlesource.com/chromium/src/+/832d8b7c8aff004eb7480fd0d20ce0508b24d7f4

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

Description was changed from

==========
arc: Prevent overwriting of the Chrome  Play Store icon.

This implement correct access to ARC icon from notification manager and
from ARC kiosk handler. Incorrect access to the Play Store icon caused
request to extract this app icon from ARC side and which finally
overwrote Chrome Play Store icon, which was not expected.

TEST=Manually, Ensure that Play Store notification icon (in noticiation
     settings) correctly loaded with Chrome version of Play Store. Also
     checked arc.app icon cache to confirm that Android version of Play
     Store icon did not exist.
BUG=719118
==========

to

==========
arc: Prevent overwriting of the Chrome  Play Store icon.

This implement correct access to ARC icon from notification manager and
from ARC kiosk handler. Incorrect access to the Play Store icon caused
request to extract this app icon from ARC side and which finally
overwrote Chrome Play Store icon, which was not expected.

TEST=Manually, Ensure that Play Store notification icon (in 
     notification settings) correctly loaded with Chrome version of
     Play Store. Also checked arc.app icon cache to confirm that
     Android version of Play Store icon did not exist there.
BUG=719118
==========

[khmel, 2017-05-06 01:03:35]

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

[khmel, 2017-05-06 01:03:36]

Hi Xiyuan and Yoshiki

PTAL

Thanks!

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

https://codereview.chromium.org/2865813002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:255: image_ =
gfx::Image(image_skia_);
This fixes yet another problem, when in case somebody access ARC icon as image()
it does not contain default image (for Play Store icon this is Chrome version of
Play Store).

https://codereview.chromium.org/2865813002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:261: void
ArcAppIcon::LoadForScaleFactor(ui::ScaleFactor scale_factor) {
LoadForScaleFactor is internal ARC Icon function and it is not intended to be
used by ordinary consumer. Only ArcAppIconLoader and ArcModelBuilder has right
to call this. Agree that looks bit confusing. I will try to refactor this
probably similar as my current refactoring for extension icons).

[khmel, 2017-05-06 01:03:43]

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

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

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-06 01:37:25]

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

[commit-bot: I haz the power, 2017-05-06 01:37:25]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-08 16:15:13]

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

https://codereview.chromium.org/2865813002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:255: image_ =
gfx::Image(image_skia_);
On 2017/05/06 01:03:36, khmel wrote:
> This fixes yet another problem, when in case somebody access ARC icon as
image()
> it does not contain default image (for Play Store icon this is Chrome version
of
> Play Store).

Get rid of this member and make image() to do the construction when it is called
? Or make the caller to do this by themselves?

https://codereview.chromium.org/2865813002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:261: void
ArcAppIcon::LoadForScaleFactor(ui::ScaleFactor scale_factor) {
On 2017/05/06 01:03:36, khmel wrote:
> LoadForScaleFactor is internal ARC Icon function and it is not intended to be
> used by ordinary consumer. Only ArcAppIconLoader and ArcModelBuilder has right
> to call this. Agree that looks bit confusing. I will try to refactor this
> probably similar as my current refactoring for extension icons).

Make this private and make ArcAppIconLoader/ArcModelBuilder friend to limit the
access ?

[khmel, 2017-05-08 16:36:07]

Thanks for comments,

PTAL updated version

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

https://codereview.chromium.org/2865813002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:255: image_ =
gfx::Image(image_skia_);
On 2017/05/08 16:15:13, xiyuan wrote:
> On 2017/05/06 01:03:36, khmel wrote:
> > This fixes yet another problem, when in case somebody access ARC icon as
> image()
> > it does not contain default image (for Play Store icon this is Chrome
version
> of
> > Play Store).
> 
> Get rid of this member and make image() to do the construction when it is
called
> ? Or make the caller to do this by themselves?

Yes, it is rather confusing than helpful.
Discarded image()

https://codereview.chromium.org/2865813002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:261: void
ArcAppIcon::LoadForScaleFactor(ui::ScaleFactor scale_factor) {
On 2017/05/08 16:15:13, xiyuan wrote:
> On 2017/05/06 01:03:36, khmel wrote:
> > LoadForScaleFactor is internal ARC Icon function and it is not intended to
be
> > used by ordinary consumer. Only ArcAppIconLoader and ArcModelBuilder has
right
> > to call this. Agree that looks bit confusing. I will try to refactor this
> > probably similar as my current refactoring for extension icons).
> 
> Make this private and make ArcAppIconLoader/ArcModelBuilder friend to limit
the
> access ?

Done.

[xiyuan, 2017-05-08 16:47:37]

lgtm

[yoshiki, 2017-05-08 16:59:59]

lgtm

[khmel, 2017-05-08 17:48:03]

Thank you for review!

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

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-05-08 17:48:49]

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-08 18:38:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494265691058080,
"parent_rev": "d50d8466d0bdae0b5ae8af4108ecaa1376ed5f21", "commit_rev":
"832d8b7c8aff004eb7480fd0d20ce0508b24d7f4"}

[commit-bot: I haz the power, 2017-05-08 18:38:26]

Description was changed from

==========
arc: Prevent overwriting of the Chrome  Play Store icon.

This implement correct access to ARC icon from notification manager and
from ARC kiosk handler. Incorrect access to the Play Store icon caused
request to extract this app icon from ARC side and which finally
overwrote Chrome Play Store icon, which was not expected.

TEST=Manually, Ensure that Play Store notification icon (in 
     notification settings) correctly loaded with Chrome version of
     Play Store. Also checked arc.app icon cache to confirm that
     Android version of Play Store icon did not exist there.
BUG=719118
==========

to

==========
arc: Prevent overwriting of the Chrome  Play Store icon.

This implement correct access to ARC icon from notification manager and
from ARC kiosk handler. Incorrect access to the Play Store icon caused
request to extract this app icon from ARC side and which finally
overwrote Chrome Play Store icon, which was not expected.

TEST=Manually, Ensure that Play Store notification icon (in 
     notification settings) correctly loaded with Chrome version of
     Play Store. Also checked arc.app icon cache to confirm that
     Android version of Play Store icon did not exist there.
BUG=719118

Review-Url: https://codereview.chromium.org/2865813002
Cr-Commit-Position: refs/heads/master@{#470058}
Committed:
https://chromium.googlesource.com/chromium/src/+/832d8b7c8aff004eb7480fd0d20c...
==========

[commit-bot: I haz the power, 2017-05-08 18:38:27]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/832d8b7c8aff004eb7480fd0d20c...