Mojo: Replace RemotePtr with InterfacePtr and InterfaceImpl

mojo/service_manager/service_manager.cc

Index: mojo/service_manager/service_manager.cc
diff --git a/mojo/service_manager/service_manager.cc b/mojo/service_manager/service_manager.cc
index b62c6e14b012f187c98247ac5a471c94a46fa4df..c1131823835fcc127adcc344908838b3ea8d5b21 100644
--- a/mojo/service_manager/service_manager.cc
+++ b/mojo/service_manager/service_manager.cc
@@ -11,8 +11,6 @@
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "mojo/public/cpp/bindings/allocation_scope.h"
-#include "mojo/public/cpp/bindings/error_handler.h"
-#include "mojo/public/cpp/bindings/remote_ptr.h"
 #include "mojo/service_manager/service_loader.h"
 
 namespace mojo {
@@ -22,74 +20,74 @@ namespace {
 bool has_created_instance = false;
 }
 
-class ServiceManager::ServiceFactory : public Shell, public ErrorHandler {
+class ServiceManager::ServiceFactory : public Shell {
  public:
   ServiceFactory(ServiceManager* manager, const GURL& url)
       : manager_(manager),
-        url_(url) {
-    InterfacePipe<Shell> pipe;
-    shell_client_.reset(pipe.handle_to_peer.Pass(), this, this);
-    manager_->GetLoaderForURL(url)->LoadService(manager_,
-                                                url,
-                                                pipe.handle_to_self.Pass());
+        url_(url),
+        client_(NULL) {
   }
 
-  virtual ~ServiceFactory() {}
+  virtual ~ServiceFactory() {
+    if (manager_)
+      manager_->RemoveServiceFactory(this);
+  }
+
+  void Orphan() {

[DaveMoore, 2014/05/06 21:30:54]

It can be hard to track down bugs when you can either delete the ServiceFactory
directly or through the Manager. I used to have something just like this and it
got very confusing. What now requires it?

[darin (slow to review), 2014/05/06 21:39:11]

The lifetime of the ServiceFactory is now bound to the lifetime of the
MessagePipe used by the client to talk to its Shell instance.

+    manager_ = NULL;
+  }
 
   void ConnectToClient(ScopedMessagePipeHandle handle) {
-    if (handle.is_valid()) {
-      AllocationScope scope;
-      shell_client_->AcceptConnection(url_.spec(), handle.Pass());
+    if (!handle.is_valid()) {
+      assert(false);
+      return;
     }
+    AllocationScope scope;
+    client_->AcceptConnection(url_.spec(), handle.Pass());
   }
 
-  virtual void Connect(const String& url,
-                       ScopedMessagePipeHandle client_pipe) OVERRIDE {
-    manager_->Connect(GURL(url.To<std::string>()), client_pipe.Pass());
+  // Shell implementation:
+
+  virtual void SetClient(ShellClient* client) OVERRIDE {
+    client_ = client;
   }
 
-  virtual void OnError() OVERRIDE {
-    manager_->OnServiceFactoryError(this);
+  virtual void Connect(const String& url,
+                       ScopedMessagePipeHandle client_pipe) OVERRIDE {
+    if (manager_)
+      manager_->Connect(GURL(url.To<std::string>()), client_pipe.Pass());
   }
 
   const GURL& url() const { return url_; }
 
  private:
-  ServiceManager* const manager_;
+  ServiceManager* manager_;
   const GURL url_;
-  RemotePtr<ShellClient> shell_client_;
+  ShellClient* client_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceFactory);
 };
 
-class ServiceManager::TestAPI::TestShellConnection
-    : public Shell,
-      public ErrorHandler {
+class ServiceManager::TestAPI::TestShellConnection : public Shell {
  public:
-  explicit TestShellConnection(ServiceManager* manager) : manager_(manager) {
-    InterfacePipe<Shell> pipe;
-    shell_client_.reset(pipe.handle_to_peer.Pass(), this, this);
-    shell_handle_ = pipe.handle_to_self.Pass();
+  explicit TestShellConnection(ServiceManager* manager)
+      : manager_(manager),
+        client_(NULL) {
   }
   virtual ~TestShellConnection() {}
 
-  ScopedShellHandle GetShellHandle() {
-    return shell_handle_.Pass();
-  }
-
   // Shell:
+  virtual void SetClient(ShellClient* client) OVERRIDE {
+    client_ = client;
+  }
   virtual void Connect(const String& url,
                        ScopedMessagePipeHandle client_pipe) OVERRIDE {
     manager_->Connect(GURL(url.To<std::string>()), client_pipe.Pass());
   }
 
-  virtual void OnError() OVERRIDE {
-  }
-
  private:
   ServiceManager* manager_;
-  RemotePtr<ShellClient> shell_client_;
-  ScopedShellHandle shell_handle_;
+  ShellClient* client_;
 
   DISALLOW_COPY_AND_ASSIGN(TestShellConnection);
 };
@@ -105,15 +103,16 @@ bool ServiceManager::TestAPI::HasCreatedInstance() {
   return has_created_instance;
 }
 
-ScopedShellHandle ServiceManager::TestAPI::GetShellHandle() {
-  if (!shell_connection_.get())
-    shell_connection_.reset(new TestShellConnection(manager_));
-  return shell_connection_->GetShellHandle().Pass();
+ScopedMessagePipeHandle ServiceManager::TestAPI::GetShellHandle() {
+  MessagePipe pipe;
+  shell_.Bind(new TestShellConnection(manager_));
+  shell_.ConfigureStub(pipe.handle0.Pass());
+  return pipe.handle1.Pass();
 }
 
 bool ServiceManager::TestAPI::HasFactoryForURL(const GURL& url) const {
   return manager_->url_to_service_factory_.find(url) !=
-      manager_->url_to_service_factory_.end();
+         manager_->url_to_service_factory_.end();
 }
 
 ServiceManager::ServiceManager()
@@ -121,7 +120,12 @@ ServiceManager::ServiceManager()
 }
 
 ServiceManager::~ServiceManager() {
-  STLDeleteValues(&url_to_service_factory_);
+  // Make sure any lingering service factories no longer reference us.
+  for (URLToServiceFactoryMap::iterator it = url_to_service_factory_.begin();
+       it != url_to_service_factory_.end(); ++it) {
+    it->second->Orphan();
+  }
+
   STLDeleteValues(&url_to_loader_);
   STLDeleteValues(&scheme_to_loader_);
 }
@@ -143,6 +147,14 @@ void ServiceManager::Connect(const GURL& url,
     service_factory = service_it->second;
   } else {
     service_factory = new ServiceFactory(this, url);
+
+    GetLoaderForURL(url)->LoadService(
+        this, url, BindToPipe<Shell>(service_factory).Pass());
+
+    // NOTE: The lifetime of the service_factory instance is now bound to the
+    // MessagePipe passed to LoadService. We retain a raw pointer to
+    // service_factory so that we can continue communicating with it.
+
     url_to_service_factory_[url] = service_factory;
   }
   if (interceptor_) {
@@ -185,11 +197,10 @@ ServiceLoader* ServiceManager::GetLoaderForURL(const GURL& url) {
   return default_loader_.get();
 }
 
-void ServiceManager::OnServiceFactoryError(ServiceFactory* service_factory) {
+void ServiceManager::RemoveServiceFactory(ServiceFactory* service_factory) {

[DaveMoore, 2014/05/06 21:30:54]

This is called OnServiceFactoryError() because it sends OnServiceError() to the
Loader. Either that name should be changed as well, or this should remain.

[darin (slow to review), 2014/05/06 21:39:11]

I just had service.h on the mind. Renamed back to OnServiceFactoryError.

   const GURL url = service_factory->url();
   URLToServiceFactoryMap::iterator it = url_to_service_factory_.find(url);
   DCHECK(it != url_to_service_factory_.end());
-  delete it->second;
   url_to_service_factory_.erase(it);
   GetLoaderForURL(url)->OnServiceError(this, url);
 }