arc: Make Play Store item persistance in shelf.

arc: Make Play Store item persistance in shelf.

This also shares item with OptIn UI.

BUG=b/28485354
TEST=Manually on device. Play Store icon exists always on shelf.
     when Arc is not OptedIn, clicking this icon opens OptIn UI
     and this shelf icon behaives as ordinal Platform app. Clicking
     on it minimizes/shows UI. Play Store is automatically started
     on OptIn flow finishes. Once Play Store started, there is no
     additional icon for Android app. It reuses the same shelf
     item. Menu in all cases does not have pin/unpin option. If
     Chromebook is opted in already, this just starts/activates
     PlayStore window and no additional item in shelf is created.

Committed: https://crrev.com/18cc6b017a26f267792161bdb150eea81f2fecb4
Cr-Commit-Position: refs/heads/master@{#394444}

[khmel, 2016-05-11 18:11:36]

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

[khmel, 2016-05-11 18:11:37]

Hi Stefan and Xiyuan,

PTAL

[xiyuan, 2016-05-11 22:10:08]

lgtm

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h:36:
ChromeLauncherController* controller() { return chrome_launcher_controller_; }
nit: move this with other accessors, around line 60

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:10:
#include "ash/shelf/shelf_item_delegate.h"
nit: not needed since we include app_shortcut_launcher_item_controller.h?

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:19:
class ShelfMenuModel;
nit: not needed?

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:23:
class Event;
nit: not needed?

[khmel, 2016-05-12 18:28:05]

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

[khmel, 2016-05-12 18:28:06]

Thanks Xiyuan for review. Nits were addressed.

Adding oshima@ to review (min c/b/ui/ash/chrome_launcher_prefs.cc)

PTAL

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h:36:
ChromeLauncherController* controller() { return chrome_launcher_controller_; }
On 2016/05/11 22:10:08, xiyuan wrote:
> nit: move this with other accessors, around line 60

Done.

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:10:
#include "ash/shelf/shelf_item_delegate.h"
On 2016/05/11 22:10:08, xiyuan wrote:
> nit: not needed since we include app_shortcut_launcher_item_controller.h?

Done.

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:19:
class ShelfMenuModel;
On 2016/05/11 22:10:08, xiyuan wrote:
> nit: not needed?

Done.

https://codereview.chromium.org/1973603002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:23:
class Event;
On 2016/05/11 22:10:08, xiyuan wrote:
> nit: not needed?

Done.

[khmel, 2016-05-12 18:28:09]

Thanks Xiyuan for review. Nits were addressed.

Adding oshima@ to review (min c/b/ui/ash/chrome_launcher_prefs.cc)

PTAL

[Mr4D (OOO till 08-26), 2016-05-12 19:51:03]

lgtm

[oshima, 2016-05-12 22:12:01]

https://codereview.chromium.org/1973603002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.h (right):

https://codereview.chromium.org/1973603002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.h:19: static const char
kPlayStoreAppId[] = "gpkmicpkkebkmabiaedjognfppcchdfa";
Please move the definition in .cc.

https://codereview.chromium.org/1973603002/diff/20001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/1973603002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
nuke (c)

[khmel, 2016-05-12 22:40:27]

Thanks for your review.

I've addressed your comments and applied minor fix to make unit_tests happy.

PTAL

https://codereview.chromium.org/1973603002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.h (right):

https://codereview.chromium.org/1973603002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.h:19: static const char
kPlayStoreAppId[] = "gpkmicpkkebkmabiaedjognfppcchdfa";
On 2016/05/12 22:12:01, oshima wrote:
> Please move the definition in .cc.

Done.

https://codereview.chromium.org/1973603002/diff/20001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/1973603002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/05/12 22:12:01, oshima wrote:
> nuke (c)

Done.

[khmel, 2016-05-12 23:56:23]

Hi,

After fixing problem with unit tests I realized that this icon can be easy shown
on non-arc devices.
To prevent this:
1. Add PlayStore to list of pinned apps only if Arc is allowed for current
profile.
2. Don't serialize apps that has pinanble status NO_PIN in order to prevent the
case when user singed on Arc device first and on non-Arc device second.

This is not trivial change.

PTAL

Thanks

[xiyuan, 2016-05-13 15:16:34]

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.h (right):

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.h:67: LauncherControllerHelper*
helper);
nit: Can we make ArcAuthService::IsAllowedForProfile take a const Profile* and
helper getter profile() to return a const Profile*?

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:810:
AppListControllerDelegate::NO_PIN) {
It is funny to see that an item is pinned but it is also not pinnable. Maybe add
a comment to explain?

[khmel, 2016-05-13 16:01:49]

PTAL

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.h (right):

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.h:67: LauncherControllerHelper*
helper);
On 2016/05/13 15:16:34, xiyuan wrote:
> nit: Can we make ArcAuthService::IsAllowedForProfile take a const Profile* and
> helper getter profile() to return a const Profile*?

Good point, I overlooked this.

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:810:
AppListControllerDelegate::NO_PIN) {
On 2016/05/13 15:16:34, xiyuan wrote:
> It is funny to see that an item is pinned but it is also not pinnable. Maybe
add
> a comment to explain?

Agree :), There are 2 items here with special handling, Chrome and App Launcher.
I tried to insert to that workflow first. However result was too massive and
code was not readable. I used current approach and it worked quite well. We have
TODO already to refactore AppListControllerDelegate:: pin constants. May be
solution is to extend, by adding something like PIN_INTERNAL. Best to do in
separate CL, imho.

[xiyuan, 2016-05-13 16:10:07]

