Split IsArcPlayStoreEnabled() and ArcSessionManager's enabled concepts.

chrome/browser/chromeos/arc/arc_session_manager.h

Index: chrome/browser/chromeos/arc/arc_session_manager.h
diff --git a/chrome/browser/chromeos/arc/arc_session_manager.h b/chrome/browser/chromeos/arc/arc_session_manager.h
index 7c97cdc1aaabc2c93fa74e8e5700f3ff54e016fe..4bee8722172448c364d28d691c0a8575b68960fe 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.h
+++ b/chrome/browser/chromeos/arc/arc_session_manager.h
@@ -77,6 +77,9 @@ class ArcSessionManager : public ArcSessionObserver,
   // In the second (or later) boot case:
   //   STOPPED -> ACTIVE: when arc.enabled preference is checked that it is
   //     true. Practically, this is when the primary Profile gets ready.
+  //
+  // TODO(hidehiko): Fix the state machine, and update the comment including
+  // relationship with |enabled_|.
   enum class State {
     NOT_INITIALIZED,
     STOPPED,
@@ -170,6 +173,19 @@ class ArcSessionManager : public ArcSessionObserver,
   // Google Play Store, then ARC can run without opt-in.
   void SetArcPlayStoreEnabled(bool enable);
 
+  // Enables ARC session. This starts ARC instance, or maybe starts Terms Of
+  // Service negotiation if they haven't been accepted yet.
+  // If it is already enabled, no-op.
+  // Currently, enabled/disabled is tied to whether Google Play Store is
+  // enabled or disabled. Please see also TODO of SetArcPlayStoreEnabled().
+  void Enable();
+
+  // Disables ARC session. This stops ARC instance, or quits Terms Of Service
+  // negotiation if it is the middle of the process (e.g. closing UI for
+  // manual negotiation if it is shown).
+  // If it is already disabled, no-op.
+  void Disable();
+
   // Called from the Chrome OS metrics provider to record Arc.State
   // periodically.
   void RecordArcState();
@@ -222,6 +238,10 @@ class ArcSessionManager : public ArcSessionObserver,
   void SetAttemptUserExitCallbackForTesting(const base::Closure& callback);
 
  private:
+  // Enable() has a check in order not to trigger starting procedure twice.
+  // This method can be called to bypass that check when restarting.
+  void EnableImpl();
+
   // Negotiates the terms of service to user.
   void StartTermsOfServiceNegotiation();
   void OnTermsOfServiceNegotiated(bool accepted);
@@ -260,6 +280,11 @@ class ArcSessionManager : public ArcSessionObserver,
   // Registrar used to monitor ARC enabled state.
   PrefChangeRegistrar pref_change_registrar_;
 
+  // Whether ArcSessionManager is enabled (starting to run ARC instance) or

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

nit: given that you want to have this represent what the client's intention is,
should re rename this to |enable_requested_| or something along those lines?

Also, can you update the comment so that it says that by setting this to true it
triggers the start of the state machine that ultimately enables ARC?

(The main motivation for this nit-pickiness is that we have over-used the term
"enabled" in the past and this has brought us to do some amount of change to be
a bit more nuanced with the actual meaning. Can we preemptively do that this
time?)

[hidehiko, 2017/02/16 02:44:28]

Makes sense. Done.

+  // not.
+  bool enabled_ = false;
+
+  // Internal state machine. See also State enum class.
   State state_ = State::NOT_INITIALIZED;
   base::ObserverList<Observer> observer_list_;
   base::ObserverList<ArcSessionObserver> arc_session_observer_list_;