Split IsArcPlayStoreEnabled() and ArcSessionManager's enabled concepts.

chrome/browser/chromeos/arc/arc_session_manager.h

 // 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.
 
 #ifndef CHROME_BROWSER_CHROMEOS_ARC_ARC_SESSION_MANAGER_H_
 #define CHROME_BROWSER_CHROMEOS_ARC_ARC_SESSION_MANAGER_H_
 
 #include <memory>
 #include <ostream>
 #include <string>
@@ skipping 59 matching lines @@
   // In the first boot case:
   //   STOPPED -> SHOWING_TERMS_OF_SERVICE: when arc.enabled preference is set.
   //   SHOWING_TERMS_OF_SERVICE -> CHECKING_ANDROID_MANAGEMENT: when a user
   //     agree with "Terms Of Service"
   //   CHECKING_ANDROID_MANAGEMENT -> ACTIVE: when the auth token is
   //     successfully fetched.
   //
   // 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,
     SHOWING_TERMS_OF_SERVICE,
     CHECKING_ANDROID_MANAGEMENT,
     REMOVING_DATA_DIR,
     ACTIVE,
   };
 
   class Observer {
@@ skipping 73 matching lines @@
   bool IsArcPlayStoreEnabled() const;
 
   // Enables/disables Google Play Store on ARC. Currently, it is tied to
   // ARC enabled state, too, so this also should trigger to enable/disable
   // whole ARC system.
   // TODO(hidehiko): De-couple the concept to enable ARC system and opt-in
   // to use Google Play Store. Note that there is a plan to use ARC without
   // 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();
 
   // sync_preferences::PrefServiceSyncableObserver
   void OnIsSyncingChanged() override;
 
   // ArcSupportHost::Observer:
   void OnWindowClosed() override;
   void OnTermsAgreed(bool is_metrics_enabled,
@@ skipping 32 matching lines @@
   // Returns the time when ARC was about to start, or a null time if ARC has not
   // been started yet.
   base::Time arc_start_time() const { return arc_start_time_; }
 
   // Injectors for testing.
   void SetArcSessionRunnerForTesting(
       std::unique_ptr<ArcSessionRunner> arc_session_runner);
   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);
 
   void SetState(State state);
   void ShutdownSession();
   void OnOptInPreferenceChanged();
   void OnAndroidManagementPassed();
   void OnArcDataRemoved(bool success);
   void OnArcSignInTimeout();
@@ skipping 18 matching lines @@
   void OnSessionStopped(StopReason reason) override;
 
   std::unique_ptr<ArcSessionRunner> arc_session_runner_;
 
   // Unowned pointer. Keeps current profile.
   Profile* profile_ = nullptr;
 
   // 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_;
   std::unique_ptr<ArcAppLauncher> playstore_launcher_;
   bool reenable_arc_ = false;
   bool provisioning_reported_ = false;
   base::OneShotTimer arc_sign_in_timer_;
 
   std::unique_ptr<ArcSupportHost> support_host_;
 
@@ skipping 14 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(ArcSessionManager);
 };
 
 // Outputs the stringified |state| to |os|. This is only for logging purposes.
 std::ostream& operator<<(std::ostream& os,
                          const ArcSessionManager::State& state);
 
 }  // namespace arc
 
 #endif  // CHROME_BROWSER_CHROMEOS_ARC_ARC_SESSION_MANAGER_H_