Split IsArcPlayStoreEnabled() and ArcSessionManager's enabled concepts.

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 e18cc1594c52cefd1f2f7049c1789bbc5e344c46..8bf2c787eaf60c3544a3a771e6800458599ff2f5 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -231,15 +231,15 @@ void ArcSessionManager::MaybeReenableArc() {
   // "on managed lost" case. In that case, OnSessionStopped() should trigger
   // the RemoveArcData(), then this.
   // TODO(hidehiko): It looks necessary to reset |reenable_arc_| regardless of
-  // IsArcPlayStoreEnabled(). Fix it.
-  if (!reenable_arc_ || !IsArcPlayStoreEnabled())
+  // enabled_. Fix it.

[Daniel Erat, 2017/02/15 14:50:05]

nit: |enabled_| to match rest of comment

[hidehiko, 2017/02/15 19:45:11]

Done.

+  if (!reenable_arc_ || !enabled_)
     return;
 
   // Restart ARC anyway. Let the enterprise reporting instance decide whether
   // the ARC user data wipe is still required or not.
   reenable_arc_ = false;
   VLOG(1) << "Reenable ARC";
-  OnOptInPreferenceChanged();
+  EnableImpl();
 }
 
 void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
@@ -250,7 +250,7 @@ void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
   // container. Ignore all |result|s arriving while ARC is disabled, in order to
   // avoid popping up an error message triggered below. This code intentionally
   // does not support the case of reenabling.
-  if (!IsArcPlayStoreEnabled()) {
+  if (!enabled_) {
     LOG(WARNING) << "Provisioning result received after ARC was disabled. "
                  << "Ignoring result " << static_cast<int>(result) << ".";
     return;
@@ -418,6 +418,7 @@ void ArcSessionManager::OnPrimaryUserProfilePrepared(Profile* profile) {
       !IsArcKioskMode()) {
     DCHECK(!support_host_);
     support_host_ = base::MakeUnique<ArcSupportHost>(profile_);
+    support_host_->SetArcManaged(IsArcManaged());
     support_host_->AddObserver(this);
   }
 
@@ -430,23 +431,26 @@ void ArcSessionManager::OnPrimaryUserProfilePrepared(Profile* profile) {
       g_enable_check_android_management_for_testing) {
     ArcAndroidManagementChecker::StartClient();
   }
+
   pref_change_registrar_.Init(profile_->GetPrefs());
   pref_change_registrar_.Add(
       prefs::kArcEnabled,
       base::Bind(&ArcSessionManager::OnOptInPreferenceChanged,
                  weak_ptr_factory_.GetWeakPtr()));
-  if (profile_->GetPrefs()->GetBoolean(prefs::kArcEnabled)) {
-    // Don't start ARC if there is a pending request to remove the data. Restart
-    // ARC once data removal finishes.
-    if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
-      reenable_arc_ = true;
-      VLOG(1) << "ARC previously requested to remove data.";
-      RemoveArcData();
-    } else {
-      OnOptInPreferenceChanged();
-    }
+
+  // Chrome may be shutdown before completing ARC data removal.

[Daniel Erat, 2017/02/15 14:50:05]

nit: s/shutdown/shut down/

("shut down" is a verb, "shutdown" is a noun)

[hidehiko, 2017/02/15 19:45:11]

Done.

+  // In such a case, start removing the data now.
+  if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
+    VLOG(1) << "ARC previously requested to remove data.";

[Daniel Erat, 2017/02/15 14:50:05]

nit: "ARC data removal requested."?

[hidehiko, 2017/02/15 19:45:11]

Done. Mentioned "previous session" explicitly, because it is key reason here.

+    RemoveArcData();
+  }
+
+  if (IsArcPlayStoreEnabled()) {
+    VLOG(1) << "ARC is already enabled at beginning.";

[Daniel Erat, 2017/02/15 14:50:06]

nit: i'd make this just "ARC is already enabled."

[hidehiko, 2017/02/15 19:45:11]

Done.

+    DCHECK(!enabled_);
+    Enable();
   } else {
-    VLOG(1) << "ARC disabled on profile. Removing data.";
+    VLOG(1) << "ARC is disabled at beginning. Removing the data.";

[Daniel Erat, 2017/02/15 14:50:05]

nit: "ARC is initially disabled. Removing data."

[hidehiko, 2017/02/15 19:45:11]

Done.

     RemoveArcData();
     PrefServiceSyncableFromProfile(profile_)->AddObserver(this);
     OnIsSyncingChanged();
@@ -473,6 +477,7 @@ void ArcSessionManager::Shutdown() {
   if (!g_disable_ui_for_testing)
     ArcAuthNotification::Hide();
 
+  enabled_ = false;

[khmel, 2017/02/15 16:00:24]

This works as dupe of State. Can we probably better introduce new state? Now we
have arc.enabled, enabled_, state_ and all are dependent. I think it could be
very hard to understand the logic. 
Also it would be nice to see unit_tests that covers test cases when enabled_
invoked.

[Luis Héctor Chávez, 2017/02/15 19:18:53]

it doesn't seem to be _quite_ a dupe in this change. Having said that,
hidehiko@, do you think we can incorporate |enabled_| into |state_| so we have
halve our number of possible states?

If it is definitely not possible, please clearly document the relationship
between the three variables somewhere.

[hidehiko, 2017/02/15 19:45:11]

|enabled_| can be changed at any time by Enable()/Disable() ((will be) external
events), and it represents what client code thinks about its state.

|state_| is the state machine, and has transition rules based on its
circumstances, including old |state_| value, |enabled_| value, and other
conditions (like whether ARC is running/stopped etc.). (Though, current code has
some problematic state transition, e.g. STOPPED is immediately set on ARC
instance stop triggering, while it takes time, etc.).

So, although technically we can merge those two variable into one, but it just
makes state-machine complicated (Actually, we need all current State enum *
enable/disable combination).

Luis, I added comment and TODO, to document, after the fix of state transition.
Does it work for you?

Yury, I agree that the logic around |enabled_| and |state_| machine code is
complicated. Though, due to complicated dependency we now have here, it is not
simple task to clean them up. Actually, this is one of the CL series to rework
on it.
As for "arc.enabled" preference, it will be removed from this class. Please let
me work on it in a following CL, to keep the CL size reasonably smaller for each
step.

[Luis Héctor Chávez, 2017/02/15 21:54:43]

OK, then since this is orthogonal to |state_|, it does make sense to have it as
a separate bit. Thanks for the explanation.
This is what I suspected. Let's keep |enabled_| separate from |state_|.
Totally!

   ShutdownSession();
   if (support_host_) {
     support_host_->Close();
@@ -520,8 +525,8 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
     }
   }
 
-  for (auto& observer : observer_list_)
-    observer.OnArcPlayStoreEnabledChanged(arc_enabled);
+  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.
@@ -530,87 +535,13 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
     ArcAuthNotification::Hide();
   }
 
-  if (!arc_enabled) {
-    // Reset any pending request to re-enable ARC.
-    VLOG(1) << "ARC opt-out. Removing user data.";
-    reenable_arc_ = false;
-    StopArc();
-    RemoveArcData();
-    return;
-  }
-
-  if (state_ == State::ACTIVE)
-    return;
-
-  if (state_ == State::REMOVING_DATA_DIR) {
-    // Data removal request is in progress. Set flag to re-enable Arc once it is
-    // finished.
-    reenable_arc_ = true;
-    return;
-  }
-
-  if (support_host_)
-    support_host_->SetArcManaged(IsArcManaged());
-
-  // 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())
-    profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
-
-  // If it is marked that sign in has been successfully done, then directly
-  // start ARC.
-  // For testing, and for Kisok mode, we also skip ToS negotiation procedure.
-  // For backward compatibility, this check needs to be prior to the
-  // kArcTermsAccepted check below.
-  if (profile_->GetPrefs()->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()));
-    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 (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
-    // Don't show UI for this progress if it was not shown.
-    if (support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE)
-      support_host_->ShowArcLoading();
-    StartArcAndroidManagementCheck();
-    return;
-  }
+  if (arc_enabled)
+    Enable();
+  else
+    Disable();
 