SLGTM

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/1973603002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:810:
AppListControllerDelegate::NO_PIN) {
On 2016/05/13 16:01:49, khmel wrote:
> On 2016/05/13 15:16:34, xiyuan wrote:
> > It is funny to see that an item is pinned but it is also not pinnable. Maybe
> add
> > a comment to explain?
> 
> Agree :), There are 2 items here with special handling, Chrome and App
Launcher.
> I tried to insert to that workflow first. However result was too massive and
> code was not readable. I used current approach and it worked quite well. We
have
> TODO already to refactore AppListControllerDelegate:: pin constants. May be
> solution is to extend, by adding something like PIN_INTERNAL. Best to do in
> separate CL, imho.

Acknowledged.

[oshima, 2016-05-13 16:53:27]

https://codereview.chromium.org/1973603002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_utils.h (right):

https://codereview.chromium.org/1973603002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_utils.h:19: extern const char
kPlayStoreAppId[];
do you need extern? This is necessary only if this is linked into shared
library.

[oshima, 2016-05-13 16:54:39]

lgtm

https://codereview.chromium.org/1973603002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_utils.h (right):

https://codereview.chromium.org/1973603002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_utils.h:19: extern const char
kPlayStoreAppId[];
On 2016/05/13 16:53:27, oshima wrote:
> do you need extern? This is necessary only if this is linked into shared
> library.

Ahh, never mind. I was somehow confused with export. I think I need more sleep.

[khmel, 2016-05-13 16:55:51]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-05-13 16:55:51]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/1973603002/#ps100001
(title: "comments")

[commit-bot: I haz the power, 2016-05-13 16:56:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973603002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973603002/100001

[commit-bot: I haz the power, 2016-05-13 16:59:07]

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

[commit-bot: I haz the power, 2016-05-13 16:59:08]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[khmel, 2016-05-13 17:36:43]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-05-13 17:36:43]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
oshima@chromium.org, skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/1973603002/#ps110001
(title: "rebase only")

[commit-bot: I haz the power, 2016-05-13 17:37:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973603002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973603002/110001

[commit-bot: I haz the power, 2016-05-13 18:45:43]

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

[commit-bot: I haz the power, 2016-05-13 18:45:45]

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

[khmel, 2016-05-13 19:37:54]

khmel@chromium.org changed reviewers:
+ bartfab@chromium.org, pastarmovj@chromium.org

[khmel, 2016-05-13 19:37:55]

Hi Julian and Bartosz,

Could one of you review minor change in
chrome/browser/policy/policy_browsertest.cc?

Thanks,

PTAL

[khmel, 2016-05-17 15:38:48]

On 2016/05/13 19:37:55, khmel wrote:
> Hi Julian and Bartosz,
> 
> Could one of you review minor change in
> chrome/browser/policy/policy_browsertest.cc?
> 
> Thanks,
> 
> PTAL

Gentle ping,

PTAL

[pastarmovj, 2016-05-17 22:13:05]

https://codereview.chromium.org/1973603002/diff/130001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1973603002/diff/130001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:676:
command_line->AppendSwitch(chromeos::switches::kEnableArc);
please document the behavior and the reason why these switch is needed for this
test suite.

[khmel, 2016-05-17 22:48:35]

Thanks Julian for comments,

PTAL

https://codereview.chromium.org/1973603002/diff/130001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1973603002/diff/130001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:676:
command_line->AppendSwitch(chromeos::switches::kEnableArc);
On 2016/05/17 22:13:05, pastarmovj wrote:
> please document the behavior and the reason why these switch is needed for
this
> test suite.

Done, and moved it to Arc test, where it looks more logical

[pastarmovj, 2016-05-18 07:31:53]

lgtm

[khmel, 2016-05-18 15:21:08]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-05-18 15:21:08]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
oshima@chromium.org, skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/1973603002/#ps150001
(title: "rebase + policy_browsertest.cc")

[commit-bot: I haz the power, 2016-05-18 15:21:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973603002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973603002/150001

[commit-bot: I haz the power, 2016-05-18 16:55:47]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-05-18 16:57:21]

Description was changed from

==========
arc: Make Play Store item persistance in shelf.

This also shares item with OptIn UI.

BUG=b/28485354
TEST=Manually on device. Play Store icon exists always on shelf.
     when Arc is not OptedIn, clicking this icon opens OptIn UI
     and this shelf icon behaives as ordinal Platform app. Clicking
     on it minimizes/shows UI. Play Store is automatically started
     on OptIn flow finishes. Once Play Store started, there is no
     additional icon for Android app. It reuses the same shelf
     item. Menu in all cases does not have pin/unpin option. If
     Chromebook is opted in already, this just starts/activates
     PlayStore window and no additional item in shelf is created.
==========

to

==========
arc: Make Play Store item persistance in shelf.

This also shares item with OptIn UI.

BUG=b/28485354
TEST=Manually on device. Play Store icon exists always on shelf.
     when Arc is not OptedIn, clicking this icon opens OptIn UI
     and this shelf icon behaives as ordinal Platform app. Clicking
     on it minimizes/shows UI. Play Store is automatically started
     on OptIn flow finishes. Once Play Store started, there is no
     additional icon for Android app. It reuses the same shelf
     item. Menu in all cases does not have pin/unpin option. If
     Chromebook is opted in already, this just starts/activates
     PlayStore window and no additional item in shelf is created.

Committed: https://crrev.com/18cc6b017a26f267792161bdb150eea81f2fecb4
Cr-Commit-Position: refs/heads/master@{#394444}
==========

[commit-bot: I haz the power, 2016-05-18 16:57:23]

Patchset 9 (id:??) landed as
https://crrev.com/18cc6b017a26f267792161bdb150eea81f2fecb4
Cr-Commit-Position: refs/heads/master@{#394444}