Split IsArcPlayStoreEnabled() and ArcSessionManager's enabled concepts.

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 213 matching lines @@
 
   MaybeReenableArc();
 }
 
 void ArcSessionManager::MaybeReenableArc() {
   // Here check if |reenable_arc_| is marked or not.
   // The only case this happens should be in the special case for enterprise
   // "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) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // If the Mojo message to notify finishing the provisioning is already sent
   // from the container, it will be processed even after requesting to stop the
   // 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;
   }
 
   // Due asynchronous nature of stopping the ARC instance,
   // OnProvisioningFinished may arrive after setting the |State::STOPPED| state
   // and |State::Active| is not guaranteed to be set here.
   // prefs::kArcDataRemoveRequested also can be active for now.
 
@@ skipping 147 matching lines @@
   // TODO(hidehiko): Revisit to think about lazy initialization.
   //
   // Don't show UI for ARC Kiosk because the only one UI in kiosk mode must
   // be the kiosk app. In case of error the UI will be useless as well, because
   // in typical use case there will be no one nearby the kiosk device, who can
   // do some action to solve the problem be means of UI.
   if (!g_disable_ui_for_testing && !IsArcOptInVerificationDisabled() &&
       !IsArcKioskMode()) {
     DCHECK(!support_host_);
     support_host_ = base::MakeUnique<ArcSupportHost>(profile_);
+    support_host_->SetArcManaged(IsArcManaged());
     support_host_->AddObserver(this);
   }
 
   DCHECK_EQ(State::NOT_INITIALIZED, state_);
   SetState(State::STOPPED);
 
   context_.reset(new ArcAuthContext(profile_));
 
   if (!g_disable_ui_for_testing ||
       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();
   }
 }
 
 void ArcSessionManager::OnIsSyncingChanged() {
   sync_preferences::PrefServiceSyncable* const pref_service_syncable =
       PrefServiceSyncableFromProfile(profile_);
   if (!pref_service_syncable->IsSyncing())
     return;
 
   pref_service_syncable->RemoveObserver(this);
   if (!g_disable_ui_for_testing &&
       !base::CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kEnableArcOOBEOptIn) &&
       profile_->IsNewProfile() &&
       !profile_->GetPrefs()->HasPrefPath(prefs::kArcEnabled)) {
     ArcAuthNotification::Show(profile_);
   }
 }
 
 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();
     support_host_->RemoveObserver(this);
     support_host_.reset();
   }
   if (profile_) {
     sync_preferences::PrefServiceSyncable* pref_service_syncable =
         PrefServiceSyncableFromProfile(profile_);
     pref_service_syncable->RemoveObserver(this);
@@ skipping 27 matching lines @@
     if (!arc_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);
     }
   }
 
-  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.
   if (!g_disable_ui_for_testing &&
       profile_->GetPrefs()->HasPrefPath(prefs::kArcEnabled)) {
     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 (arc_enabled)
+    Enable();
+  else
+    Disable();
 
-  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;
-  }
-
-  // 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() {
   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 117 matching lines @@
     return;
   }
 
   profile_->GetPrefs()->SetBoolean(prefs::kArcEnabled, enabled);
 }
 
 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() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!terms_of_service_negotiator_);
 
   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
     // has stopped.
@@ skipping 200 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