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..90285b92c468b4fc882850e5c285e675048d2fe2 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.h
+++ b/chrome/browser/chromeos/arc/arc_session_manager.h
@@ -170,6 +170,20 @@ class ArcSessionManager : public ArcSessionObserver,
   // Google Play Store, then ARC can run without opt-in.
   void SetArcPlayStoreEnabled(bool enable);
 
+  // Enabels ARC session. This triggers to start ARC instance, or maybe

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

nit: "Enables"

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

Done.

+  // to start Terms Of Service negotiation.

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

maybe "This starts the ARC instance (or displays the ToS if they haven't been
accepted yet)."?

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

Updated the comment. Note: ToS negotiation have various ways depending on the
situation for ARC. To reflect it, I just avoided to use "display" / "close" word
explicitly.

+  // If it is already enabled, no-op.
+  // Currently, enabled/disabled is tried to whether Google Play Store is

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

nit: s/tried/tied/ ?

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

Good catch. Done.

+  // enabled or disabled.
+  void Enable();

[khmel, 2017/02/15 15:54:34]

These functions are public, Why do we need to have it public? Is any test case,
consumer, unit tests for this?

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

This is preparation to extract preference handler related code from this class.
In following CLs, Enable()/Disable() will be called from a new class, so these
are public.

We can temporarily put them in private now, and can move in the following CLs,
but it sounds redundant to me, so I just put them into public: section.

As for testing, currently existing ones should cover them.
In a following CL to extract preference handler from here, I'll rework on
unittests.

+
+  // Disables ARC session. This triggers to stop ARC instance, or if it is

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

"This stops the ARC instance (or closes the ToS if they're shown)."

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

Done. Similar to Enable() case, I avoided to limit ToS related stuff to UI.

+  // middle of Terms Of Service negotiation, closes its UI etc.
+  // If it is already disabled, no-op.
+  // Currently, enabled/disabled is tried to whether Google Play Store is

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

i'd delete this sentence since you have it earlier

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

I see. Done.

+  // enabled or disabled.
+  void Disable();
+
   // Called from the Chrome OS metrics provider to record Arc.State
   // periodically.
   void RecordArcState();
@@ -222,6 +236,10 @@ class ArcSessionManager : public ArcSessionObserver,
   void SetAttemptUserExitCallbackForTesting(const base::Closure& callback);
 
  private:
+  // Enable() has a check in order not to trigger starting procedure twice.
+  // For restarting, we would like to workaround it, and this is the method.

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

nit: "This method can be called to bypass that check when restarting."

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

Thank you for good suggestion! Done.

+  void EnableImpl();
+
   // Negotiates the terms of service to user.
   void StartTermsOfServiceNegotiation();
   void OnTermsOfServiceNegotiated(bool accepted);
@@ -260,6 +278,7 @@ class ArcSessionManager : public ArcSessionObserver,
   // Registrar used to monitor ARC enabled state.
   PrefChangeRegistrar pref_change_registrar_;
 
+  bool enabled_ = false;
   State state_ = State::NOT_INITIALIZED;
   base::ObserverList<Observer> observer_list_;
   base::ObserverList<ArcSessionObserver> arc_session_observer_list_;