Do not run Android management check for re-auth case.

chrome/browser/chromeos/arc/arc_auth_service.cc

Index: chrome/browser/chromeos/arc/arc_auth_service.cc
diff --git a/chrome/browser/chromeos/arc/arc_auth_service.cc b/chrome/browser/chromeos/arc/arc_auth_service.cc
index 026f3cb12523cb83dda47841dc17a79843fbdf97..80eea15d16b62eed82e1b59b75916a042a238829 100644
--- a/chrome/browser/chromeos/arc/arc_auth_service.cc
+++ b/chrome/browser/chromeos/arc/arc_auth_service.cc
@@ -635,16 +635,7 @@ void ArcAuthService::ShowUI(ArcSupportHost::UIPage page,
 
 void ArcAuthService::OnContextReady() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  // TODO(hidehiko): The check is not necessary if this is a part of re-auth
-  // flow and OOBE OptIn where Android Management check must be a part of
-  // checking if Arc OptIn should be skip. Remove this.
-  android_management_checker_.reset(new ArcAndroidManagementChecker(
-      profile_, context_->token_service(), context_->account_id(),
-      false /* retry_on_error */));
-  android_management_checker_->StartCheck(
-      base::Bind(&ArcAuthService::OnAndroidManagementChecked,
-                 weak_ptr_factory_.GetWeakPtr()));
+  FetchAuthCode();
 }
 
 void ArcAuthService::OnSyncedPrefChanged(const std::string& path,
@@ -794,8 +785,6 @@ void ArcAuthService::SetAuthCodeAndStartArc(const std::string& auth_code) {
   DCHECK(!auth_code.empty());
 
   if (IsAuthCodeRequest()) {
-    DCHECK_EQ(state_, State::FETCHING_CODE);

[hidehiko, 2016/11/10 01:46:23]

here, state_ should be State::ACTIVE in most cases, but may be not (race
issue... It needs to be addressed later.).

-    SetState(State::ACTIVE);
     account_info_notifier_->Notify(!IsOptInVerificationDisabled(), auth_code,
                                    mojom::ChromeAccountType::USER_ACCOUNT,
                                    policy_util::IsAccountManaged(profile_));
@@ -803,7 +792,8 @@ void ArcAuthService::SetAuthCodeAndStartArc(const std::string& auth_code) {
     return;
   }
 
-  if (state_ != State::FETCHING_CODE) {
+  if (state_ != State::TERMS && state_ != State::ANDROID_MANAGEMENT_CHECK &&
+      state_ != State::FETCHING_CODE) {
     ShutdownBridgeAndCloseUI();
     return;
   }
@@ -828,19 +818,8 @@ void ArcAuthService::OnArcSignInTimeout() {
 void ArcAuthService::StartLso() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Terms were accepted
-  profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);

[hidehiko, 2016/11/10 01:46:23]

Note: Moved to OnTermsAccepted(). Actually, this is not a part of Lso starting.

-
-  // Update UMA only if error (with or without feedback) is currently shown.
-  if (ui_page_ == ArcSupportHost::UIPage::ERROR) {
-    UpdateOptInActionUMA(OptInActionType::RETRY);
-  } else if (ui_page_ == ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK) {
-    UpdateOptInActionUMA(OptInActionType::RETRY);
-    ShutdownBridge();
-  }
-
   DCHECK(arc_bridge_service()->stopped());
-  SetState(State::FETCHING_CODE);
+  SetState(State::ANDROID_MANAGEMENT_CHECK);

[xiyuan, 2016/11/10 17:55:21]

Why ANDROID_MANAGEMENT_CHECK instead of FETCHING_CODE?

[hidehiko, 2016/11/14 10:55:09]

Sorry this was my mistake. We no longer need StartLso(). Removed.

   PrepareContextForAuthCodeRequest();
 }
 
@@ -855,7 +834,8 @@ void ArcAuthService::CancelAuthCode() {
   // In case |state_| is ACTIVE, |ui_page_| can be START_PROGRESS (which means
   // normal Arc booting) or  ERROR or ERROR_WITH_FEEDBACK (in case Arc can not
   // be started). If Arc is booting normally dont't stop it on progress close.
-  if (state_ != State::FETCHING_CODE &&
+  if ((state_ != State::FETCHING_CODE && state_ != State::TERMS &&
+       state_ != State::ANDROID_MANAGEMENT_CHECK) &&
       ui_page_ != ArcSupportHost::UIPage::ERROR &&
       ui_page_ != ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK) {
     return;
@@ -926,7 +906,7 @@ void ArcAuthService::StartUI() {
     return;
   }
 
-  SetState(State::FETCHING_CODE);
+  SetState(State::TERMS);
   ShowUI(ArcSupportHost::UIPage::TERMS, base::string16());
 }
 
@@ -952,12 +932,33 @@ void ArcAuthService::OnAuthCodeFailed() {
   UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
 }
 
+void ArcAuthService::StartArcAndroidManagementCheck() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(arc_bridge_service()->stopped());
+  SetState(State::ANDROID_MANAGEMENT_CHECK);

[Luis Héctor Chávez, 2016/11/10 23:30:58]

Is it possible to have a DCHECK_EQ(state_, ...) every time SetState is called?
That way the transitions are more explicit.

[hidehiko, 2016/11/14 10:55:09]

WDY mean?
SetState(...) is just "state_ = ...". So;

SetState(CHECKING_ANDROID_MANAGEMENT);
DCHECK_EQ(CHECKING_ANDROID_MANAGEMENT, state_);

looks redundant to me?

Note: let me keep SetState() for now. It will be gone when we introduce
ArcSessionManager.

[Luis Héctor Chávez, 2016/11/14 17:06:50]

Sorry, I meant before assigning. In other words, making sure the transition is
happening from an expected state (if possible avoid being in an expected set of
states to simplify logic and make it more understandable).

[hidehiko, 2016/11/14 19:07:57]

I got your point.
However, currently, the state transition is un-manageable, IMHO...
Many methods are called without taking care of its state already.
Even just adding DCHECK is sometimes difficult or more complex than simple
thought, and it looks not practical to me at the moment.

In following CLs, I'll give it a try to have clearer state machine...

+
+  android_management_checker_.reset(new ArcAndroidManagementChecker(
+      profile_, context_->token_service(), context_->account_id(),
+      false /* retry_on_error */));
+  android_management_checker_->StartCheck(
+      base::Bind(&ArcAuthService::OnAndroidManagementChecked,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 void ArcAuthService::OnAndroidManagementChecked(
     policy::AndroidManagementClient::Result result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK_EQ(state_, State::ANDROID_MANAGEMENT_CHECK);
+
   switch (result) {
     case policy::AndroidManagementClient::Result::UNMANAGED:
-      OnAndroidManagementPassed();
+      if (IsOptInVerificationDisabled()) {
+        StartArc();
+      } else {
+        // TODO(hidehiko): Merge this prefetching into re-auth case.
+        SetState(State::FETCHING_CODE);
+        PrepareContextForAuthCodeRequest();
+      }
       break;
     case policy::AndroidManagementClient::Result::MANAGED:
       ShutdownBridgeAndShowUI(
@@ -1010,23 +1011,6 @@ void ArcAuthService::FetchAuthCode() {
   }
 }
 
-void ArcAuthService::OnAndroidManagementPassed() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  if (state_ == State::ACTIVE) {

[hidehiko, 2016/11/10 01:46:23]

Because AndroidManagement check is no longer running for re-auth flow, so this
is gone.

-    if (IsAuthCodeRequest())
-      FetchAuthCode();
-    return;
-  }
-
-  if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn) ||

[hidehiko, 2016/11/10 01:46:23]

This is migrated into OnAndroidManagementChecked().

-      IsOptInVerificationDisabled()) {
-    StartArc();
-  } else {
-    FetchAuthCode();
-  }
-}
-
 void ArcAuthService::OnWindowClosed() {
   CancelAuthCode();
 }
@@ -1034,6 +1018,9 @@ void ArcAuthService::OnWindowClosed() {
 void ArcAuthService::OnTermsAgreed(bool is_metrics_enabled,
                                    bool is_backup_and_restore_enabled,
                                    bool is_location_service_enabled) {
+  // Terms were accepted
+  profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
+
   // This is ARC support's UI event callback, so this is called only when
   // the UI is visible. The condition to open the UI is
   // !g_disable_ui_for_testing && !IsOptInVerificationDisabled() (see ShowUI())
@@ -1044,13 +1031,39 @@ void ArcAuthService::OnTermsAgreed(bool is_metrics_enabled,
   preference_handler_->EnableMetrics(is_metrics_enabled);
   preference_handler_->EnableBackupRestore(is_backup_and_restore_enabled);
   preference_handler_->EnableLocationService(is_location_service_enabled);
-  StartLso();
+  StartArcAndroidManagementCheck();
 }
 
 void ArcAuthService::OnAuthSucceeded(const std::string& auth_code) {
   SetAuthCodeAndStartArc(auth_code);
 }
 
+void ArcAuthService::OnRetryClicked() {
+  UpdateOptInActionUMA(OptInActionType::RETRY);
+  // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, we postpone
+  // to stop the ARC to obtain the internal state.
+  // Here, on retry, stop it.
+  if (ui_page_ == ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK)
+    ShutdownBridge();
+
+  switch (state_) {
+    case State::NOT_INITIALIZED:
+      NOTREACHED();
+      break;
+    case State::STOPPED:
+    case State::TERMS:
+      ShowUI(ArcSupportHost::UIPage::TERMS, base::string16());
+      break;
+    case State::ANDROID_MANAGEMENT_CHECK:
+      StartArcAndroidManagementCheck();
+      break;
+    case State::FETCHING_CODE:
+    case State::ACTIVE:
+      PrepareContextForAuthCodeRequest();
+      break;
+  }
+}
+
 void ArcAuthService::OnSendFeedbackClicked() {
   chrome::OpenFeedbackDialog(nullptr);
 }
@@ -1079,6 +1092,10 @@ std::ostream& operator<<(std::ostream& os, const ArcAuthService::State& state) {
       return os << "NOT_INITIALIZED";
     case ArcAuthService::State::STOPPED:
       return os << "STOPPED";
+    case ArcAuthService::State::TERMS:
+      return os << "TERMS";
+    case ArcAuthService::State::ANDROID_MANAGEMENT_CHECK:
+      return os << "ANDROID_MANAGEMENT_CHECK";
     case ArcAuthService::State::FETCHING_CODE:
       return os << "FETCHING_CODE";
     case ArcAuthService::State::ACTIVE: