Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

chrome/browser/chromeos/arc/arc_session_manager.cc

Index: chrome/browser/chromeos/arc/arc_session_manager.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager.cc b/chrome/browser/chromeos/arc/arc_session_manager.cc
index d7486cc88a207a16115fe9586493505ef165cd12..71ac2bcecc7244e5bf076c21708399261605319a 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -40,6 +40,7 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "components/arc/arc_bridge_service.h"
+#include "components/arc/arc_session_runner.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/sync_preferences/pref_service_syncable.h"
@@ -81,22 +82,22 @@ ash::ShelfDelegate* GetShelfDelegate() {
 
 }  // namespace
 
-ArcSessionManager::ArcSessionManager(ArcBridgeService* bridge_service)
-    : ArcService(bridge_service),
+ArcSessionManager::ArcSessionManager(
+    std::unique_ptr<ArcSessionRunner> arc_session_runner)
+    : arc_session_runner_(std::move(arc_session_runner)),
       attempt_user_exit_callback_(base::Bind(chrome::AttemptUserExit)),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!g_arc_session_manager);
   g_arc_session_manager = this;
-
-  arc_bridge_service()->AddObserver(this);
+  arc_session_runner_->AddObserver(this);
 }
 
 ArcSessionManager::~ArcSessionManager() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   Shutdown();
-  arc_bridge_service()->RemoveObserver(this);
+  arc_session_runner_->RemoveObserver(this);
 
   DCHECK_EQ(this, g_arc_session_manager);
   g_arc_session_manager = nullptr;
@@ -193,6 +194,11 @@ bool ArcSessionManager::IsArcKioskMode() {
   return user_manager::UserManager::Get()->IsLoggedInAsArcKioskApp();
 }
 
+void ArcSessionManager::OnSessionReady() {
+  for (auto& observer : arc_session_observer_list_)
+    observer.OnSessionReady();
+}
+
 void ArcSessionManager::OnSessionStopped(StopReason reason) {
   // TODO(crbug.com/625923): Use |reason| to report more detailed errors.
   if (arc_sign_in_timer_.IsRunning())
@@ -201,7 +207,7 @@ void ArcSessionManager::OnSessionStopped(StopReason reason) {
   if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
     // This should be always true, but just in case as this is looked at
     // inside RemoveArcData() at first.
-    DCHECK(arc_bridge_service()->stopped());
+    DCHECK(arc_session_runner_->IsStopped());
     RemoveArcData();
   } else {
     // To support special "Stop and enable ARC" procedure for enterprise,
@@ -211,6 +217,9 @@ void ArcSessionManager::OnSessionStopped(StopReason reason) {
         FROM_HERE, base::Bind(&ArcSessionManager::MaybeReenableArc,
                               weak_ptr_factory_.GetWeakPtr()));
   }
+
+  for (auto& observer : arc_session_observer_list_)
+    observer.OnSessionStopped(reason);
 }
 
 void ArcSessionManager::RemoveArcData() {
@@ -221,8 +230,8 @@ void ArcSessionManager::RemoveArcData() {
   // OnArcDataRemoved resets this flag.
   profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, true);
 
-  if (!arc_bridge_service()->stopped()) {
-    // Just set a flag. On bridge stopped, this will be re-called,
+  if (!arc_session_runner_->IsStopped()) {
+    // Just set a flag. On session stopped, this will be re-called,
     // then session manager should remove the data.
     return;
   }
@@ -256,7 +265,7 @@ void ArcSessionManager::OnArcDataRemoved(bool success) {
 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, OnBridgeStopped() should trigger
+  // "on managed lost" case. In that case, OnSessionStopped() should trigger
   // the RemoveArcData(), then this.
   if (!reenable_arc_ || !IsArcEnabled())
     return;
@@ -271,10 +280,10 @@ void ArcSessionManager::MaybeReenableArc() {
 void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Due asynchronous nature of stopping Arc bridge, OnProvisioningFinished may
-  // arrive after setting the |State::STOPPED| state and |State::Active| is not
-  // guaranty set here. prefs::kArcDataRemoveRequested is also can be active
-  // for now.
+  // Due asynchronous nature of stopping ARC instance, OnProvisioningFinished

[Luis Héctor Chávez, 2016/12/22 18:29:56]

nit: the ARC instance

[hidehiko, 2016/12/22 19:04:12]

Done.

+  // may arrive after setting the |State::STOPPED| state and |State::Active|
+  // is not guaranty set here. prefs::kArcDataRemoveRequested is also can be

[Luis Héctor Chávez, 2016/12/22 18:29:56]

nit: s/guaranty/guaranteed to be/;s/is also can be/also can be/

[hidehiko, 2016/12/22 19:04:12]

Done.

+  // active for now.
 
   if (provisioning_reported_) {
     // We don't expect ProvisioningResult::SUCCESS is reported twice or reported
@@ -368,7 +377,7 @@ void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
       result == ProvisioningResult::CHROME_SERVER_COMMUNICATION_ERROR) {
     if (profile_->GetPrefs()->HasPrefPath(prefs::kArcSignedIn))
       profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, false);
-    ShutdownBridge();
+    ShutdownSession();
     if (support_host_)
       support_host_->ShowError(error, false);
     return;
@@ -385,8 +394,8 @@ void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
     RemoveArcData();
   }
 
-  // We'll delay shutting down the bridge in this case to allow people to send
-  // feedback.
+  // We'll delay shutting down the ARC instance in this case to allow people
+  // to send feedback.
   if (support_host_)
     support_host_->ShowError(error, true /* = show send feedback button */);
 }
@@ -491,7 +500,7 @@ void ArcSessionManager::Shutdown() {
   if (!g_disable_ui_for_testing)
     ArcAuthNotification::Hide();
 
-  ShutdownBridge();
+  ShutdownSession();
   if (support_host_) {
     support_host_->Close();
     support_host_->RemoveObserver(this);
@@ -533,7 +542,7 @@ void ArcSessionManager::StopArc() {
     profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, false);
     profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, false);
   }
-  ShutdownBridge();
+  ShutdownSession();
   if (support_host_)
     support_host_->Close();
 }
@@ -632,12 +641,15 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
   StartTermsOfServiceNegotiation();
 }
 
