Start ARC and sign in after Chrome OS login

chrome/browser/chromeos/arc/arc_session_manager.cc

Index: chrome/browser/chromeos/arc/arc_session_manager.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager.cc b/chrome/browser/chromeos/arc/arc_session_manager.cc
index 7eab36a63c72b086c0c29b6e9e06b531d7179912..a87b1b41836752785befe3551b9ad76db3687af3 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -156,6 +156,12 @@ void ArcSessionManager::EnableCheckAndroidManagementForTesting() {
   g_enable_check_android_management_for_testing = true;
 }
 
+// static
+bool ArcSessionManager::IsPersistentArc() {
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+      chromeos::switches::kEnableArcPersistently);
+}
+
 void ArcSessionManager::OnSessionReady() {
   for (auto& observer : arc_session_observer_list_)
     observer.OnSessionReady();
@@ -395,6 +401,7 @@ void ArcSessionManager::OnPrimaryUserProfilePrepared(Profile* profile) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile && profile != profile_);
 
+  // TODO(victorhsieh): Keep using the instance from login screen when ready.
   Shutdown();
 
   if (!IsArcAllowedForProfile(profile))
@@ -449,7 +456,7 @@ void ArcSessionManager::OnPrimaryUserProfilePrepared(Profile* profile) {
     RemoveArcData();
   }
 
-  if (IsArcPlayStoreEnabled()) {
+  if (IsPersistentArc() || IsArcPlayStoreEnabled()) {
     VLOG(1) << "ARC is already enabled.";
     DCHECK(!enable_requested_);
     RequestEnable();
@@ -513,13 +520,13 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile_);
 
-  const bool arc_enabled = IsArcPlayStoreEnabled();
+  const bool play_enabled = IsArcPlayStoreEnabled();

[hidehiko, 2017/02/23 10:29:16]

Could you use "play_store" instead of "play" for consistency? Ditto for all this
CL.

[victorhsieh, 2017/02/24 00:44:59]

Done.

[hidehiko, 2017/02/28 19:26:06]

There are still many "play" (not "play store") usage in this CL. Could you fix
all of them for consistency?

[victorhsieh, 2017/02/28 21:47:06]

Really done.  Double checked with `git show | grep play -i |grep -v -i 'play[
_]*store' |grep -v display -i`.

   if (!IsArcManaged()) {
     // Update UMA only for non-Managed cases.
-    UpdateOptInActionUMA(arc_enabled ? OptInActionType::OPTED_IN
-                                     : OptInActionType::OPTED_OUT);
+    UpdateOptInActionUMA(play_enabled ? OptInActionType::OPTED_IN
+                                      : OptInActionType::OPTED_OUT);
 
-    if (!arc_enabled) {
+    if (!play_enabled) {
       // Remove the pinned Play Store icon launcher in Shelf.
       // This is only for non-Managed cases. In managed cases, it is expected
       // to be "disabled" rather than "removed", so keep it here.
@@ -539,13 +546,19 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
     ArcAuthNotification::Hide();
   }
 
-  if (arc_enabled)
+  if (IsPersistentArc()) {

[hidehiko, 2017/02/23 10:29:16]

Clarification: Several services expect OnArcPlayStoreEnabledChanged() callback
below.
Any plans about that?

[victorhsieh, 2017/02/24 00:44:59]

Fixed.  It's not intentional.  Thanks.

+    // TODO(victorhsieh): Implement opt-in and opt-out flow.  For now, do
+    // nothing but keep the existing ARC instance running.
+    return;
+  }
+
+  if (play_enabled)
     RequestEnable();
   else
     RequestDisable();
 
   for (auto& observer : observer_list_)
-    observer.OnArcPlayStoreEnabledChanged(arc_enabled);
+    observer.OnArcPlayStoreEnabledChanged(play_enabled);
 }
 
 void ArcSessionManager::ShutdownSession() {
@@ -728,6 +741,14 @@ void ArcSessionManager::RequestEnableImpl() {
     prefs->SetBoolean(prefs::kArcTermsAccepted, true);
   }
 
+  if (IsPersistentArc()) {

[hidehiko, 2017/02/23 10:29:16]

Maybe merge this into L757?

[victorhsieh, 2017/02/24 00:44:59]

Done.

+    DCHECK(IsArcAllowedForProfile(profile_));
+    StartArc();
+    if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))

[hidehiko, 2017/02/23 10:29:16]

IIUC, background management check is always needed?
If there is special reason here, could you comment?

[victorhsieh, 2017/02/24 00:44:59]

Done.

+      StartArcAndroidManagementCheckedInBackground();
+    return;
+  }
+
   // If it is marked that sign in has been successfully done, then directly
   // start ARC.
   // For testing, and for Kiosk mode, we also skip ToS negotiation procedure.
@@ -751,12 +772,7 @@ void ArcSessionManager::RequestEnableImpl() {
     // on the same stack), StartCheck() needs to be called *after* StartArc().
     // Otherwise, SetArcPlayStoreEnabled() which may be called in
     // OnBackgroundAndroidManagementChecked() could be ignored.
-    android_management_checker_ = base::MakeUnique<ArcAndroidManagementChecker>(
-        profile_, context_->token_service(), context_->account_id(),
-        true /* retry_on_error */);
-    android_management_checker_->StartCheck(
-        base::Bind(&ArcSessionManager::OnBackgroundAndroidManagementChecked,
-                   weak_ptr_factory_.GetWeakPtr()));
+    StartArcAndroidManagementCheckedInBackground();

[hidehiko, 2017/02/23 10:29:16]

Maybe: "Check" rather than "Checked"?

[victorhsieh, 2017/02/24 00:44:59]

Done.

     return;
   }
 
@@ -869,6 +885,15 @@ void ArcSessionManager::StartArcAndroidManagementCheck() {
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
+void ArcSessionManager::StartArcAndroidManagementCheckedInBackground() {
+  android_management_checker_ = base::MakeUnique<ArcAndroidManagementChecker>(

[hidehiko, 2017/02/23 10:29:16]

Could you add DCHECKs similar to StartArcAndroidManagementCheck()

[victorhsieh, 2017/02/24 00:44:59]

Reverted this change.

+      profile_, context_->token_service(), context_->account_id(),
+      true /* retry_on_error */);
+  android_management_checker_->StartCheck(
+      base::Bind(&ArcSessionManager::OnBackgroundAndroidManagementChecked,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 void ArcSessionManager::OnAndroidManagementChecked(
     policy::AndroidManagementClient::Result result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);