Fix ArcSessionManager state machine, part 1.

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 f82e67c428a728954a8bffa79a62274fdc35d1e6..9969660d41666b905a3193feff59d4edada142ba 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -64,6 +64,17 @@ bool g_enable_check_android_management_for_testing = false;
 // but present the UI to try again.
 constexpr base::TimeDelta kArcSignInTimeout = base::TimeDelta::FromMinutes(5);
 
+// Update UMA with user cancel only if error is not currently shown.

[Yusuke Sato, 2017/03/06 20:52:16]

Updates

[hidehiko, 2017/03/07 14:52:20]

Done.

+void MaybeUpdateOptInCancelUMA(const ArcSupportHost* support_host) {
+  if (!support_host ||
+      support_host->ui_page() == ArcSupportHost::UIPage::NO_PAGE ||
+      support_host->ui_page() == ArcSupportHost::UIPage::ERROR) {
+    return;
+  }
+
+  UpdateOptInCancelUMA(OptInCancelReason::USER_CANCEL);
+}
+
 }  // namespace
 
 ArcSessionManager::ArcSessionManager(
@@ -510,7 +521,7 @@ void ArcSessionManager::CancelAuthCode() {
   // LSO, closing the window should stop ARC since the user activity chooses to
   // not sign in. In any other case, ARC is booting normally and the instance
   // should not be stopped.
-  if ((state_ != State::SHOWING_TERMS_OF_SERVICE &&
+  if ((state_ != State::NEGOTIATING_TERMS_OF_SERVICE &&
        state_ != State::CHECKING_ANDROID_MANAGEMENT) &&
       (!support_host_ ||
        (support_host_->ui_page() != ArcSupportHost::UIPage::ERROR &&
@@ -518,13 +529,7 @@ void ArcSessionManager::CancelAuthCode() {
     return;
   }
 
-  // Update UMA with user cancel only if error is not currently shown.
-  if (support_host_ &&
-      support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE &&
-      support_host_->ui_page() != ArcSupportHost::UIPage::ERROR) {
-    UpdateOptInCancelUMA(OptInCancelReason::USER_CANCEL);
-  }
-
+  MaybeUpdateOptInCancelUMA(support_host_.get());
   StopArc();
   SetArcPlayStoreEnabledForProfile(profile_, false);
 }
@@ -568,18 +573,6 @@ void ArcSessionManager::RequestEnableImpl() {
 
   PrefService* const prefs = profile_->GetPrefs();
 
-  // For ARC Kiosk we skip ToS because it is very likely that near the device
-  // there will be no one who is eligible to accept them.
-  // TODO(poromov): Move to more Kiosk dedicated set-up phase.
-  if (IsArcKioskMode())
-    prefs->SetBoolean(prefs::kArcTermsAccepted, true);
-
-  // Skip to show UI asking users to set up ARC OptIn preferences, if all of
-  // them are managed by the admin policy. Note that the ToS agreement is anyway
-  // not shown in the case of the managed ARC.
-  if (AreArcAllOptInPreferencesManaged())
-    prefs->SetBoolean(prefs::kArcTermsAccepted, true);
-
   // 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.
@@ -612,23 +605,6 @@ void ArcSessionManager::RequestEnableImpl() {
     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.
-  if (prefs->GetBoolean(prefs::kArcTermsAccepted)) {
-    // Don't show UI for this progress if it was not shown.
-    if (support_host_ &&
-        support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE) {
-      support_host_->ShowArcLoading();
-    }
-    StartArcAndroidManagementCheck();
-    return;
-  }
-
   StartTermsOfServiceNegotiation();
 }
 
@@ -651,8 +627,10 @@ void ArcSessionManager::RequestDisable() {
 
 void ArcSessionManager::StartTermsOfServiceNegotiation() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile_);
   DCHECK(!terms_of_service_negotiator_);
 
+  // TODO(hidehiko): Remove this condition, when the state machine is fixed.
   if (!arc_session_runner_->IsStopped()) {
     // If the user attempts to re-enable ARC while the ARC instance is still
     // running the user should not be able to continue until the ARC instance
@@ -664,7 +642,17 @@ void ArcSessionManager::StartTermsOfServiceNegotiation() {
     return;
   }
 
-  SetState(State::SHOWING_TERMS_OF_SERVICE);
+  // TODO(hidehiko): DCHECK if |state_| is STOPPED, when the state machine
+  // is fixed.
+  SetState(State::NEGOTIATING_TERMS_OF_SERVICE);
+
+  if (!IsArcTermsOfServiceNegotiationNeeded()) {
+    // Moves to next state, Android management check, immediately, as if
+    // Terms of Service negotiation is done successfully.
+    StartArcAndroidManagementCheck();
+    return;
+  }
+
   if (IsOobeOptInActive()) {
     VLOG(1) << "Use OOBE negotiator.";
     terms_of_service_negotiator_ =
@@ -684,26 +672,56 @@ void ArcSessionManager::StartTermsOfServiceNegotiation() {
 }
 
 void ArcSessionManager::OnTermsOfServiceNegotiated(bool accepted) {
+  DCHECK_EQ(state_, State::NEGOTIATING_TERMS_OF_SERVICE);
+  DCHECK(profile_);
   DCHECK(terms_of_service_negotiator_);
   terms_of_service_negotiator_.reset();
 
   if (!accepted) {
-    // To cancel, user needs to close the window. Note that clicking "Cancel"
-    // button effectively just closes the window.
-    CancelAuthCode();
+    // User does not accept the Terms of Service. Disable Google Play Store.
+    MaybeUpdateOptInCancelUMA(support_host_.get());
+    SetArcPlayStoreEnabledForProfile(profile_, false);
     return;
   }
 
   // Terms were accepted.
   profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
-
-  // Don't show UI for this progress if it was not shown.
-  if (support_host_ &&
-      support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE)
-    support_host_->ShowArcLoading();
   StartArcAndroidManagementCheck();
 }
 
+bool ArcSessionManager::IsArcTermsOfServiceNegotiationNeeded() const {
+  DCHECK(profile_);
+
+  // For ARC Kiosk we skip ToS because it is very likely that near the device
+  // there will be no one who is eligible to accept them.
+  if (IsArcKioskMode()) {

[Yusuke Sato, 2017/03/06 20:52:16]

When is this check necessary? Because of the IsArcKioskMode() check at L582,
RequestEnableImpl() does not seem to call into this function at least.

[hidehiko, 2017/03/07 14:52:20]

Good catch!
So, I removed this from here, and added DCHECKs in StartToSNegotiation(). The
comment at L695- is moved to RequestEnableImpl()'s condition, with some
rephrasing.

+    VLOG(1) << "In Kiosk mode. Skip ARC Terms of Service negotiation.";
+    return false;
+  }
+
+  // Skip to show UI asking users to set up ARC OptIn preferences, if all of
+  // them are managed by the admin policy. Note that the ToS agreement is anyway
+  // not shown in the case of the managed ARC.
+  if (AreArcAllOptInPreferencesManaged()) {

[hidehiko, 2017/03/06 13:38:39]

Note: crrev.com/2731453003 moves the utility to c/b/c/arc/arc_util, which helps
to extract this function.

+    VLOG(1) << "All opt-in preferences are under managed. "
+            << "Skip ARC Terms of Service negotiation.";
+    return false;
+  }
+
+  // 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 a user accepted the Terms of service on Opt-in

[hidehiko, 2017/03/06 13:38:39]

Note: 1) of the original comment is obsolete. khmel@ redisigned the OOBE flow,
so for better adaption to the ASM state transition.

+  // flow, but logged out before ARC sign in procedure was done. Then, logs
+  // in again.
+  if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
+    VLOG(1) << "The user already accepts ARC Terms of Service.";
+    return false;
+  }
+
+  return true;
+}
+
 bool ArcSessionManager::AreArcAllOptInPreferencesManaged() const {
   return IsArcPlayStoreEnabledPreferenceManagedForProfile(profile_) &&
          profile_->GetPrefs()->IsManagedPreference(
@@ -715,12 +733,16 @@ bool ArcSessionManager::AreArcAllOptInPreferencesManaged() const {
 void ArcSessionManager::StartArcAndroidManagementCheck() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(arc_session_runner_->IsStopped());
-  DCHECK(state_ == State::SHOWING_TERMS_OF_SERVICE ||
-         state_ == State::CHECKING_ANDROID_MANAGEMENT ||
-         (state_ == State::STOPPED &&
-          profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)));
+  DCHECK(state_ == State::NEGOTIATING_TERMS_OF_SERVICE ||
+         state_ == State::CHECKING_ANDROID_MANAGEMENT);
   SetState(State::CHECKING_ANDROID_MANAGEMENT);
 
+  // Don't show UI for this progress if it was not shown.

[victorhsieh, 2017/03/06 23:11:52]

optional: I'm doing the same thing locally, and I wonder if it makes sense to
change the comment to "// Show UI only if opt-in is ongoing".  WDYT?

[hidehiko, 2017/03/07 14:52:20]

Rephrased for positive case, with its background.
Note that "opt-in is ongoing" is not the condition here, because even after OOBE
ToS negotiation, if management check fails, ARC support is shown with retry
button. On clicking the retry, we need to show loading. Hmm... complicated...

+  if (support_host_ &&
+      support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE) {
+    support_host_->ShowArcLoading();
+  }
+
   android_management_checker_.reset(new ArcAndroidManagementChecker(

[Yusuke Sato, 2017/03/06 20:52:16]

MakeUnique? (L317 too)

[hidehiko, 2017/03/07 14:52:20]

Done in another CL. As for L317, I'm making another CL to extract the launcher
part. Let me replace it in the CL.

       profile_, context_->token_service(), context_->account_id(),
       false /* retry_on_error */));
@@ -849,7 +871,6 @@ void ArcSessionManager::OnRetryClicked() {
     // Otherwise, we restart ARC. Note: this is the first boot case.
     // For second or later boot, either ERROR_WITH_FEEDBACK case or ACTIVE
     // case must hit.
-    support_host_->ShowArcLoading();
     StartArcAndroidManagementCheck();
   }
 }
@@ -884,7 +905,7 @@ std::ostream& operator<<(std::ostream& os,
   switch (state) {
     MAP_STATE(NOT_INITIALIZED);
     MAP_STATE(STOPPED);
-    MAP_STATE(SHOWING_TERMS_OF_SERVICE);
+    MAP_STATE(NEGOTIATING_TERMS_OF_SERVICE);
     MAP_STATE(CHECKING_ANDROID_MANAGEMENT);
     MAP_STATE(REMOVING_DATA_DIR);
     MAP_STATE(ACTIVE);