-  // Need user's explicit Terms Of Service agreement. Prevent race condition
-  // when ARC can be enabled before profile is synced. In last case
-  // OnOptInPreferenceChanged is called twice.
-  // TODO(crbug.com/687185): Remove the condition.
-  if (state_ != State::SHOWING_TERMS_OF_SERVICE)
-    StartTermsOfServiceNegotiation();
+  for (auto& observer : observer_list_)
+    observer.OnArcPlayStoreEnabledChanged(arc_enabled);
 }
 
 void ArcSessionManager::ShutdownSession() {
@@ -748,7 +679,106 @@ void ArcSessionManager::RecordArcState() {
   // Only record Enabled state if ARC is allowed in the first place, so we do
   // not split the ARC population by devices that cannot run ARC.
   if (IsAllowed())
-    UpdateEnabledStateUMA(IsArcPlayStoreEnabled());
+    UpdateEnabledStateUMA(enabled_);
+}
+
+void ArcSessionManager::Enable() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile_);
+
+  if (enabled_) {
+    VLOG(1) << "ARC is already enabled. Do nothing.";
+    return;
+  }
+  enabled_ = true;
+
+  VLOG(1) << "ARC opt-in. Starting ARC session.";
+  EnableImpl();
+}
+
+void ArcSessionManager::EnableImpl() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile_);
+  DCHECK_NE(state_, State::ACTIVE);
+
+  if (state_ == State::REMOVING_DATA_DIR) {
+    // Data removal request is in progress. Set flag to re-enable Arc once it is

[Daniel Erat, 2017/02/15 14:50:05]

nit: s/Arc/ARC/

[hidehiko, 2017/02/15 19:45:11]

Done.

+    // finished.
+    reenable_arc_ = true;
+    return;
+  }
+
+  // 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())
+    profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
+
+  // If it is marked that sign in has been successfully done, then directly
+  // start ARC.
+  // For testing, and for Kisok mode, we also skip ToS negotiation procedure.
+  // For backward compatibility, this check needs to be prior to the
+  // kArcTermsAccepted check below.
+  if (profile_->GetPrefs()->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()));
+    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 (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
+    // Don't show UI for this progress if it was not shown.
+    if (support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE)
+      support_host_->ShowArcLoading();
+    StartArcAndroidManagementCheck();
+    return;
+  }
+
+  StartTermsOfServiceNegotiation();

[hidehiko, 2017/02/15 14:17:00]

Note: dup call check is no longer needed, because it is guarded by |enabled_| in
Enable().

+}
+
+void ArcSessionManager::Disable() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile_);
+
+  if (!enabled_) {
+    VLOG(1) << "ARC is already disabled. Do nothing.";
+    return;
+  }
+  enabled_ = false;
+
+  // Reset any pending request to re-enable ARC.
+  VLOG(1) << "ARC opt-out. Removing user data.";
+  reenable_arc_ = false;
+  StopArc();
+  RemoveArcData();
 }
 
 void ArcSessionManager::StartTermsOfServiceNegotiation() {