Switch from Delegate to Observer.

This CL switches the interface between ArcBridgeBootstrap
and ArcBridgeServiceImpl from Delegate to Observer.
Also, ArcBridgeHost will be instantiated in ArcBridgeBootstrap
and kept in it.

BUG=633258
TEST=Trybots.

Review-Url: https://codereview.chromium.org/2379223004
Cr-Commit-Position: refs/heads/master@{#423051}

components/arc/BUILD.gn

@@ -165,8 +165,6 @@ static_library("arc_test_support") {
     "test/fake_app_instance.h",
     "test/fake_arc_bridge_bootstrap.cc",
     "test/fake_arc_bridge_bootstrap.h",
-    "test/fake_arc_bridge_instance.cc",
-    "test/fake_arc_bridge_instance.h",
     "test/fake_arc_bridge_service.cc",
     "test/fake_arc_bridge_service.h",
     "test/fake_bluetooth_instance.cc",

components/arc/arc_bridge_bootstrap.cc

@@ -26,6 +26,7 @@
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
+#include "components/arc/arc_bridge_host_impl.h"
 #include "components/arc/arc_features.h"
 #include "components/user_manager/user_manager.h"
 #include "ipc/unix_domain_socket_util.h"
@@ -126,7 +127,7 @@ class ArcBridgeBootstrapImpl : public ArcBridgeBootstrap,
   //
   // At any state, Stop() can be called. It does not immediately stop the
   // instance, but will eventually stop it.
-  // The actual stop will be notified via OnStopped() of the |delegate_|.
+  // The actual stop will be notified via Observer::OnStopped().
   //
   // When Stop() is called, it makes various behavior based on the current
   // phase.
@@ -248,6 +249,9 @@ class ArcBridgeBootstrapImpl : public ArcBridgeBootstrap,
   // to notify cancelling of the procedure.
   base::ScopedFD accept_cancel_pipe_;
 
+  // Mojo endpoint.
+  std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host_;
+
   base::ThreadChecker thread_checker_;
 
   // WeakPtrFactory to use callbacks.
@@ -277,7 +281,6 @@ ArcBridgeBootstrapImpl::~ArcBridgeBootstrapImpl() {
 
 void ArcBridgeBootstrapImpl::Start() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(delegate_);
   DCHECK_EQ(state_, State::NOT_STARTED);
   VLOG(2) << "Starting ARC session.";
   VLOG(2) << "Checking disk space...";
@@ -514,17 +517,15 @@ void ArcBridgeBootstrapImpl::OnMojoConnected(base::ScopedFD fd) {
   mojom::ArcBridgeInstancePtr instance;
   instance.Bind(mojo::InterfacePtrInfo<mojom::ArcBridgeInstance>(
       std::move(server_pipe), 0u));
-  // TODO(hidehiko): Move to creating ArcBridgeHost here to fix the twisted
-  // state change.
+  arc_bridge_host_.reset(new ArcBridgeHostImpl(std::move(instance)));
 
   VLOG(2) << "Mojo is connected. ARC is running.";
   state_ = State::RUNNING;
-  delegate_->OnConnectionEstablished(std::move(instance));
+  FOR_EACH_OBSERVER(ArcBridgeBootstrap::Observer, observer_list_, OnReady());
 }
 
 void ArcBridgeBootstrapImpl::Stop() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(delegate_);
   VLOG(2) << "Stopping ARC session is requested.";
 
   // For second time or later, just do nothing.
@@ -533,6 +534,7 @@ void ArcBridgeBootstrapImpl::Stop() {
     return;
 
   stop_requested_ = true;
+  arc_bridge_host_.reset();
   switch (state_) {
     case State::NOT_STARTED:
       OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
@@ -617,12 +619,25 @@ void ArcBridgeBootstrapImpl::OnStopped(ArcBridgeService::StopReason reason) {
   // OnStopped() should be called once per instance.
   DCHECK_NE(state_, State::STOPPED);
   VLOG(2) << "ARC session is stopped.";
+  arc_bridge_host_.reset();
   state_ = State::STOPPED;
-  delegate_->OnStopped(reason);
+  FOR_EACH_OBSERVER(ArcBridgeBootstrap::Observer, observer_list_,
+                    OnStopped(reason));
 }
 
 }  // namespace
 
+ArcBridgeBootstrap::ArcBridgeBootstrap() = default;
+ArcBridgeBootstrap::~ArcBridgeBootstrap() = default;
+
+void ArcBridgeBootstrap::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void ArcBridgeBootstrap::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
 // static
 std::unique_ptr<ArcBridgeBootstrap> ArcBridgeBootstrap::Create() {
   return base::MakeUnique<ArcBridgeBootstrapImpl>();

components/arc/arc_bridge_bootstrap.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "components/arc/arc_bridge_service.h"
@@ -26,27 +27,25 @@ namespace arc {
 // Rename this to ArcSession.
 class ArcBridgeBootstrap {
  public:
-  // TODO(hidehiko): Switch to Observer style, which fits more for this design.
-  class Delegate {
+  class Observer {
    public:
+    Observer() = default;
+    virtual ~Observer() = default;
+
     // Called when the connection with ARC instance has been established.
-    // TODO(hidehiko): Moving ArcBridgeHost to the ArcBridgeBootstrapImpl
-    // so that this can be replaced by OnReady() simply.
-    virtual void OnConnectionEstablished(
-        mojom::ArcBridgeInstancePtr instance_ptr) = 0;
+    virtual void OnReady() = 0;
 
     // Called when ARC instance is stopped. This is called exactly once
     // per instance which is Start()ed.
     virtual void OnStopped(ArcBridgeService::StopReason reason) = 0;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Observer);
   };
 
   // Creates a default instance of ArcBridgeBootstrap.
   static std::unique_ptr<ArcBridgeBootstrap> Create();
-  virtual ~ArcBridgeBootstrap() = default;
-
-  // This must be called before calling Start() or Stop(). |delegate| is owned
-  // by the caller and must outlive this instance.
-  void set_delegate(Delegate* delegate) { delegate_ = delegate; }
+  virtual ~ArcBridgeBootstrap();
 
   // Starts and bootstraps a connection with the instance. The Delegate's
   // OnConnectionEstablished() will be called if the bootstrapping is
@@ -58,11 +57,13 @@ class ArcBridgeBootstrap {
   // The completion is notified via OnStopped() of the Delegate.
   virtual void Stop() = 0;
 
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
  protected:
-  ArcBridgeBootstrap() = default;
+  ArcBridgeBootstrap();
 
-  // Owned by the caller.
-  Delegate* delegate_ = nullptr;
+  base::ObserverList<Observer> observer_list_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ArcBridgeBootstrap);

components/arc/arc_bridge_service_impl.cc

@@ -17,7 +17,6 @@
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
-#include "components/arc/arc_bridge_host_impl.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 
@@ -38,6 +37,9 @@ ArcBridgeServiceImpl::ArcBridgeServiceImpl()
 }
 
 ArcBridgeServiceImpl::~ArcBridgeServiceImpl() {
+  if (bootstrap_)
+    bootstrap_->RemoveObserver(this);
+
   DCHECK(g_arc_bridge_service == this);
   g_arc_bridge_service = nullptr;
 }
@@ -82,9 +84,12 @@ void ArcBridgeServiceImpl::PrerequisitesChanged() {
     VLOG(0) << "Prerequisites met, starting ARC";
     SetStopReason(StopReason::SHUTDOWN);
 
+    if (bootstrap_)
+      bootstrap_->RemoveObserver(this);
+
     SetState(State::CONNECTING);
     bootstrap_ = factory_.Run();
-    bootstrap_->set_delegate(this);
+    bootstrap_->AddObserver(this);
     bootstrap_->Start();
   } else {
     if (session_started_)
@@ -107,22 +112,18 @@ void ArcBridgeServiceImpl::StopInstance() {
   VLOG(1) << "Stopping ARC";
   DCHECK(bootstrap_.get());
   SetState(State::STOPPING);
-  arc_bridge_host_.reset();
 
   // Note: this can call OnStopped() internally as a callback.
   bootstrap_->Stop();
 }
 
-void ArcBridgeServiceImpl::OnConnectionEstablished(
-    mojom::ArcBridgeInstancePtr instance) {
+void ArcBridgeServiceImpl::OnReady() {
   DCHECK(CalledOnValidThread());
   if (state() != State::CONNECTING) {
     VLOG(1) << "StopInstance() called while connecting";
     return;
   }
 
-  arc_bridge_host_.reset(new ArcBridgeHostImpl(std::move(instance)));
-
   // The container can be considered to have been successfully launched, so
   // restart if the connection goes down without being requested.
   reconnect_ = true;
@@ -133,7 +134,7 @@ void ArcBridgeServiceImpl::OnConnectionEstablished(
 void ArcBridgeServiceImpl::OnStopped(StopReason stop_reason) {
   DCHECK(CalledOnValidThread());
   VLOG(0) << "ARC stopped: " << stop_reason;
-  arc_bridge_host_.reset();
+  bootstrap_->RemoveObserver(this);
   bootstrap_.reset();
   SetStopReason(stop_reason);
   SetState(State::STOPPED);

components/arc/arc_bridge_service_impl.h

@@ -25,7 +25,7 @@ namespace arc {
 
 // Real IPC based ArcBridgeService that is used in production.
 class ArcBridgeServiceImpl : public ArcBridgeService,
-                             public ArcBridgeBootstrap::Delegate {
+                             public ArcBridgeBootstrap::Observer {
  public:
   // This is the factory interface to inject ArcBridgeBootstrap instance
   // for testing purpose.
@@ -65,8 +65,8 @@ class ArcBridgeServiceImpl : public ArcBridgeService,
   // Stops the running instance.
   void StopInstance();
 
-  // ArcBridgeBootstrap::Delegate:
-  void OnConnectionEstablished(mojom::ArcBridgeInstancePtr instance) override;
+  // ArcBridgeBootstrap::Observer:
+  void OnReady() override;
   void OnStopped(StopReason reason) override;
 
   std::unique_ptr<ArcBridgeBootstrap> bootstrap_;
@@ -74,10 +74,6 @@ class ArcBridgeServiceImpl : public ArcBridgeService,
   // If the user's session has started.
   bool session_started_;
 
-  // Mojo endpoint.
-  // TODO(hidehiko): Move this to ArcBridgeBootstrap.
-  std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host_;
-
   // If the instance had already been started but the connection to it was
   // lost. This should make the instance restart.
   bool reconnect_ = false;

components/arc/arc_bridge_service_unittest.cc

@@ -13,7 +13,6 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/arc/arc_bridge_service_impl.h"
 #include "components/arc/test/fake_arc_bridge_bootstrap.h"
-#include "components/arc/test/fake_arc_bridge_instance.h"
 #include "mojo/public/cpp/system/message_pipe.h"
 #include "testing/gtest/include/gtest/gtest.h"
 

components/arc/test/fake_arc_bridge_bootstrap.cc

@@ -5,7 +5,6 @@
 #include "components/arc/test/fake_arc_bridge_bootstrap.h"
 
 #include <memory>
-#include <utility>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -17,13 +16,11 @@ FakeArcBridgeBootstrap::FakeArcBridgeBootstrap() = default;
 FakeArcBridgeBootstrap::~FakeArcBridgeBootstrap() = default;
 
 void FakeArcBridgeBootstrap::Start() {
-  DCHECK(delegate_);
   if (boot_failure_emulation_enabled_) {
-    delegate_->OnStopped(boot_failure_reason_);
+    FOR_EACH_OBSERVER(Observer, observer_list_,
+                      OnStopped(boot_failure_reason_));
   } else if (!boot_suspended_) {
-    mojom::ArcBridgeInstancePtr instance_ptr;
-    instance_.Bind(mojo::GetProxy(&instance_ptr));
-    delegate_->OnConnectionEstablished(std::move(instance_ptr));
+    FOR_EACH_OBSERVER(Observer, observer_list_, OnReady());
   }
 }
 
@@ -33,9 +30,7 @@ void FakeArcBridgeBootstrap::Stop() {
 
 void FakeArcBridgeBootstrap::StopWithReason(
     ArcBridgeService::StopReason reason) {
-  DCHECK(delegate_);
-  instance_.Unbind();
-  delegate_->OnStopped(reason);
+  FOR_EACH_OBSERVER(Observer, observer_list_, OnStopped(reason));
 }
 
 void FakeArcBridgeBootstrap::EnableBootFailureEmulation(

components/arc/test/fake_arc_bridge_bootstrap.h

@@ -9,7 +9,6 @@
 
 #include "base/macros.h"
 #include "components/arc/arc_bridge_bootstrap.h"
-#include "components/arc/test/fake_arc_bridge_instance.h"
 
 namespace arc {
 
@@ -42,7 +41,6 @@ class FakeArcBridgeBootstrap : public ArcBridgeBootstrap {
  private:
   bool boot_failure_emulation_enabled_ = false;
   ArcBridgeService::StopReason boot_failure_reason_;
-  FakeArcBridgeInstance instance_;
 
   bool boot_suspended_ = false;
 

components/arc/test/fake_arc_bridge_instance.cc

-// Copyright 2015 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.
-
-#include "components/arc/test/fake_arc_bridge_instance.h"
-
-#include <utility>
-
-#include "base/run_loop.h"
-
-namespace arc {
-
-FakeArcBridgeInstance::FakeArcBridgeInstance() : binding_(this) {}
-FakeArcBridgeInstance::~FakeArcBridgeInstance() {}
-
-void FakeArcBridgeInstance::Init(mojom::ArcBridgeHostPtr host) {
-  host_ptr_ = std::move(host);
-  init_calls_++;
-
-  // Wake WaitForInitCall().
-  if (!quit_closure_.is_null())
-    quit_closure_.Run();
-  quit_closure_.Reset();
-}
-
-void FakeArcBridgeInstance::Unbind() {
-  host_ptr_.reset();
-  if (binding_.is_bound())
-    binding_.Close();
-}
-
-void FakeArcBridgeInstance::Bind(
-    mojo::InterfaceRequest<mojom::ArcBridgeInstance> interface_request) {
-  binding_.Bind(std::move(interface_request));
-}
-
-void FakeArcBridgeInstance::WaitForInitCall() {
-  base::RunLoop run_loop;
-  quit_closure_ = run_loop.QuitClosure();
-  binding_.set_connection_error_handler(run_loop.QuitClosure());
-  run_loop.Run();
-}
-
-void FakeArcBridgeInstance::Stop(ArcBridgeService::StopReason reason) {
-  if (!delegate_)
-    return;
-  delegate_->OnStopped(reason);
-}
-
-}  // namespace arc

components/arc/test/fake_arc_bridge_instance.h

-// Copyright 2015 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 COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_INSTANCE_H_
-#define COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_INSTANCE_H_
-
-#include "base/callback_forward.h"
-#include "base/macros.h"
-#include "components/arc/arc_bridge_service.h"
-#include "components/arc/common/arc_bridge.mojom.h"
-#include "mojo/public/cpp/bindings/binding.h"
-
-namespace arc {
-
-class FakeArcBridgeInstance : public mojom::ArcBridgeInstance {
- public:
-  class Delegate {
-   public:
-    virtual ~Delegate() = default;
-    virtual void OnStopped(ArcBridgeService::StopReason reason) = 0;
-  };
-
-  FakeArcBridgeInstance();
-  ~FakeArcBridgeInstance() override;
-
-  void set_delegate(Delegate* delegate) { delegate_ = delegate; }
-
-  // Finalizes the connection between the host and the instance, and signals
-  // the host that the boot sequence has finished.
-  void Bind(mojo::InterfaceRequest<mojom::ArcBridgeInstance> interface_request);
-
-  // Resets the binding. Useful to simulate a restart.
-  void Unbind();
-
-  // ArcBridgeInstance:
-  void Init(mojom::ArcBridgeHostPtr host) override;
-
-  // Ensures the call to Init() has been dispatched.
-  void WaitForInitCall();
-
-  // The number of times Init() has been called.
-  int init_calls() const { return init_calls_; }
-
-  // Stops the instance.
-  void Stop(ArcBridgeService::StopReason reason);
-
- private:
-  Delegate* delegate_ = nullptr;
-
-  // Keeps quit closure to wake the running nested RunLoop.
-  base::Closure quit_closure_;
-
-  // Mojo endpoints.
-  mojo::Binding<mojom::ArcBridgeInstance> binding_;
-  mojom::ArcBridgeHostPtr host_ptr_;
-  int init_calls_ = 0;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeArcBridgeInstance);
-};
-
-}  // namespace arc
-
-#endif  // COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_INSTANCE_H_

components/arc/test/fake_intent_helper_instance.h

@@ -10,7 +10,6 @@
 
 #include "base/callback.h"
 #include "components/arc/common/intent_helper.mojom.h"
-#include "components/arc/test/fake_arc_bridge_instance.h"
 #include "mojo/public/cpp/bindings/binding.h"
 
 namespace arc {

components/arc/test/fake_notifications_instance.h

@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "components/arc/common/notifications.mojom.h"
-#include "components/arc/test/fake_arc_bridge_instance.h"
 
 namespace arc {
 

components/arc/test/fake_policy_instance.h

@@ -8,7 +8,6 @@
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/common/policy.mojom.h"
 #include "components/arc/instance_holder.h"
-#include "components/arc/test/fake_arc_bridge_instance.h"
 
 namespace arc {
 

ui/arc/notification/arc_notification_manager_unittest.cc

@@ -10,7 +10,6 @@
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "components/arc/instance_holder.h"
-#include "components/arc/test/fake_arc_bridge_instance.h"
 #include "components/arc/test/fake_arc_bridge_service.h"
 #include "components/arc/test/fake_notifications_instance.h"
 #include "testing/gtest/include/gtest/gtest.h"