arc: Add support of default and OEM apps.

arc: Add support of default and OEM apps.

This implements support for default and OEM Arc apps. These apps are
visible before OptIn. Activating this when Arc is not opted in starts
OptIn flow and schedules deferred launch for this app. User may remove
these apps and default apps stay non vosible until explicitly installed
again. Oem apps exists in app list OEM folder.
Such apps required to be installed in vendor/system Android image or
implement auto-install feature.
https://drive.google.com/a/google.com/file/d/0B373dspH0jyNWVpMVmhVdWJBeTg/view

Preview available here:
https://drive.google.com/file/d/0B63tZNwdjs-qby15dXZyUHBSTEU/view

BUG=b/30953084
BUG=641130
TEST=Manually on device
TEST=Extended unit_tests

Committed: https://crrev.com/6a710e40ddcc54f50f8279cd41ae19f5d170bd00
Cr-Commit-Position: refs/heads/master@{#415498}

[khmel, 2016-08-25 22:06:07]

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

[khmel, 2016-08-25 22:06:08]

Hi Xiyuan,

PTAL

(and sorry for big CL)

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

https://codereview.chromium.org/2281743002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:169:
DCHECK(IsArcEnabled());
Moved to class declaration, I have to call class function

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

https://codereview.chromium.org/2281743002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:361:
base::WeakPtrFactory<ChromeLauncherControllerImpl> weak_ptr_factory_;
To fix crashes in unit tests and possible crash in production

[xiyuan, 2016-08-26 19:32:30]

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

https://codereview.chromium.org/2281743002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:169:
DCHECK(IsArcEnabled());
On 2016/08/25 22:06:08, khmel wrote:
> Moved to class declaration, I have to call class function

Acknowledged.

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:226: base_path =
base_path.Append(kArcDefaultDirectory);
chrome::DIR_DEFAULT_APPS is not used on ChromeOS. It points "default_apps" under
where chrome is, i.e. /opt/google/chrome/default_apps. This is in rootfs and OEM
would not be 
able to put stuff there.

Are you sure this is the place we want to read list?

For regular user, chromeos gets default chrome apps json using
DIR_STANDALONE_EXTERNAL_EXTENSIONS (/usr/share/google-chrome/extensions). Not
sure whether we want to use this dir. Or maybe under "/usr/share/oem", where oem
could put their content there.

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:958:
package_dict->SetBoolean(kUninstalled, true);
Note that AppListSyncableService has support to track uninstalled default apps
(look for TYPE_REMOVE_DEFAULT_APP there).

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:294: static
std::unique_ptr<DefaultAppInfoMap> ReadDefaultAppsFromFileThread();
nit: private static can probably be moved to an anonymous namespace in cc file.

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:305: bool
HasDefaultPackage(const std::string& package_name) const;
Could we move those default app methods and data into a separate class, e.g.
DefaultArcApp ? ArcAppListPrefs can then have it as a member to use the default
app info.

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_test.cc:144:
base::RunLoop().RunUntilIdle();
Is there a better way? Can we extend the default app reader to have a callback
and bind that with RunLoop::QuitClosure?

If we want to use the while loop, let's add add
BrowserThread::GetBlockingPool()->FlushForTesting() to make the loop less busy.

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:228: if
(!auth_service->IsArcEnabled()) {
Can we limit this to only default apps?

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1527:
weak_ptr_factory_.GetWeakPtr()));
This is not necessary as long as PrefChangeRegistrar is owned by |this|.

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3563:
void TestMe() {
nit: This function seems unnecessary if it is not shared with other test cases?

[khmel, 2016-08-26 23:26:19]

Thank for reviewing!

PTAL

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:226: base_path =
base_path.Append(kArcDefaultDirectory);
On 2016/08/26 19:32:29, xiyuan wrote:
> chrome::DIR_DEFAULT_APPS is not used on ChromeOS. It points "default_apps"
under
> where chrome is, i.e. /opt/google/chrome/default_apps. This is in rootfs and
OEM
> would not be 
> able to put stuff there.
> 
> Are you sure this is the place we want to read list?
> 
> For regular user, chromeos gets default chrome apps json using
> DIR_STANDALONE_EXTERNAL_EXTENSIONS (/usr/share/google-chrome/extensions). Not
> sure whether we want to use this dir. Or maybe under "/usr/share/oem", where
oem
> could put their content there.

Yes, I also had doubts that this would be bad choice (it seems used on linux).
Thank you for confirmation! DIR_STANDALONE_EXTERNAL_EXTENSIONS sounds good.

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:958:
package_dict->SetBoolean(kUninstalled, true);
On 2016/08/26 19:32:29, xiyuan wrote:
> Note that AppListSyncableService has support to track uninstalled default apps
> (look for TYPE_REMOVE_DEFAULT_APP there).

I took a look and it seems it has very special handling of extensions. We have
our own sync system for Arc app (packages_sync_service). I feel that solution to
track uninstalled apps(packages) based on TYPE_REMOVE_DEFAULT_APP would take
significant code change and duplicate functional from our package sync service.

WDYT?

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:294: static
std::unique_ptr<DefaultAppInfoMap> ReadDefaultAppsFromFileThread();
On 2016/08/26 19:32:29, xiyuan wrote:
> nit: private static can probably be moved to an anonymous namespace in cc
file.

I used it here in order to access private declaration of DefaultAppInfo. Once we
move to separate file this is no longer needed.

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:305: bool
HasDefaultPackage(const std::string& package_name) const;
On 2016/08/26 19:32:29, xiyuan wrote:
> Could we move those default app methods and data into a separate class, e.g.
> DefaultArcApp ? ArcAppListPrefs can then have it as a member to use the
default
> app info.

Initially did like this :) but moved to one file, due some cross include. Now
was able to resolve.

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_test.cc:144:
base::RunLoop().RunUntilIdle();
On 2016/08/26 19:32:29, xiyuan wrote:
> Is there a better way? Can we extend the default app reader to have a callback
> and bind that with RunLoop::QuitClosure?
> 
> If we want to use the while loop, let's add add
> BrowserThread::GetBlockingPool()->FlushForTesting() to make the loop less
busy.

