Add mojo::Binding<Interface> for more flexible pipe<->impl binding

mojo/public/cpp/bindings/interface_impl.h

Index: mojo/public/cpp/bindings/interface_impl.h
diff --git a/mojo/public/cpp/bindings/interface_impl.h b/mojo/public/cpp/bindings/interface_impl.h
index da1055de535bb71445541320ab274be536bd5fe1..da37328a7744a3eb029a471b9c474fc2c03af90b 100644
--- a/mojo/public/cpp/bindings/interface_impl.h
+++ b/mojo/public/cpp/bindings/interface_impl.h
@@ -5,8 +5,8 @@
 #ifndef MOJO_PUBLIC_CPP_BINDINGS_INTERFACE_IMPL_H_
 #define MOJO_PUBLIC_CPP_BINDINGS_INTERFACE_IMPL_H_
 
+#include "mojo/public/cpp/bindings/connector.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
-#include "mojo/public/cpp/bindings/lib/interface_impl_internal.h"
 #include "mojo/public/cpp/environment/environment.h"
 #include "mojo/public/cpp/system/macros.h"
 
@@ -16,29 +16,34 @@ namespace mojo {
 // implementation. It may be bound to a pipe or a proxy, see BindToPipe and
 // BindToProxy.
 template <typename Interface>
-class InterfaceImpl : public internal::InterfaceImplBase<Interface> {
+class InterfaceImpl : public Interface, public ErrorHandler {
  public:
-  typedef typename Interface::Client Client;
-  typedef Interface ImplementedInterface;
+  using ImplementedInterface = Interface;
+  using Client = typename Interface::Client;
 
-  InterfaceImpl() : internal_state_(this) {}
+  InterfaceImpl() : connector_(this), error_handler_impl_(this) {

[DaveMoore, 2014/11/11 17:18:45]

I'm finding it surprising that to change the connector you no longer use
InterfaceImpl<>. I'm not sure if that's because it's new or because I would
rather have some mechanism for controlling which Connector class an
InterfaceImpl uses. Optional second template arg perhaps?

Essentially we're telling people to create their implementations one way, unless
they want something different and then to do it a different way.

Another option would be to do away with InterfaceImpl<> entirely and make
Connector<> have everything we need. But then does it just become
InterfaceImpl<> under another name?

[jamesr, 2014/11/11 17:47:49]

That is the goal.  You don't inherit from InterfaceImpl<> which is the key
improvement

+    connector_.set_error_handler(&error_handler_impl_);
+  }
   virtual ~InterfaceImpl() {}
 
-  // Returns a proxy to the client interface. This is null upon construction,
-  // and becomes non-null after OnClientConnected. NOTE: It remains non-null
-  // until this instance is deleted.
-  Client* client() { return internal_state_.client(); }
+  void BindConnector(

[DaveMoore, 2014/11/11 17:18:45]

Maybe drop the "Connector" from these names, as the connector is not passed.

[jamesr, 2014/11/11 17:47:49]

Yeah (I needed a different name in an intermediate state but don't any more). 
Just Bind() is better

[jamesr, 2014/11/11 17:49:20]

Actually never mind - this name has to be this way since things subclass from
InterfaceImpl<> and define their own function called Bind (see
mojo/services/network/udp_socket_impl.h for one example).  Fix is to get rid of
the subclassing here

+      ScopedMessagePipeHandle handle,
+      const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) {
+    connector_.Bind(handle.Pass(), waiter);
+  }
+
+  void BindConnector(
+      InterfacePtr<Interface>* ptr,
+      const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) {
+    connector_.Bind(ptr, waiter);
+  }
 
-  // Blocks the current thread for the first incoming method call, i.e., either
-  // a call to a method or a client callback method. Returns |true| if a method
-  // has been called, |false| in case of error. It must only be called on a
-  // bound object.
   bool WaitForIncomingMethodCall() {
-    return internal_state_.WaitForIncomingMethodCall();
+    return connector_.WaitForIncomingMethodCall();
   }
 
-  // Called when the client has connected to this instance.
-  virtual void OnConnectionEstablished() {}

[DaveMoore, 2014/11/11 17:18:45]

This seemed like useful functionality. What replaces it?

[jamesr, 2014/11/11 17:47:49]

I disagree - it's not useful at all.  The connection is "established" whenever
Bind() is called, which is something the -caller- knows.

Having this be a virtual is not useful either since you don't inherit from this
class

+  Client* client() { return connector_.client(); }

[DaveMoore, 2014/11/11 17:18:45]

There should at least be an extra comment about router() only being for testing.
Or we should make a bigger barrier. The name should indicate internal as well as
the type.

+  internal::Router* router() { return connector_.router(); }
 
   // Called when the client is no longer connected to this instance. NOTE: The
   // client() method continues to return a non-null pointer after this method
@@ -46,13 +51,35 @@ class InterfaceImpl : public internal::InterfaceImplBase<Interface> {
   // will be silently ignored.
   virtual void OnConnectionError() {}
 
-  // DO NOT USE. Exposed only for internal use and for testing.
-  internal::InterfaceImplState<Interface>* internal_state() {
-    return &internal_state_;
+  void set_delete_on_error(bool delete_on_error) {
+    error_handler_impl_.set_delete_on_error(delete_on_error);
   }
 
  private:
-  internal::InterfaceImplState<Interface> internal_state_;
+  class ErrorHandlerImpl : public ErrorHandler {
+   public:
+    explicit ErrorHandlerImpl(InterfaceImpl* impl) : impl_(impl) {}
+    ~ErrorHandlerImpl() override {}
+
+    // ErrorHandler implementation:
+    void OnConnectionError() override {
+      impl_->OnConnectionError();
+      if (delete_on_error_)
+        delete impl_;
+    }
+
+    void set_delete_on_error(bool delete_on_error) {
+      delete_on_error_ = delete_on_error;
+    }
+
+   private:
+    InterfaceImpl* impl_;
+    bool delete_on_error_ = false;
+  };
+
+  Connector<Interface> connector_;
+  ErrorHandlerImpl error_handler_impl_;
+
   MOJO_DISALLOW_COPY_AND_ASSIGN(InterfaceImpl);
 };
 
@@ -67,14 +94,13 @@ class InterfaceImpl : public internal::InterfaceImplBase<Interface> {
 // called on the current thread, and if the current thread exits, then the end
 // point of the pipe will be closed and the error handler's OnConnectionError
 // method will be called.
-//
-// Before returning, the instance's OnConnectionEstablished method is called.
 template <typename Impl>
 Impl* BindToPipe(
     Impl* instance,
     ScopedMessagePipeHandle handle,
     const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) {
-  instance->internal_state()->Bind(handle.Pass(), true, waiter);
+  instance->set_delete_on_error(true);
+  instance->BindConnector(handle.Pass(), waiter);
   return instance;
 }
 
@@ -84,7 +110,7 @@ Impl* WeakBindToPipe(
     Impl* instance,
     ScopedMessagePipeHandle handle,
     const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) {
-  instance->internal_state()->Bind(handle.Pass(), false, waiter);
+  instance->BindConnector(handle.Pass(), waiter);
   return instance;
 }
 
@@ -97,14 +123,13 @@ Impl* WeakBindToPipe(
 // The instance is also bound to the current thread. Its methods will only be
 // called on the current thread, and if the current thread exits, then it will
 // also be deleted, and along with it, its end point of the pipe will be closed.
-//
-// Before returning, the instance's OnConnectionEstablished method is called.
 template <typename Impl, typename Interface>
 Impl* BindToProxy(
     Impl* instance,
     InterfacePtr<Interface>* ptr,
     const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) {
-  instance->internal_state()->BindProxy(ptr, true, waiter);
+  instance->set_delete_on_error(true);
+  instance->BindConnector(ptr, waiter);
   return instance;
 }
 
@@ -114,7 +139,7 @@ Impl* WeakBindToProxy(
     Impl* instance,
     InterfacePtr<Interface>* ptr,
     const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) {
-  instance->internal_state()->BindProxy(ptr, false, waiter);
+  instance->BindConnector(ptr, waiter);
   return instance;
 }