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

mojo/public/cpp/bindings/interface_impl.h

 // Copyright 2014 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 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"
 
 namespace mojo {
 
 // InterfaceImpl<..> is designed to be the base class of an interface
 // 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(); }
-
-  // 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();
+  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);
   }
 
-  // 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

+  void BindConnector(
+      InterfacePtr<Interface>* ptr,
+      const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) {
+    connector_.Bind(ptr, waiter);
+  }
+
+  bool WaitForIncomingMethodCall() {
+    return connector_.WaitForIncomingMethodCall();
+  }
+
+  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
   // is called. After this method is called, any method calls made on client()
   // 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);
 };
 
 // Takes an instance of an InterfaceImpl<..> subclass and binds it to the given
 // MessagePipe. The instance is returned for convenience in member initializer
 // lists, etc.
 //
 // If the pipe is closed, the instance's OnConnectionError method will be called
 // and then the instance will be deleted.
 //
 // 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 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;
 }
 
 // Like BindToPipe but does not delete the instance after a channel error.
 template <typename Impl>
 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;
 }
 
 // Takes an instance of an InterfaceImpl<..> subclass and binds it to the given
 // InterfacePtr<..>. The instance is returned for convenience in member
 // initializer lists, etc. If the pipe is closed, the instance's
 // OnConnectionError method will be called and then the instance will be
 // deleted.
 //
 // 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;
 }
 
 // Like BindToProxy but does not delete the instance after a channel error.
 template <typename Impl, typename Interface>
 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;
 }
 
 // Takes an instance of an InterfaceImpl<..> subclass and binds it to the given
 // InterfaceRequest<..>. The instance is returned for convenience in member
 // initializer lists, etc. If the pipe is closed, the instance's
 // OnConnectionError method will be called and then the instance will be
 // deleted.
 //
 // The instance is also bound to the current thread. Its methods will only be
@@ skipping 15 matching lines @@
 Impl* WeakBindToRequest(
     Impl* instance,
     InterfaceRequest<Interface>* request,
     const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) {
   return WeakBindToPipe(instance, request->PassMessagePipe(), waiter);
 }
 
 }  // namespace mojo
 
 #endif  // MOJO_PUBLIC_CPP_BINDINGS_INTERFACE_IMPL_H_