Done.

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:228: if
(!auth_service->IsArcEnabled()) {
On 2016/08/26 19:32:30, xiyuan wrote:
> Can we limit this to only default apps?

Actually we should have here only default apps. So assert looks good for this
case.

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1527:
weak_ptr_factory_.GetWeakPtr()));
On 2016/08/26 19:32:30, xiyuan wrote:
> This is not necessary as long as PrefChangeRegistrar is owned by |this|.

Overfixed :)

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3563:
void TestMe() {
On 2016/08/26 19:32:30, xiyuan wrote:
> nit: This function seems unnecessary if it is not shared with other test
cases?

Oh, forgot to move into TEST_F(ChromeLauncherControllerArcDefaultAppsTest,
DefaultApps).
My IDE does not work well with this construction so I use temp function to
implement initial functionality.

[xiyuan, 2016-08-29 19:21:56]

lgtm

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

https://codereview.chromium.org/2281743002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:958:
package_dict->SetBoolean(kUninstalled, true);
On 2016/08/26 23:26:18, khmel wrote:
> On 2016/08/26 19:32:29, xiyuan wrote:
> > Note that AppListSyncableService has support to track uninstalled default
apps
> > (look for TYPE_REMOVE_DEFAULT_APP there).
> 
> I took a look and it seems it has very special handling of extensions. We have
> our own sync system for Arc app (packages_sync_service). I feel that solution
to
> track uninstalled apps(packages) based on TYPE_REMOVE_DEFAULT_APP would take
> significant code change and duplicate functional from our package sync
service.
> 
> WDYT? 

Thanks for digging into it. It is fine as long as the default install is synced
so that user does not have to uninstall the default apps many times.

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

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:149: using
DefaultAppsReadyCallback = base::Callback<void(void)>;
nit: this is base::Closure. Can we just use that?

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

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.cc:55: do {
nit: Can we use a for loop instead of "while (true)" ?

like e.g.

https://cs.chromium.org/chromium/src/base/files/file_util_posix.cc?rcl=147246...

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

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.h:76: Delegate* delegate_;
nit: Delegate* const delegate_; since it does not change during lifetime of this
class.

[khmel, 2016-08-30 18:03:49]

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

[khmel, 2016-08-30 18:03:49]

Thanks Xiyuan for review,

Stefan, could you pleas take a look: /b/ui/ash/* ?

PTAL

[Mr4D (OOO till 08-26), 2016-08-30 20:21:46]

A few nits. Please address, then lgtm.

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc
(right):

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:68:
auth_service->AddObserver(this);
Just checking - is it guaranteed that the lifetime of the object is entirely
including the lifetime of these controllers? (so does it get created before the
first object is created and does it get destroyed after the last one goes away?

Furthermore - when can this be null? Would a DCHECK work, or is this for unit
tests? If the latter - please add a comment about that.

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:140: //
If Arc was disabled, remove all deferred launch requests.
I suppose you want to move this comment to line 143?

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:387: 
Can you please remove this line (which would make this file untouched)

[khmel, 2016-08-30 21:22:12]

Thank you for your review!

I addressed last comments, so will try to land this.

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

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:149: using
DefaultAppsReadyCallback = base::Callback<void(void)>;
On 2016/08/29 19:21:56, xiyuan wrote:
> nit: this is base::Closure. Can we just use that?

Done.

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

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.cc:55: do {
On 2016/08/29 19:21:56, xiyuan wrote:
> nit: Can we use a for loop instead of "while (true)" ?
> 
> like e.g.
> 
>
https://cs.chromium.org/chromium/src/base/files/file_util_posix.cc?rcl=147246...

Nice, thanks

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

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_default_app_list.h:76: Delegate* delegate_;
On 2016/08/29 19:21:56, xiyuan wrote:
> nit: Delegate* const delegate_; since it does not change during lifetime of
this
> class.

Done.

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc
(right):

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:68:
auth_service->AddObserver(this);
On 2016/08/30 20:21:46, Mr4D wrote:
> Just checking - is it guaranteed that the lifetime of the object is entirely
> including the lifetime of these controllers? (so does it get created before
the
> first object is created and does it get destroyed after the last one goes
away?
> 
> Furthermore - when can this be null? Would a DCHECK work, or is this for unit
> tests? If the latter - please add a comment about that.

Actually we have unit tests were ArcAuthService might be NULL.
ArcAppDeferredLauncherController is always created in ChromeLauncherController
so we should support tests here. Comments added. Currently ArcBridgeService and
ArcAuthService get released before Shelf deleting. So currently this DTOR is
called after ArcAuthService DTOR (which sets its instance to nullptr).

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:140: //
If Arc was disabled, remove all deferred launch requests.
On 2016/08/30 20:21:46, Mr4D wrote:
> I suppose you want to move this comment to line 143?

Right, that the better place

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2281743002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:387: 
On 2016/08/30 20:21:46, Mr4D wrote:
> Can you please remove this line (which would make this file untouched)

Done.

[khmel, 2016-08-30 21:22:47]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-08-30 21:22:48]

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/2281743002/#ps60001
(title: "rebased + nits")

[commit-bot: I haz the power, 2016-08-30 21:23:29]

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, 2016-08-30 23:53:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-30 23:57:16]

Description was changed from

==========
arc: Add support of default and OEM apps.

This implements support for default and OEM Arc apps. These apps are
visible before OptIn. Activating this when Arc is not opted in starts
OptIn flow and schedules deferred launch for this app. User may remove
these apps and default apps stay non vosible until explicitly installed
again. Oem apps exists in app list OEM folder.
Such apps required to be installed in vendor/system Android image or
implement auto-install feature.
https://drive.google.com/a/google.com/file/d/0B373dspH0jyNWVpMVmhVdWJBeTg/view

Preview available here:
https://drive.google.com/file/d/0B63tZNwdjs-qby15dXZyUHBSTEU/view

BUG=b/30953084
BUG=641130
TEST=Manually on device
TEST=Extended unit_tests
==========

to

==========
arc: Add support of default and OEM apps.

This implements support for default and OEM Arc apps. These apps are
visible before OptIn. Activating this when Arc is not opted in starts
OptIn flow and schedules deferred launch for this app. User may remove
these apps and default apps stay non vosible until explicitly installed
again. Oem apps exists in app list OEM folder.
Such apps required to be installed in vendor/system Android image or
implement auto-install feature.
https://drive.google.com/a/google.com/file/d/0B373dspH0jyNWVpMVmhVdWJBeTg/view

Preview available here:
https://drive.google.com/file/d/0B63tZNwdjs-qby15dXZyUHBSTEU/view

BUG=b/30953084
BUG=641130
TEST=Manually on device
TEST=Extended unit_tests

Committed: https://crrev.com/6a710e40ddcc54f50f8279cd41ae19f5d170bd00
Cr-Commit-Position: refs/heads/master@{#415498}
==========

[commit-bot: I haz the power, 2016-08-30 23:57:17]

Patchset 4 (id:??) landed as
https://crrev.com/6a710e40ddcc54f50f8279cd41ae19f5d170bd00
Cr-Commit-Position: refs/heads/master@{#415498}