-void ArcSessionManager::ShutdownBridge() {
+void ArcSessionManager::ShutdownSession() {
   arc_sign_in_timer_.Stop();
   playstore_launcher_.reset();
   terms_of_service_negotiator_.reset();
   android_management_checker_.reset();
-  arc_bridge_service()->RequestStop();
+  arc_session_runner_->RequestStop();
+  // TODO(hidehiko): Actual ARC instance stop will be delayed, and until
+  // it's completed, ARC instance is still running. Do not set the state
+  // STOPPED immediately here.

[Luis Héctor Chávez, 2016/12/22 18:29:56]

just curious: is the plan to delete IsSessionStopped() and have
IsSessionRunning() be the only source of truth?

If that's the case, maybe it's better to change all the checks in
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc to only use
IsSessionRunning() to avoid future changes there. (only if the weird nature of
both flags not being exactly opposite of each other doesn't interact badly with
the test, but given that they all used ready() I *think* it should be fine).

[hidehiko, 2016/12/22 19:04:12]

TBD. Currently, IsSessionStopped()/Running() is just proxy from
ArcSessionRunner. IMHO, whether Actually ARC is running or not looks too details
for this layer, and maybe it's better to avoid exposing them for encapsulation.
However, for now, it looks not yet clear enough to me whether it is technically
doable. So, let's address it separately.

As for testing code, indeed the original code looked to assume "STOPPED ==
!RUNNING", but it is not true, as you know. So, I'd like to keep the current
code to follow the current semantics, for now.

WDYT?

   if (state_ != State::NOT_INITIALIZED && state_ != State::REMOVING_DATA_DIR)
     SetState(State::STOPPED);
   for (auto& observer : observer_list_)
@@ -654,11 +666,31 @@ void ArcSessionManager::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+void ArcSessionManager::AddSessionObserver(ArcSessionObserver* observer) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  arc_session_observer_list_.AddObserver(observer);
+}
+
+void ArcSessionManager::RemoveSessionObserver(ArcSessionObserver* observer) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  arc_session_observer_list_.RemoveObserver(observer);
+}
+
+bool ArcSessionManager::IsSessionRunning() const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return arc_session_runner_->IsRunning();
+}
+
+bool ArcSessionManager::IsSessionStopped() const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return arc_session_runner_->IsStopped();
+}
+
 // This is the special method to support enterprise mojo API.
 // TODO(hidehiko): Remove this.
 void ArcSessionManager::StopAndEnableArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(!arc_bridge_service()->stopped());
