Use Binding instead of InterfaceImpl for ExampleService.

Use Binding instead of InterfaceImpl for ExampleService.

BUG=392646
TEST=mojo_shell mojo:example_apptests still works.
R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/cb82b1fafb3591fe5cdeb6b22c84398014d15727

[msw, 2014-11-17 21:41:42]

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

[msw, 2014-11-17 21:42:36]

Hey James, please take a look; and lmk what you think about an
InterfaceFactoryImpl for the simple Binding case.

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_application.h (left):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_application.h:27:
InterfaceFactoryImpl<ExampleServiceImpl> example_service_factory_;
Question: Do we want an InterfaceFactory implementation that supports simple
implementation binding cases like this? I could add something like
interface_factory_impl.h's InterfaceFactoryImpl, that instantiates the Impl and
passes the InterfaceRequest (or ScopedMessagePipeHandle as cited by binding.h?)
to the Impl's ctor.

[jamesr, 2014-11-17 22:33:04]

I think this version leaks.

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_application.cc (right):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_application.cc:26: new
ExampleServiceImpl(request.Pass());
what controls the lifetime of this service?

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_application.h (left):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_application.h:27:
InterfaceFactoryImpl<ExampleServiceImpl> example_service_factory_;
On 2014/11/17 21:42:36, msw wrote:
> Question: Do we want an InterfaceFactory implementation that supports simple
> implementation binding cases like this? I could add something like
> interface_factory_impl.h's InterfaceFactoryImpl, that instantiates the Impl
and
> passes the InterfaceRequest (or ScopedMessagePipeHandle as cited by
binding.h?)
> to the Impl's ctor.

Maybe. I think the way to evaluate whether helpers like this pull their weight
is to look at how many places would user a helper of a particular form. That'll
be easier to assess when more things are ported from using InterfaceImpl to
using Binding<>.

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_impl.cc (right):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_impl.cc:10:
mojo::InterfaceRequest<ExampleService> request)
no ::mojo

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_impl.h (right):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_impl.h:17: explicit
ExampleServiceImpl(mojo::InterfaceRequest<ExampleService> request);
this code is in namespace mojo, don't say mojo:: again

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_impl.h:25: mojo::Binding<ExampleService>
binding_;
no mojo::

if you want this object to delete itself on detecting a pipe error, you want to
use StrongBinding<> here

[msw, 2014-11-17 23:05:32]

Please take another look; thanks!

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_application.cc (right):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_application.cc:26: new
ExampleServiceImpl(request.Pass());
On 2014/11/17 22:33:04, jamesr wrote:
> what controls the lifetime of this service?

Yikes, thank you! I changed ExampleServiceImpl::binding_ to be a StrongBinding
as you suggested and the instances are now destroyed with each test.

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_application.h (left):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_application.h:27:
InterfaceFactoryImpl<ExampleServiceImpl> example_service_factory_;
On 2014/11/17 22:33:04, jamesr wrote:
> On 2014/11/17 21:42:36, msw wrote:
> > Question: Do we want an InterfaceFactory implementation that supports simple
> > implementation binding cases like this? I could add something like
> > interface_factory_impl.h's InterfaceFactoryImpl, that instantiates the Impl
> and
> > passes the InterfaceRequest (or ScopedMessagePipeHandle as cited by
> binding.h?)
> > to the Impl's ctor.
> 
> Maybe. I think the way to evaluate whether helpers like this pull their weight
> is to look at how many places would user a helper of a particular form.
That'll
> be easier to assess when more things are ported from using InterfaceImpl to
> using Binding<>.

Acknowledged, I'll hold off on this.

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_impl.cc (right):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_impl.cc:10:
mojo::InterfaceRequest<ExampleService> request)
On 2014/11/17 22:33:04, jamesr wrote:
> no ::mojo

Done.

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
File examples/apptest/example_service_impl.h (right):

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_impl.h:17: explicit
ExampleServiceImpl(mojo::InterfaceRequest<ExampleService> request);
On 2014/11/17 22:33:04, jamesr wrote:
> this code is in namespace mojo, don't say mojo:: again

Done.

https://codereview.chromium.org/731293003/diff/20001/examples/apptest/example...
examples/apptest/example_service_impl.h:25: mojo::Binding<ExampleService>
binding_;
On 2014/11/17 22:33:04, jamesr wrote:
> no mojo::
> 
> if you want this object to delete itself on detecting a pipe error, you want
to
> use StrongBinding<> here

Done.

[jamesr, 2014-11-17 23:16:14]

lgtm

https://codereview.chromium.org/731293003/diff/40001/examples/apptest/example...
File examples/apptest/example_service_impl.h (right):

https://codereview.chromium.org/731293003/diff/40001/examples/apptest/example...
examples/apptest/example_service_impl.h:26: // ExampleServiceImpl deletes itself
on detecting a pipe error.
not technically true (the binding deletes ExampleServiceImpl) and a bit
confusing. i'd simplify this to something like "ExampleServiceImpl is strongly
bound to the pipe" or something like that

[msw, 2014-11-17 23:27:03]

Landing, thanks!

https://codereview.chromium.org/731293003/diff/40001/examples/apptest/example...
File examples/apptest/example_service_impl.h (right):

https://codereview.chromium.org/731293003/diff/40001/examples/apptest/example...
examples/apptest/example_service_impl.h:26: // ExampleServiceImpl deletes itself
on detecting a pipe error.
On 2014/11/17 23:16:14, jamesr wrote:
> not technically true (the binding deletes ExampleServiceImpl) and a bit
> confusing. i'd simplify this to something like "ExampleServiceImpl is strongly
> bound to the pipe" or something like that

Sorry; done!

[msw, 2014-11-17 23:28:14]

Committed patchset #5 (id:80001) manually as
cb82b1fafb3591fe5cdeb6b22c84398014d15727 (presubmit successful).