Start ARC and sign in after Chrome OS login

chrome/browser/chromeos/arc/arc_session_manager.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 
 #include <utility>
 
 #include "ash/common/shelf/shelf_delegate.h"
 #include "ash/common/wm_shell.h"
@@ skipping 138 matching lines @@
 void ArcSessionManager::SetShelfDelegateForTesting(
     ash::ShelfDelegate* shelf_delegate) {
   g_shelf_delegate_for_testing = shelf_delegate;
 }
 
 // static
 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();
 }
 
 void ArcSessionManager::OnSessionStopped(StopReason reason) {
   // TODO(crbug.com/625923): Use |reason| to report more detailed errors.
   if (arc_sign_in_timer_.IsRunning())
     OnProvisioningFinished(ProvisioningResult::ARC_STOPPED);
 
@@ skipping 219 matching lines @@
 
 bool ArcSessionManager::IsAllowed() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return profile_ != nullptr;
 }
 
 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))
     return;
 
   // TODO(khmel): Move this to IsArcAllowedForProfile.
   if (policy_util::IsArcDisabledForEnterprise() &&
       policy_util::IsAccountManaged(profile)) {
     VLOG(2) << "Enterprise users are not supported in ARC.";
     return;
@@ skipping 34 matching lines @@
       base::Bind(&ArcSessionManager::OnOptInPreferenceChanged,
                  weak_ptr_factory_.GetWeakPtr()));
 
   // Chrome may be shut down before completing ARC data removal.
   // In such a case, start removing the data now.
   if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
     VLOG(1) << "ARC data removal requested in previous session.";
     RemoveArcData();
   }
 
-  if (IsArcPlayStoreEnabled()) {
+  if (IsPersistentArc() || IsArcPlayStoreEnabled()) {
     VLOG(1) << "ARC is already enabled.";
     DCHECK(!enable_requested_);
     RequestEnable();
   } else {
     VLOG(1) << "ARC is initially disabled. Removing data.";
     RemoveArcData();
     PrefServiceSyncableFromProfile(profile_)->AddObserver(this);
     OnIsSyncingChanged();
   }
 }
@@ skipping 43 matching lines @@
   }
   ShutdownSession();
   if (support_host_)
     support_host_->Close();
 }
 
 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.
       ash::ShelfDelegate* shelf_delegate = GetShelfDelegate();
       if (shelf_delegate)
         shelf_delegate->UnpinAppWithID(ArcSupportHost::kHostAppId);
     }
   }
 
   if (support_host_)
     support_host_->SetArcManaged(IsArcManaged());
 
   // Hide auth notification if it was opened before and arc.enabled pref was
   // explicitly set to true or false.
   if (!g_disable_ui_for_testing &&
       profile_->GetPrefs()->HasPrefPath(prefs::kArcEnabled)) {
     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() {
   arc_sign_in_timer_.Stop();
   playstore_launcher_.reset();
   terms_of_service_negotiator_.reset();
   android_management_checker_.reset();
   arc_session_runner_->RequestStop();
   // TODO(hidehiko): The ARC instance's stopping is asynchronous, so it might
   // still be running when we return from this function. Do not set the
@@ skipping 162 matching lines @@
   // Skip to show UI asking users to enable/disable their preference for
   // backup-restore and location-service, if both are managed by the admin
   // policy. Note that the ToS agreement is anyway not shown in the case of the
   // managed ARC.
   if (IsArcManaged() &&
       prefs->IsManagedPreference(prefs::kArcBackupRestoreEnabled) &&
       prefs->IsManagedPreference(prefs::kArcLocationServiceEnabled)) {
     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.
   // For backward compatibility, this check needs to be prior to the
   // kArcTermsAccepted check below.
   if (prefs->GetBoolean(prefs::kArcSignedIn) ||
       IsArcOptInVerificationDisabled() || IsArcKioskMode()) {
     StartArc();
 
     // Skip Android management check for testing.
     // We also skip if Android management check for Kiosk mode,
     // because there are no managed human users for Kiosk exist.
     if (IsArcOptInVerificationDisabled() || IsArcKioskMode() ||
         (g_disable_ui_for_testing &&
          !g_enable_check_android_management_for_testing)) {
       return;
     }
 
     // Check Android management in parallel.
     // Note: Because the callback may be called in synchronous way (i.e. called
     // 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;
   }
 
   // If it is marked that the Terms of service is accepted already,
   // just skip the negotiation with user, and start Android management
   // check directly.
   // This happens, e.g., when;
   // 1) User accepted the Terms of service on OOBE flow.
   // 2) User accepted the Terms of service on Opt-in flow, but logged out
   //   before ARC sign in procedure was done. Then, logs in again.
@@ skipping 92 matching lines @@
   SetState(State::CHECKING_ANDROID_MANAGEMENT);
 
   android_management_checker_.reset(new ArcAndroidManagementChecker(
       profile_, context_->token_service(), context_->account_id(),
       false /* retry_on_error */));
   android_management_checker_->StartCheck(
       base::Bind(&ArcSessionManager::OnAndroidManagementChecked,
                  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);
   DCHECK_EQ(state_, State::CHECKING_ANDROID_MANAGEMENT);
 
   switch (result) {
     case policy::AndroidManagementClient::Result::UNMANAGED:
       VLOG(1) << "Starting ARC for first sign in.";
       sign_in_start_time_ = base::Time::Now();
       arc_sign_in_timer_.Start(
@@ skipping 126 matching lines @@
 
 #undef MAP_STATE
 
   // Some compilers report an error even if all values of an enum-class are
   // covered exhaustively in a switch statement.
   NOTREACHED() << "Invalid value " << static_cast<int>(state);
   return os;
 }
 
 }  // namespace arc