Provides a way to use bindings that doesn't require subclassing

Provides a way to use bindings that doesn't require subclassing

I'm writing a test and want to test a concrete implementation of my
mojo service. My service has a client. I don't want to create a pipe
or anything for my test; that all doesn't matter. Rather I want to
directly create my concrete service class and supply a test client
implementation. This has the added benefit of all being synchronous.

Having my service implementation subclass InterfaceImpl is a bit
awkward. It doesn't make sense for InterfaceImpl to allow setting the
client and I get some baggage from the superclass.

I split out the logic managing the binding into a class called
InterfaceBinding. I changed InterfaceImpl to internally use this
class. So, you can either subclass InterfaceImpl (as you always have)
or use InterfaceBinding supplying your service implementation (that
doesn't subclass InterfaceImpl).

Having InterfaceImpl and InterfaceBinding share code results in some
what tricky code.

R=davemoore@chromium.org, jamesr@chromium.org

[sky, 2014-11-10 23:25:53]

sky@chromium.org changed reviewers:
+ darin@chromium.org

[sky, 2014-11-10 23:26:04]

sky@chromium.org changed reviewers:
- darin@chromium.org

[jamesr, 2014-11-10 23:35:22]

I wrote almost the same class independently on a plane this weekend so I
definitely think something like this is needed.  Could you split the new class
into its own file?

I found it really useful to separate the delete-on-error behavior out of the
base type.

[Aaron Boodman, 2014-11-10 23:45:40]

I don't see the part that allows you to set the client...?

[DaveMoore, 2014-11-10 23:54:23]

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

https://codereview.chromium.org/686883005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:30: // . InterfaceBinding can own the
Nit: Extra line?

https://codereview.chromium.org/686883005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:77: }
Nit: blank line

https://codereview.chromium.org/686883005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:100: // while calling to delegate.
Where does the destructor set this?

[sky, 2014-11-11 00:34:57]

I've uploaded a new patch. I have one more TODO should we decide to go this
route. James is going to upload his approach and then we can decide which one we
want to go with.

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

https://codereview.chromium.org/686883005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:30: // . InterfaceBinding can own the
On 2014/11/10 23:54:23, DaveMoore wrote:
> Nit: Extra line?

Done.

https://codereview.chromium.org/686883005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:77: }
On 2014/11/10 23:54:23, DaveMoore wrote:
> Nit: blank line


The lack of newline is done to reinforce OnConnectionError is part of
InterfaceBindingDelegate. It's the same as would be done if this was the
declaration.

https://codereview.chromium.org/686883005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:100: // while calling to delegate.
On 2014/11/10 23:54:23, DaveMoore wrote:
> Where does the destructor set this?

D'OH! Apparently no tests needed this logic. Added in setting of destroyed_ in
destructor.

[jamesr, 2014-11-11 00:37:27]

https://codereview.chromium.org/718473003 is the thing I wrote up on the plane -
it goes slightly further than this patch but does basically the same thing -
i.e. have a separate object to manage the relationship between the subclass of
the mojom.h interface and the pipe that's less prescriptive.

[DaveMoore, 2014-11-11 16:04:15]

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

https://codereview.chromium.org/686883005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:100: // while calling to delegate.
On 2014/11/11 00:34:57, sky wrote:
> On 2014/11/10 23:54:23, DaveMoore wrote:
> > Where does the destructor set this?
> 
> D'OH! Apparently no tests needed this logic. Added in setting of destroyed_ in
> destructor.

If we keep this cl you should add a test for this behavior, as it's tricky.

[sky, 2014-11-11 18:35:45]

There doesn't seem to be a strong preference for this or James's. James seems
further along and his has some nice benefits, so I'm closing this out.