arc: Enable Play Store item in App Launcher for managed case.

arc: Enable Play Store item in App Launcher for managed case.

This fixes issue when Play Store item is not available for ARC
managed and enabled case.

BUG=b/36043525
TEST=Extended unit_tests
TEST=Manually on Caroline, normal case (@gmail.com), managed
     enabled (@google.com) and managed disabled (@chromium.org).

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

[khmel, 2017-03-09 00:27:49]

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

[khmel, 2017-03-09 00:27:49]

Hi Xiyuan,

PTAL,

Thanks!

https://codereview.chromium.org/2738983003/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/2738983003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:125: class
ArcAppModelBuilderTest : public extensions::ExtensionServiceTestBase,
AppListTestBase has functionality to init profile using test file. This creates
syncable prefs but not TestingPrefService. I dropped this inheritance and use
default init test init not file based. This creates TestingPrefService needed
for updated test and this is sufficient for other tests.

[hidehiko, 2017-03-09 06:17:00]

hidehiko@chromium.org changed reviewers:
+ hidehiko@chromium.org

[hidehiko, 2017-03-09 06:17:01]

Drive-by: just a quick request of re-wording for disambiguate.

https://codereview.chromium.org/2738983003/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/2738983003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:100: ARC_UNMANAGED,
Please avoid using just "enabled" meaning PlayStore is enabled for ARC,
considering pARC is coming.
Maybe:

// By default, Play Store is unmanaged and non-persistent.
ARC_PLAY_STORE_UNMANAGED

// Play Store is non-persistent, managed and enabled.
ARC_PLAY_STORE_MANAGED_AND_ENABLED

and so on.

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

https://codereview.chromium.org/2738983003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.h:53: // managed and enabled
case.
Ditto.

[khmel, 2017-03-09 18:00:43]

https://codereview.chromium.org/2738983003/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/2738983003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:100: ARC_UNMANAGED,
On 2017/03/09 06:17:00, hidehiko wrote:
> Please avoid using just "enabled" meaning PlayStore is enabled for ARC,
> considering pARC is coming.
> Maybe:
> 
> // By default, Play Store is unmanaged and non-persistent.
> ARC_PLAY_STORE_UNMANAGED
> 
> // Play Store is non-persistent, managed and enabled.
> ARC_PLAY_STORE_MANAGED_AND_ENABLED
> 
> and so on.

Thanks for good point! Done.

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

https://codereview.chromium.org/2738983003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.h:53: // managed and enabled
case.
On 2017/03/09 06:17:00, hidehiko wrote:
> Ditto.

Done.

[khmel, 2017-03-09 18:01:04]

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

[commit-bot: I haz the power, 2017-03-09 18:01:28]

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-03-09 18:30:15]

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

[commit-bot: I haz the power, 2017-03-09 18:30:15]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-09 20:51:21]

just nits

https://codereview.chromium.org/2738983003/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_default_app_list.h (right):

https://codereview.chromium.org/2738983003/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.h:51: FILTER_NOTHING,
nit: FILTER_ prefix seems redundant
i.e.

  enum class FilterLevel {
    // Filter nothing.
    NOTHING,
    // Filter only optional apps,...
    OPTIONAL_APPS,
  ...

https://codereview.chromium.org/2738983003/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.h:52: // Filter only
optional apps, excluding Play Store for example. Used in case
Is the comment correct? FILTER_OPTIONAL_APPS seems filtering everything except
Play Store. The comment seems suggesting the other way around - removing Play
Store and keeping everything else.

[khmel, 2017-03-09 21:14:00]

Thank you comments!

PTAL updated version.

https://codereview.chromium.org/2738983003/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_default_app_list.h (right):

https://codereview.chromium.org/2738983003/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.h:51: FILTER_NOTHING,
On 2017/03/09 20:51:21, xiyuan (slow) wrote:
> nit: FILTER_ prefix seems redundant
> i.e.
> 
>   enum class FilterLevel {
>     // Filter nothing.
>     NOTHING,
>     // Filter only optional apps,...
>     OPTIONAL_APPS,
>   ...

Yep, makes sense since we use class Filter...

https://codereview.chromium.org/2738983003/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.h:52: // Filter only
optional apps, excluding Play Store for example. Used in case
On 2017/03/09 20:51:21, xiyuan (slow) wrote:
> Is the comment correct? FILTER_OPTIONAL_APPS seems filtering everything except
> Play Store. The comment seems suggesting the other way around - removing Play
> Store and keeping everything else.
:) agree, It seems filter _out_ solves this incorrect reading.

[xiyuan, 2017-03-09 21:15:31]

lgtm

[khmel, 2017-03-09 21:15:54]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-09 21:16:20]

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-03-09 22:25:37]

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

[commit-bot: I haz the power, 2017-03-09 22:25:39]

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

[khmel, 2017-03-09 22:33:48]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-09 22:34:42]

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-03-09 23:22:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489098828593840,
"parent_rev": "fbb226e6e24e82cb0dcf4559510790e23d7665bc", "commit_rev":
"8d651457cff4086e56e9861bc24ae1f127b590d6"}

[commit-bot: I haz the power, 2017-03-09 23:23:08]

Description was changed from

==========
arc: Enable Play Store item in App Launcher for managed case.

This fixes issue when Play Store item is not available for ARC
managed and enabled case.

BUG=b/36043525
TEST=Extended unit_tests
TEST=Manually on Caroline, normal case (@gmail.com), managed
     enabled (@google.com) and managed disabled (@chromium.org).
==========

to

==========
arc: Enable Play Store item in App Launcher for managed case.

This fixes issue when Play Store item is not available for ARC
managed and enabled case.

BUG=b/36043525
TEST=Extended unit_tests
TEST=Manually on Caroline, normal case (@gmail.com), managed
     enabled (@google.com) and managed disabled (@chromium.org).

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

[commit-bot: I haz the power, 2017-03-09 23:23:09]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8d651457cff4086e56e9861bc24a...