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 152 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);
 
+  // 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();
 
   // 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.
+  // 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);
 
   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_;
 
+  bool enabled_ = false;
   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_