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

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

This provides a mojo::Binding<Interface> class that takes a pointer to an
interface implementation and binds it to a pipe. This type can be composed into
the interface implementation or managed separately, depending on the needs of
the implementation. This is in contrast to InterfaceImpl<Interface> which uses
implementation inheritance to store the binding with the pipe and thus forces
the implementation into having a rigid relationship with the pipe.

Binding<Interface> maps messages on the pipe to the interface and allows
registering an error handler to be notified of connection errors but does not
implement an error handling strategy itself. Deleting a Binding closes the pipe.

StrongBinding<Interface> additionally deletes the interface on receiving a pipe
error.

InterfaceImpl<Interface> is rewritten in this patch to use a Binding<Interface>
internally. I intend to transition all code away from using
InterfaceImpl<Interface> to using Binding<Interface> directly, or if it's useful
adding other helper classes for common patterns. I have a few examples of how
this transition looks here: https://codereview.chromium.org/713313002 and here:
https://codereview.chromium.org/718623004.

BUG=431963
R=davemoore@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/43fd6836c314a82308f2d158a1ddc6588d6005bc

[jamesr, 2014-11-11 00:44:42]

jamesr@chromium.org changed reviewers:
+ davemoore@chromium.org

[jamesr, 2014-11-11 00:44:42]

The problem this patch solves is that InterfaceImpl<> puts the router_/etc in a
member variable on a base class which is highly inflexible.  It means that the
implementation of the interface has to connect to exactly one pipe, can't really
change that over time, can't control the lifetime of the pipe as it wants to,
can't be the endpoint of multiple different interfaces, etc etc. 
InterfaceImpl<> is basically a textbook example of why implementation
inheritance is a bad idea.  This separates the pipe<->interface connection into
a mojo::Connector class (better names welcome) that can be controlled
separately.  This implements InterfaceImpl<> using a single Connector and
provides a StrongConnector for easier strong binding, but doesn't require
clients to use either.

https://codereview.chromium.org/710043004/ is an example of where this could
come in handy as it's a case where what we really want is one C++ object that
handles messages from multiple incoming pipes.

This is the same idea as https://codereview.chromium.org/686883005/ in a
slightly different way (we wrote these patches independently without knowing we
were working on the same thing).

[jamesr, 2014-11-11 01:31:09]

