Refactor ArcBridgeServiceImpl part 2.

components/arc/arc_bridge_service_impl.cc

Index: components/arc/arc_bridge_service_impl.cc
diff --git a/components/arc/arc_bridge_service_impl.cc b/components/arc/arc_bridge_service_impl.cc
index 0fb2b8d42d8bb0dbe2469dc2df6482e258c4ca81..524b13ab069c594f7bf9af0e0e21b941b8317156 100644
--- a/components/arc/arc_bridge_service_impl.cc
+++ b/components/arc/arc_bridge_service_impl.cc
@@ -4,22 +4,10 @@
 
 #include "components/arc/arc_bridge_service_impl.h"
 
-#include <string>
-#include <utility>
-
-#include "base/command_line.h"
-#include "base/json/json_writer.h"
-#include "base/sequenced_task_runner.h"
-#include "base/sys_info.h"
-#include "base/task_runner_util.h"
-#include "base/threading/thread_task_runner_handle.h"
-#include "base/time/time.h"
-#include "chromeos/chromeos_switches.h"
-#include "chromeos/dbus/dbus_method_call_status.h"
-#include "chromeos/dbus/dbus_thread_manager.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/task_runner.h"
 #include "components/arc/arc_session.h"
-#include "components/prefs/pref_registry_simple.h"
-#include "components/prefs/pref_service.h"
 
 namespace arc {
 namespace {
@@ -31,39 +19,77 @@ constexpr base::TimeDelta kDefaultRestartDelay =
 
 ArcBridgeServiceImpl::ArcBridgeServiceImpl(
     const scoped_refptr<base::TaskRunner>& blocking_task_runner)
-    : session_started_(false),
-      restart_delay_(kDefaultRestartDelay),
+    : restart_delay_(kDefaultRestartDelay),
       factory_(base::Bind(ArcSession::Create, this, blocking_task_runner)),
-      weak_factory_(this) {}
+      weak_ptr_factory_(this) {}
 
 ArcBridgeServiceImpl::~ArcBridgeServiceImpl() {
+  DCHECK(CalledOnValidThread());
   if (arc_session_)
     arc_session_->RemoveObserver(this);
 }
 
 void ArcBridgeServiceImpl::RequestStart() {
   DCHECK(CalledOnValidThread());
-  if (session_started_)
+
+  // Consecutive RequestStart() call. Do nothing.
+  if (running_)
     return;
+
   VLOG(1) << "Session started";
-  session_started_ = true;
-  PrerequisitesChanged();
+  running_ = true;
+  // Here the |running_| becomes false to true. So, |restart_timer_| must be

[Luis Héctor Chávez, 2016/12/16 23:20:12]

nit: "Here |running_| transitions from false to true."

[hidehiko, 2016/12/19 08:05:11]

Done.

+  // stopped (either not even started, or has been cancelled in previous
+  // RequestStop() call).
+  DCHECK(!restart_timer_.IsRunning());
+
+  if (arc_session_) {
+    // This case, a client calls RequestStop() in advance, then soon

[Luis Héctor Chávez, 2016/12/16 23:20:12]

nit: "In this case, RequestStop() was called, and before |arc_session_| had
finished stopping, RequestStart() was called. Do nothing in that case, since
when |arc_session_| does actually stop, OnSessionStopped() will be called, where
it should automatically restart."

[hidehiko, 2016/12/19 08:05:11]

Done.

+    // RequestStart(). In such a case, |arc_session_| must be requested
+    // to stop already. Then, do nothing now, and later in OnSessionStopped(),
+    // restarting should be handled.
+    DCHECK_EQ(state(), State::STOPPING);
+  } else {
+    DCHECK_EQ(state(), State::STOPPED);
+    StartArcSession();
+  }
 }
 
 void ArcBridgeServiceImpl::RequestStop() {
   DCHECK(CalledOnValidThread());
-  if (!session_started_)
+
+  // Consecutive RequestStop() call. Do nothing.
+  if (!running_)
     return;
+
   VLOG(1) << "Session ended";
-  session_started_ = false;
-  PrerequisitesChanged();
+  running_ = false;
+
+  if (arc_session_) {
+    // The |state_| could be either STARTING, RUNNING or STOPPING.
+    DCHECK_NE(state(), State::STOPPED);
+
+    // STOPPING is found in the senario of "RequestStart() -> RequestStop()
+    // -> RequestStart() -> RequestStop()" case.
+    // In the first RequestStop() call, |state_| is set to STOPPING,
+    // and in the second RequestStop() finds it (so this is the second call).
+    // In that case, ArcSession::Stop() is already called, so do nothing.
+    if (state() != State::STOPPING) {

[Luis Héctor Chávez, 2016/12/16 23:20:12]

nit:

if (state() == State::STOPPING) {
  // STOPPING is found ...
  return;
}

that way "do nothing" is immediately followed by an explicit return that does
nothing.

[hidehiko, 2016/12/19 08:05:11]

Done.

+      SetState(State::STOPPING);
+      arc_session_->Stop();
+    }
+  } else {
+    DCHECK_EQ(state(), State::STOPPED);
+    // In case restarting is in progress, cancel it.
+    restart_timer_.Stop();
+  }
 }
 
 void ArcBridgeServiceImpl::OnShutdown() {
   DCHECK(CalledOnValidThread());
 
   VLOG(1) << "OnShutdown";
-  session_started_ = false;
+  running_ = false;
   restart_timer_.Stop();
   if (arc_session_)
     arc_session_->OnShutdown();
@@ -75,6 +101,9 @@ void ArcBridgeServiceImpl::OnShutdown() {
 void ArcBridgeServiceImpl::SetArcSessionFactoryForTesting(
     const ArcSessionFactory& factory) {
   DCHECK(!factory.is_null());
+  DCHECK_EQ(state(), State::STOPPED);
+  DCHECK(!arc_session_);
+  DCHECK(!restart_timer_.IsRunning());
   factory_ = factory;
 }
 
@@ -86,29 +115,6 @@ void ArcBridgeServiceImpl::SetRestartDelayForTesting(
   restart_delay_ = restart_delay;
 }
 
-void ArcBridgeServiceImpl::PrerequisitesChanged() {
-  DCHECK(CalledOnValidThread());
-  VLOG(1) << "Prerequisites changed. "
-          << "state=" << static_cast<uint32_t>(state())
-          << ", session_started=" << session_started_;
-  if (state() == State::STOPPED) {
-    if (!session_started_) {
-      // We were explicitly asked to stop, so do not restart.
-      restart_timer_.Stop();
-      return;
-    }
-    DCHECK(!restart_timer_.IsRunning());
-    VLOG(0) << "Prerequisites met, starting ARC";
-    StartArcSession();
-  } else {
-    DCHECK(!restart_timer_.IsRunning());
-    if (session_started_)
-      return;
-    VLOG(0) << "Prerequisites stopped being met, stopping ARC";
-    StopInstance();
-  }
-}
-
 void ArcBridgeServiceImpl::StartArcSession() {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(state(), State::STOPPED);
@@ -119,41 +125,24 @@ void ArcBridgeServiceImpl::StartArcSession() {
   SetStopReason(StopReason::SHUTDOWN);
   arc_session_ = factory_.Run();
   arc_session_->AddObserver(this);
-  SetState(State::CONNECTING);
+  SetState(State::STARTING);
   arc_session_->Start();
 }
 
-void ArcBridgeServiceImpl::StopInstance() {
-  DCHECK(CalledOnValidThread());
-  DCHECK_NE(state(), State::STOPPED);
-  DCHECK(!restart_timer_.IsRunning());
-
-  if (state() == State::STOPPING) {
-    VLOG(1) << "StopInstance() called when ARC is not running";
-    return;
-  }
-
-  VLOG(1) << "Stopping ARC";
-  DCHECK(arc_session_.get());
-  SetState(State::STOPPING);
-
-  // Note: this can call OnStopped() internally as a callback.
-  arc_session_->Stop();
-}
-
 void ArcBridgeServiceImpl::OnSessionReady() {
   DCHECK(CalledOnValidThread());
-  if (state() != State::CONNECTING) {
-    VLOG(1) << "StopInstance() called while connecting";
-    return;
-  }
+  DCHECK_EQ(state(), State::STARTING);
+  DCHECK(arc_session_);
+  DCHECK(!restart_timer_.IsRunning());
 
   VLOG(0) << "ARC ready";
-  SetState(State::READY);
+  SetState(State::RUNNING);
 }
 
 void ArcBridgeServiceImpl::OnSessionStopped(StopReason stop_reason) {
   DCHECK(CalledOnValidThread());
+  DCHECK_NE(state(), State::STOPPED);
+  DCHECK(arc_session_);
   DCHECK(!restart_timer_.IsRunning());
 
   VLOG(0) << "ARC stopped: " << stop_reason;
@@ -165,8 +154,7 @@ void ArcBridgeServiceImpl::OnSessionStopped(StopReason stop_reason) {
   // followed by RequestStart() invocation.
   // If CONNECTING, ARC instance has not been booted properly, so do not

[Luis Héctor Chávez, 2016/12/16 23:20:12]

STARTING?

[hidehiko, 2016/12/19 08:05:11]

Done.

   // restart it automatically.
-  if (session_started_ &&
-      (state() == State::READY || state() == State::STOPPING)) {
+  if (running_ && (state() == State::RUNNING || state() == State::STOPPING)) {
     // There was a previous invocation and it crashed for some reason. Try
     // starting ARC instance later again.
     // Note that even |restart_delay_| is 0 (for testing), it needs to
@@ -175,7 +163,7 @@ void ArcBridgeServiceImpl::OnSessionStopped(StopReason stop_reason) {
     VLOG(0) << "ARC restarting";
     restart_timer_.Start(FROM_HERE, restart_delay_,
                          base::Bind(&ArcBridgeServiceImpl::StartArcSession,
-                                    weak_factory_.GetWeakPtr()));
+                                    weak_ptr_factory_.GetWeakPtr()));
   }
 
   SetStopReason(stop_reason);