+  DCHECK(!arc_session_runner_->IsStopped());
   reenable_arc_ = true;
   StopArc();
 }
@@ -671,7 +703,7 @@ void ArcSessionManager::StartArc() {
 
   provisioning_reported_ = false;
 
-  arc_bridge_service()->RequestStart();
+  arc_session_runner_->RequestStart();
   SetState(State::ACTIVE);
 }
 
@@ -758,9 +790,10 @@ void ArcSessionManager::StartTermsOfServiceNegotiation() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!terms_of_service_negotiator_);
 
-  if (!arc_bridge_service()->stopped()) {
-    // If the user attempts to re-enable ARC while the bridge is still running
-    // the user should not be able to continue until the bridge has stopped.
+  if (!arc_session_runner_->IsStopped()) {
+    // If the user attempts to re-enable ARC while ARC instance is still

[Luis Héctor Chávez, 2016/12/22 18:29:56]

nit: the ARC instance. same below.

[hidehiko, 2016/12/22 19:04:12]

Done.

+    // running the user should not be able to continue until ARC instance has
+    // stopped.
     if (support_host_) {
       support_host_->ShowError(
           ArcSupportHost::Error::SIGN_IN_SERVICE_UNAVAILABLE_ERROR, false);
@@ -799,7 +832,7 @@ void ArcSessionManager::OnTermsOfServiceNegotiated(bool accepted) {
 
 void ArcSessionManager::StartArcAndroidManagementCheck() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(arc_bridge_service()->stopped());
+  DCHECK(arc_session_runner_->IsStopped());
   DCHECK(state_ == State::SHOWING_TERMS_OF_SERVICE ||
          state_ == State::CHECKING_ANDROID_MANAGEMENT);
   SetState(State::CHECKING_ANDROID_MANAGEMENT);
@@ -828,7 +861,7 @@ void ArcSessionManager::OnAndroidManagementChecked(
       StartArc();
       break;
     case policy::AndroidManagementClient::Result::MANAGED:
-      ShutdownBridge();
+      ShutdownSession();
       if (support_host_) {
         support_host_->ShowError(
             ArcSupportHost::Error::ANDROID_MANAGEMENT_REQUIRED_ERROR, false);
@@ -836,7 +869,7 @@ void ArcSessionManager::OnAndroidManagementChecked(
       UpdateOptInCancelUMA(OptInCancelReason::ANDROID_MANAGEMENT_REQUIRED);
       break;
     case policy::AndroidManagementClient::Result::ERROR:
-      ShutdownBridge();
+      ShutdownSession();
       if (support_host_) {
         support_host_->ShowError(
             ArcSupportHost::Error::SERVER_COMMUNICATION_ERROR, false);
@@ -893,13 +926,13 @@ void ArcSessionManager::OnRetryClicked() {
   } else if (!profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
     StartTermsOfServiceNegotiation();
   } else if (support_host_->ui_page() == ArcSupportHost::UIPage::ERROR &&
-             !arc_bridge_service()->stopped()) {
+             !arc_session_runner_->IsStopped()) {
     // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, stopping
     // ARC was postponed to contain its internal state into the report.
     // Here, on retry, stop it, then restart.
     DCHECK_EQ(State::ACTIVE, state_);
     support_host_->ShowArcLoading();
-    ShutdownBridge();
+    ShutdownSession();
     reenable_arc_ = true;
   } else if (state_ == State::ACTIVE) {
     // This case is handled in ArcAuthService.
@@ -918,6 +951,16 @@ void ArcSessionManager::OnSendFeedbackClicked() {
   chrome::OpenFeedbackDialog(nullptr);
 }
 
+void ArcSessionManager::SetArcSessionRunnerForTesting(
+    std::unique_ptr<ArcSessionRunner> arc_session_runner) {
+  DCHECK(arc_session_runner);
+  DCHECK(arc_session_runner_);
+  DCHECK(arc_session_runner_->IsStopped());
+  arc_session_runner_->RemoveObserver(this);
+  arc_session_runner_ = std::move(arc_session_runner);
+  arc_session_runner_->AddObserver(this);
+}
+
 void ArcSessionManager::SetAttemptUserExitCallbackForTesting(
     const base::Closure& callback) {
   DCHECK(!callback.is_null());