I agree the name collision is confusing.  We should pick a good name for the
public type and then rename the internal type(s) if they conflict. Scott used
the type InterfaceBinding<> which is a bit wordy imo but not too bad.

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

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/connector.h (right):

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/connector.h:19: // This connects an interface
implementation a pipe. Deleting the connector
nit: "to a pipe"

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (left):

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:41: virtual void
OnConnectionEstablished() {}
This seemed like useful functionality. What replaces it?

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:24: InterfaceImpl() :
connector_(this), error_handler_impl_(this) {
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?

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:29: void BindConnector(
Maybe drop the "Connector" from these names, as the connector is not passed.

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:45: Client* client() { return
connector_.client(); }
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.

https://codereview.chromium.org/718473003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_client_factory.cc
(right):

https://codereview.chromium.org/718473003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_client_factory.cc:39:
impl->OnConnectionEstablished();
Unfortunate that this now has to be done manually. Seems prone to error.

https://codereview.chromium.org/718473003/diff/40001/mojo/services/test_servi...
File mojo/services/test_service/test_request_tracker_application.cc (right):

https://codereview.chromium.org/718473003/diff/40001/mojo/services/test_servi...
mojo/services/test_service/test_request_tracker_application.cc:44:
BindToRequest(new TestRequestTrackerImpl(&context_), &request);
Removing OnConnectionEstablished() means we can no longer use the provided
factory object here. That's sad.

https://codereview.chromium.org/718473003/diff/40001/mojo/services/test_servi...
File mojo/services/test_service/test_service_application.cc (right):

https://codereview.chromium.org/718473003/diff/40001/mojo/services/test_servi...
mojo/services/test_service/test_service_application.cc:35: AddRef();
Another new need to do something...can we bring back OnConnectionEstablished()?

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

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (left):

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:41: virtual void
OnConnectionEstablished() {}
On 2014/11/11 17:18:45, DaveMoore wrote:
> This seemed like useful functionality. What replaces it?

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

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:24: InterfaceImpl() :
connector_(this), error_handler_impl_(this) {
On 2014/11/11 17:18:45, DaveMoore wrote:
> 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?

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

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:29: void BindConnector(
On 2014/11/11 17:18:45, DaveMoore wrote:
> Maybe drop the "Connector" from these names, as the connector is not passed.

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

https://codereview.chromium.org/718473003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_client_factory.cc
(right):

https://codereview.chromium.org/718473003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_client_factory.cc:39:
impl->OnConnectionEstablished();
On 2014/11/11 17:18:45, DaveMoore wrote:
> Unfortunate that this now has to be done manually. Seems prone to error.

This is due to the unfortunate fact that BindToRequest() is a free function and
so instead of the class binding itself (which would make more sense) the binding
happens separately and after the construction of the object.  If we transition
ViewManagerClientImpl away from inheriting from InterfaceImpl<> then we can get
rid of this indirection completely.  The constructor for ViewManagerClientImpl
could take care of binding (since we never want to create one without binding it
to something) and then take care of whatever else it wanted to do in order to be
fully functional.

https://codereview.chromium.org/718473003/diff/40001/mojo/services/test_servi...
File mojo/services/test_service/test_request_tracker_application.cc (right):

https://codereview.chromium.org/718473003/diff/40001/mojo/services/test_servi...
mojo/services/test_service/test_request_tracker_application.cc:44:
BindToRequest(new TestRequestTrackerImpl(&context_), &request);
On 2014/11/11 17:18:45, DaveMoore wrote:
> Removing OnConnectionEstablished() means we can no longer use the provided
> factory object here. That's sad.

The fact that the OnConnectionEstablished() exists at all is the bug (see
previous comment).  The constructor should take care of this.

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

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/718473003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:29: void BindConnector(
On 2014/11/11 17:47:49, jamesr wrote:
> On 2014/11/11 17:18:45, DaveMoore wrote:
> > Maybe drop the "Connector" from these names, as the connector is not passed.
> 
> Yeah (I needed a different name in an intermediate state but don't any more). 
> Just Bind() is better

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

[jamesr, 2014-11-11 18:06:05]

https://codereview.chromium.org/718473003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_client_factory.cc
(right):

https://codereview.chromium.org/718473003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_client_factory.cc:39:
impl->OnConnectionEstablished();
See https://codereview.chromium.org/713313002.  The constructor initializes
service_ (which is all OnConnectionEstablished was doing) so the rest of the
class has one fewer state to worry about.

[jamesr, 2014-11-11 19:16:31]

The latest patch renames Connector->Binding and adds more information in the
patch description about these classes and the expected transition plan.  I've
also uploaded 2 patches showing the new pattern used in different places in the
code - links are in the description.

PTAL

[DaveMoore, 2014-11-11 21:16:41]

Now that I understand that the goal is to always use Binding<> this makes more
sense to me. I get the desire to keep common names small but we've already got
InterfacePtr<> everywhere. Maybe Binding should become InterfaceBinding<>. Yuck.

https://codereview.chromium.org/718473003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/binding.h (right):

https://codereview.chromium.org/718473003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/binding.h:89: }
Maybe add 
void Bind(InterfaceRequest<Interface>* request)

[jamesr, 2014-11-12 00:02:21]

I think the error handling here needs more polish (i.e. you really want context
on the OnConnectionError indicating which pipe had the error), but this patch is
as good as the status quo in that area and better in many other ways so I think
this is a useful first step.  I'll keep poking at making the error handling
better to handle the one-instance-many-pipe case better in a follow up patch.

[DaveMoore, 2014-11-13 00:00:36]

lgtm

https://codereview.chromium.org/718473003/diff/140001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/718473003/diff/140001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_impl.h:17: // BindToProxy.
Nit: add comment that this class is going away.

https://codereview.chromium.org/718473003/diff/140001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/strong_binding.h (right):

https://codereview.chromium.org/718473003/diff/140001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/strong_binding.h:69: // Exposed for testing, should not
generally be used.
Nit: name should match Binding::internal_router()

[jamesr, 2014-11-13 00:24:33]

Committed patchset #8 (id:140001) manually as
43fd6836c314a82308f2d158a1ddc6588d6005bc (presubmit successful).

[jamesr, 2014-11-13 01:44:56]

https://codereview.chromium.org/718473003/diff/140001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/interface_impl_internal.h (left):

https://codereview.chromium.org/718473003/diff/140001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_impl_internal.h:100: // If the the
instance is not bound to the pipe, the instance might choose
whoops! this section of logic was pretty important.

restored in https://codereview.chromium.org/724563003/