Use WeakBindingSet to manage inspector connections

sky/services/inspector/server.cc

Index: sky/services/inspector/server.cc
diff --git a/sky/services/inspector/server.cc b/sky/services/inspector/server.cc
index 0963dd18b7e6bd614a4c3e2b03c281d97fb967c9..427517d2ed9fc886ab2147c2c6e03f77085fdb3b 100644
--- a/sky/services/inspector/server.cc
+++ b/sky/services/inspector/server.cc
@@ -2,12 +2,17 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <algorithm>
+
+#include "base/memory/weak_ptr.h"
 #include "mojo/application/application_runner_chromium.h"
 #include "mojo/public/c/system/main.h"
 #include "mojo/public/cpp/application/application_delegate.h"
 #include "mojo/public/cpp/application/application_impl.h"
 #include "mojo/public/cpp/application/interface_factory_impl.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/interface_impl.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
 #include "net/server/http_server.h"
 #include "net/socket/tcp_server_socket.h"
 #include "sky/services/inspector/inspector.mojom.h"
@@ -16,102 +21,45 @@
 namespace sky {
 namespace inspector {
 
-// TODO(eseidel): None of this Impl nonsense is necessary: crbug.com/431963
-class InspectorServerImpl : public mojo::InterfaceImpl<InspectorServer> {
-public:
-  class Delegate {
-  public:
-    virtual void Register(InspectorServerImpl* impl) = 0;
-    virtual void Unregister(InspectorServerImpl* impl) = 0;
-    virtual void Listen(int32_t port) = 0;
-  };
-  InspectorServerImpl(Delegate* delegate) : delegate_(delegate) {
-    delegate_->Register(this);
-  }
-  virtual ~InspectorServerImpl() {
-    delegate_->Unregister(this);
-  }
-
-  // InspectorServer:
-  void Listen(int32_t port, const mojo::Callback<void()>& callback) override {
-    delegate_->Listen(port);
-    callback.Run();
-  }
+namespace {
+const int kNotConnected = -1;
+}
 
-  void OnShutdown() {
-    delete this;
+// This provides a binding to an InspectorFrontend implementation that exposes a

[jamesr, 2014/11/12 20:47:38]

this could be made generic and live somewhere more common that can still depend
on base (maybe mojo/common).  That would make the initial patch bigger but make
it easier to reuse if this pattern is indeed common (and I think it is).  I'll
wait until the prerequisite patch lands and then see about how to approach this.

+// weak pointer to itself. The binding itself is destroyed when a connection
+// error is detected or the binding is explicitly told to close.
+class FrontendWeakBinding : public mojo::ErrorHandler {
+ public:
+  FrontendWeakBinding(InspectorFrontend* frontend,
+                      mojo::InterfaceRequest<InspectorFrontend> request)
+      : binding_(frontend, request.Pass()), weak_ptr_factory_(this) {
+    binding_.set_error_handler(this);
   }
 
-private:
-  // InterfaceImpl:
-  void OnConnectionError() override {
-    delete this; // crbug.com/431911
-  }
+  void Close() { binding_.Close(); }
 
-  Delegate* delegate_;
-};
+  InspectorFrontend::Client* client() { return binding_.client(); }
 
-class InspectorFrontendImpl : public mojo::InterfaceImpl<InspectorFrontend> {
- public:
-  class Delegate {
-  public:
-    virtual void Register(InspectorFrontendImpl*) = 0;
-    virtual void Unregister(InspectorFrontendImpl*) = 0;
-    virtual void SendMessage(const mojo::String&) = 0;
-  };
-
-  InspectorFrontendImpl(Delegate* delegate);
-  virtual ~InspectorFrontendImpl();
+  base::WeakPtr<FrontendWeakBinding> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
 
-  void OnShutdown();
+  // mojo::ErrorHandler implementation.
+  void OnConnectionError() override { delete this; }
 
  private:
-  // InspectorFrontend:
-  void SendMessage(const mojo::String& message) override;
-
-  // InterfaceImpl:
-  void OnConnectionError() override;
-
-  Delegate* delegate_;
-
-  MOJO_DISALLOW_COPY_AND_ASSIGN(InspectorFrontendImpl);
+  mojo::Binding<InspectorFrontend> binding_;
+  base::WeakPtrFactory<FrontendWeakBinding> weak_ptr_factory_;
 };
 
-InspectorFrontendImpl::InspectorFrontendImpl(Delegate* delegate)
-    : delegate_(delegate) {
-  delegate_->Register(this);
-}
-
-InspectorFrontendImpl::~InspectorFrontendImpl() {
-  delegate_->Unregister(this);
-}
-
-void InspectorFrontendImpl::OnShutdown() {
-  client()->OnDisconnect();
-  delete this;
-}
-
-void InspectorFrontendImpl::OnConnectionError() {
-  delete this; // crbug.com/431911
-}
-
-void InspectorFrontendImpl::SendMessage(const mojo::String& message) {
-  delegate_->SendMessage(message);
-}
-
-
-namespace {
-const int kNotConnected = -1;
-}
-
 class Server : public mojo::ApplicationDelegate,
-    public InspectorFrontendImpl::Delegate,
-    public InspectorServerImpl::Delegate,
-    public mojo::InterfaceFactory<InspectorFrontend>,
-    public mojo::InterfaceFactory<InspectorServer>,
-    public net::HttpServer::Delegate {
+               public InspectorFrontend,
+               public InspectorServer,
+               public mojo::InterfaceFactory<InspectorFrontend>,
+               public mojo::InterfaceFactory<InspectorServer>,
+               public net::HttpServer::Delegate {
  public:
-  Server() : connection_id_(kNotConnected) {}
+  Server() : connection_id_(kNotConnected), server_binding_(this) {}
   virtual ~Server();
 
  private:
@@ -128,25 +76,20 @@ class Server : public mojo::ApplicationDelegate,
   // InterfaceFactory<InspectorFrontend>:
   void Create(mojo::ApplicationConnection* connection,
         mojo::InterfaceRequest<InspectorFrontend> request) override {
-    // Weak instead of strong, per crbug.com/431911
-    WeakBindToRequest(new InspectorFrontendImpl(this), &request);
+    auto binding = new FrontendWeakBinding(this, request.Pass());
+    frontend_bindings_.push_back(binding->GetWeakPtr());
   }
 
   // InterfaceFactory<InspectorServer>:
   void Create(mojo::ApplicationConnection* connection,
         mojo::InterfaceRequest<InspectorServer> request) override {
-    // Weak instead of strong, per crbug.com/431911
-    WeakBindToRequest(new InspectorServerImpl(this), &request);
+    server_binding_.Bind(request.PassMessagePipe());
   }
 
-  // InspectorServerImpl::Delegate:
-  void Register(InspectorServerImpl*) override;
-  void Unregister(InspectorServerImpl*) override;
-  void Listen(int32_t port) override;
+  // InspectorServer:
+  void Listen(int32_t port, const mojo::Closure& callback) override;
 
-  // InspectorFrontendImpl::Delegate:
-  void Register(InspectorFrontendImpl*) override;
-  void Unregister(InspectorFrontendImpl*) override;
+  // InspectorFrontend:
   void SendMessage(const mojo::String& message) override;
 
   // net::HttpServer::Delegate:
@@ -160,23 +103,37 @@ class Server : public mojo::ApplicationDelegate,
   void OnClose(int connection_id) override;
 
   void CloseAllAgentConnections();
+  void ClearNullFrontendBindings();
 
   int connection_id_;
   scoped_ptr<net::HttpServer> web_server_;
-  ObserverList<InspectorFrontendImpl> agents_;
-  ObserverList<InspectorServerImpl> clients_;
+
+  std::vector<base::WeakPtr<FrontendWeakBinding>> frontend_bindings_;
+  mojo::Binding<InspectorServer> server_binding_;
 
   DISALLOW_COPY_AND_ASSIGN(Server);
 };
 
 Server::~Server()
 {
-  FOR_EACH_OBSERVER(InspectorServerImpl, clients_, OnShutdown());
   CloseAllAgentConnections();
 }
 
 void Server::CloseAllAgentConnections() {
-  FOR_EACH_OBSERVER(InspectorFrontendImpl, agents_, OnShutdown());
+  for (const auto& it : frontend_bindings_) {
+    if (it.get())
+      it->Close();
+  }
+  frontend_bindings_.clear();
+}
+
+void Server::ClearNullFrontendBindings() {
+  frontend_bindings_.erase(
+      std::remove_if(frontend_bindings_.begin(), frontend_bindings_.end(),
+                     [](const base::WeakPtr<FrontendWeakBinding>& p) {
+        return p.get() == nullptr;
+      }),
+      frontend_bindings_.end());
 }
 
 void Server::OnConnect(int connection_id) {
@@ -195,39 +152,30 @@ void Server::OnWebSocketRequest(
   }
   web_server_->AcceptWebSocket(connection_id, info);
   connection_id_ = connection_id;
-  FOR_EACH_OBSERVER(InspectorFrontendImpl, agents_, client()->OnConnect());
+  ClearNullFrontendBindings();
+  for (const auto& it : frontend_bindings_) {
+    it->client()->OnConnect();
+  }
 }
 
 void Server::OnWebSocketMessage(
     int connection_id, const std::string& data) {
   DCHECK_EQ(connection_id, connection_id_);
-  FOR_EACH_OBSERVER(InspectorFrontendImpl, agents_, client()->OnMessage(data));
+  ClearNullFrontendBindings();
+  for (const auto& it : frontend_bindings_)
+    it->client()->OnMessage(data);
 }
 
 void Server::OnClose(int connection_id) {
   if (connection_id != connection_id_)
     return;
   connection_id_ = kNotConnected;
-  FOR_EACH_OBSERVER(InspectorFrontendImpl, agents_, client()->OnDisconnect());
-}
-
-void Server::Register(InspectorServerImpl* client) {
-  clients_.AddObserver(client);
-}
-
-void Server::Unregister(InspectorServerImpl* client) {
-  clients_.RemoveObserver(client);
-}
-
-void Server::Register(InspectorFrontendImpl* agent) {
-  agents_.AddObserver(agent);
-}
-
-void Server::Unregister(InspectorFrontendImpl* agent) {
-  agents_.RemoveObserver(agent);
+  ClearNullFrontendBindings();
+  for (const auto& it : frontend_bindings_)
+    it->client()->OnDisconnect();
 }
 
-void Server::Listen(int32_t port) {
+void Server::Listen(int32_t port, const mojo::Closure& callback) {
   CloseAllAgentConnections(); // Assume caller represents a new app.
 
   // TODO(eseidel): Early-out here if we're already bound to the right port.
@@ -236,6 +184,7 @@ void Server::Listen(int32_t port) {
       new net::TCPServerSocket(NULL, net::NetLog::Source()));
   server_socket->ListenWithAddressAndPort("0.0.0.0", port, 1);
   web_server_.reset(new net::HttpServer(server_socket.Pass(), this));
+  callback.Run();
 }
 
 void Server::SendMessage(const mojo::String& message) {