Mojo: Replace RemotePtr with InterfacePtr and InterfaceImpl

Mojo: Replace RemotePtr with InterfacePtr and InterfaceImpl

Interfaces no longer have explicit Peer attributes. An interface may now optionally have a Client interface, in which case a SetClient method will be auto-generated.

InterfacePtr is a proxy to a remote instance of an interface. InterfaceImpl is a base class used when implementing an interface. Both have facilities for binding to a pipe, etc. An InterfacePtr is movable but not copyable and looks a lot like RemotePtr save for how it gets initialized (via the Bind method now).

I've added some new top-level functions:

MakeProxy - makes it easy to initialize an InterfacePtr in say a member initializer list.

BindToPipe - this is how you bind an InterfaceImpl to a pipe. once bound, they cannot be unbound until the object is destroyed or the pipe is closed.

BindToProxy - builds on top of BindToPipe, however, it hides the details of the pipe. What you get back is an InterfacePtr.

Generated C++ code now passes InterfacePtr instead of InterfaceHandle. As a result, we have far less need for typed subclasses of MessagePipeHandle, so I eliminated them. The code that needs to deal with raw handles generally has to deal with {Scoped}MessagePipeHandle, and adding strong typing to these handles doesn't seem helpful anymore.

R=davemoore@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269443

[darin (slow to review), 2014-05-03 14:58:19]

A few things I've learned:

1) MojoMain needs to take two MessagePipe handles: one for its connection to the
shell and one that will be used for the ShellClient interface.

2) It is common to write a class that wants to own a FooPtr and implement a
client interface that gets set on the FooPtr. In my current approach, the only
way to have a FooClientPtr is to let the bindings manage the lifetime of the
impl backing the FooClientPtr. We probably need some kind of weak binding
instead so the owner of the FooPtr can implement FooClient and just set |this|
on something that gets passed as a FooClientPtr somehow.

[darin (slow to review), 2014-05-04 17:43:24]

On 2014/05/03 14:58:19, darin wrote:
> A few things I've learned:
> 
> 1) MojoMain needs to take two MessagePipe handles: one for its connection to
the
> shell and one that will be used for the ShellClient interface.
> 
> 2) It is common to write a class that wants to own a FooPtr and implement a
> client interface that gets set on the FooPtr. In my current approach, the only
> way to have a FooClientPtr is to let the bindings manage the lifetime of the
> impl backing the FooClientPtr. We probably need some kind of weak binding
> instead so the owner of the FooPtr can implement FooClient and just set |this|
> on something that gets passed as a FooClientPtr somehow.

I've brought back the concept of an interface having a client. It is optional.
To keep things simple, an interface has a SetClient(FooClient*) method, which
maybe overridden to receive the client pointer.

[qsr, 2014-05-05 08:33:38]

https://codereview.chromium.org/265793015/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl
(right):

https://codereview.chromium.org/265793015/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl:30:
virtual void SetClient(mojo::NoInterface* client) {}
I might be missing something, but is there any reason to have this method in the
base class for interfaces, instead of a derived class that would only be used
when the interface has a client? Do we expect this method to be called when the
interface doesn't have a client?

[darin (slow to review), 2014-05-05 15:06:11]

On 2014/05/05 08:33:38, qsr wrote:
...
>
mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl:30:
> virtual void SetClient(mojo::NoInterface* client) {}
> I might be missing something, but is there any reason to have this method in
the
> base class for interfaces, instead of a derived class that would only be used
> when the interface has a client? Do we expect this method to be called when
the
> interface doesn't have a client?

It is convenient to define this dummy version of SetClient (w/ default impl) to
simplify the implementation of InterfacePtr<T>. I could avoid this with
sufficient
complexity there, which may be worth it.

[qsr, 2014-05-05 15:18:28]

On 2014/05/05 15:06:11, darin wrote:
> On 2014/05/05 08:33:38, qsr wrote:
> ...
> >
>
mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl:30:
> > virtual void SetClient(mojo::NoInterface* client) {}
> > I might be missing something, but is there any reason to have this method in
> the
> > base class for interfaces, instead of a derived class that would only be
used
> > when the interface has a client? Do we expect this method to be called when
> the
> > interface doesn't have a client?
> 
> It is convenient to define this dummy version of SetClient (w/ default impl)
to
> simplify the implementation of InterfacePtr<T>. I could avoid this with
> sufficient
> complexity there, which may be worth it.

 The main reason I'm asking would be for user of those interfaces. I'm wondering
if someone might get confuse with this method for a service that doesn't have a
client.

[darin (slow to review), 2014-05-05 15:33:00]

Yup. We can remove it. I may do that as a followup CL though.
On May 5, 2014 8:18 AM, <qsr@chromium.org> wrote:

> On 2014/05/05 15:06:11, darin wrote:
>
>> On 2014/05/05 08:33:38, qsr wrote:
>> ...
>> >
>>
>
> mojo/public/tools/bindings/generators/cpp_templates/
> interface_declaration.tmpl:30:
>
>> > virtual void SetClient(mojo::NoInterface* client) {}
>> > I might be missing something, but is there any reason to have this
>> method in
>> the
>> > base class for interfaces, instead of a derived class that would only be
>>
> used
>
>> > when the interface has a client? Do we expect this method to be called
>> when
>> the
>> > interface doesn't have a client?
>>
>
>  It is convenient to define this dummy version of SetClient (w/ default
>> impl)
>>
> to
>
>> simplify the implementation of InterfacePtr<T>. I could avoid this with
>> sufficient
>> complexity there, which may be worth it.
>>
>
>  The main reason I'm asking would be for user of those interfaces. I'm
> wondering
> if someone might get confuse with this method for a service that doesn't
> have a
> client.
>
> https://codereview.chromium.org/265793015/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2014-05-06 21:30:53]

https://codereview.chromium.org/265793015/diff/270001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_ptr.h (right):

https://codereview.chromium.org/265793015/diff/270001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_ptr.h:184: //
Maybe there should be a variant of this that takes the Client, since we do this
automatically in other cases.

https://codereview.chromium.org/265793015/diff/270001/mojo/service_manager/se...
File mojo/service_manager/service_manager.cc (right):

https://codereview.chromium.org/265793015/diff/270001/mojo/service_manager/se...
mojo/service_manager/service_manager.cc:36: void Orphan() {
It can be hard to track down bugs when you can either delete the ServiceFactory
directly or through the Manager. I used to have something just like this and it
got very confusing. What now requires it?

https://codereview.chromium.org/265793015/diff/270001/mojo/service_manager/se...
mojo/service_manager/service_manager.cc:200: void
ServiceManager::RemoveServiceFactory(ServiceFactory* service_factory) {
This is called OnServiceFactoryError() because it sends OnServiceError() to the
Loader. Either that name should be changed as well, or this should remain.

[darin (slow to review), 2014-05-06 21:39:10]

https://codereview.chromium.org/265793015/diff/270001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_ptr.h (right):

https://codereview.chromium.org/265793015/diff/270001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_ptr.h:184: //
On 2014/05/06 21:30:54, DaveMoore wrote:
> Maybe there should be a variant of this that takes the Client, since we do
this
> automatically in other cases.

Yeah, that's something I've been considering as well...

https://codereview.chromium.org/265793015/diff/270001/mojo/service_manager/se...
File mojo/service_manager/service_manager.cc (right):

https://codereview.chromium.org/265793015/diff/270001/mojo/service_manager/se...
mojo/service_manager/service_manager.cc:36: void Orphan() {
On 2014/05/06 21:30:54, DaveMoore wrote:
> It can be hard to track down bugs when you can either delete the
ServiceFactory
> directly or through the Manager. I used to have something just like this and
it
> got very confusing. What now requires it?

The lifetime of the ServiceFactory is now bound to the lifetime of the
MessagePipe used by the client to talk to its Shell instance.

https://codereview.chromium.org/265793015/diff/270001/mojo/service_manager/se...
mojo/service_manager/service_manager.cc:200: void
ServiceManager::RemoveServiceFactory(ServiceFactory* service_factory) {
On 2014/05/06 21:30:54, DaveMoore wrote:
> This is called OnServiceFactoryError() because it sends OnServiceError() to
the
> Loader. Either that name should be changed as well, or this should remain.

I just had service.h on the mind. Renamed back to OnServiceFactoryError.

[darin (slow to review), 2014-05-08 08:01:33]

OK, the CL is fairly solid now. Apologies, I probably let a couple gratuitous
changes sneak into this CL, but I really tried to avoid temptation!

[DaveMoore, 2014-05-08 18:27:29]

There seems to be a lot of cases where we define trivial SetClient()
implementations, as well as OnConnectionError(). That may be a sign we don't
quite have it right yet.

https://codereview.chromium.org/265793015/diff/530001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.h (right):

https://codereview.chromium.org/265793015/diff/530001/chrome/browser/ui/webui...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.h:26: public
mojo::InterfaceImpl<OmniboxUIHandlerMojo>,
This is kind of a mouthful. Should we generate a shorter typedef?

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

https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_impl.h:29: // DO NOT USE. Exposed only for
internal use and for testing.
Perhaps this could be exposed by creating a nested class and friending that
class to the allowed clients.

https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
File mojo/public/cpp/shell/service.h (left):

https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
mojo/public/cpp/shell/service.h:180: RemotePtr<typename ServiceInterface::_Peer>
client_;
Could we somehow preserve having the client_ stored here optionally? I hate to
make all implementors that have a client have to define it themselves. Maybe
default it to NoInterface, and throw an error if accessed when there is none.
Then we could add the client into the constructor and generate the right code
when necessary.

https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
File mojo/public/cpp/shell/service.h (right):

https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
mojo/public/cpp/shell/service.h:148: typedef ServiceInterface Interface;
What uses this?

https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
mojo/public/cpp/shell/service.h:160: delete this;
I prefer the model where the delete always happens from the owner, so that this
would call RemoveConnection() which would then continue to delete what's being
removed.

[darin (slow to review), 2014-05-08 19:34:15]

On 2014/05/08 18:27:29, DaveMoore wrote:
> There seems to be a lot of cases where we define trivial SetClient()
> implementations, as well as OnConnectionError(). That may be a sign we don't
> quite have it right yet.

Ideally, the client pointer would be passed to the implementation upon
construction. The main problem with that is that I want users to be able
to control how subclasses of InterfaceImpl<..> get instantiated, and the
client pointer (the proxy impl of that) is not available until after the
implementation is bound to a pipe. Hence, it has to be post-construction
time that they get given the client.

In addition, having it be a setter allows it to be ignored or not called
when not needed.

I tried making OnConnectionError hidden and automatically delete this by
default, but that was almost never what any code wants. Forcing people to
implement OnConnectionError solves two issues: 1) it makes lifetime of the
InterfaceImpl<..> subclass more transparent to the user, 2) it makes the
fact that you need to think about OnConnectionError more prominent to the
user. These seem like good things. I'm actually thinking that
OnConnectionError should be a method alongside SetClient that you are
forced to implement! It's valuable to have it be prominent.


>
https://codereview.chromium.org/265793015/diff/530001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.h (right):
> 
>
https://codereview.chromium.org/265793015/diff/530001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.h:26: public
> mojo::InterfaceImpl<OmniboxUIHandlerMojo>,
> This is kind of a mouthful. Should we generate a shorter typedef?

I struggled with this one. Originally, I tried a Server suffix, but I didn't
like that very much. I ended up with things like ExtensionServiceServer. It
also isn't clear that "Server" is that great of a suffix. I didn't want to
go with an "Impl" suffix because I want users to have that suffix available
to them for their subclasses. I like a template because it allows the interface
name to be more explicit, so you think about the fact that you are implementing
that interface. I also think we might want to add some optional template args
in the future.

That said, I'm totally open to suggestion here!


>
https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/interface_impl.h (right):
> 
>
https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/interface_impl.h:29: // DO NOT USE. Exposed only for
> internal use and for testing.
> Perhaps this could be exposed by creating a nested class and friending that
> class to the allowed clients.

We could certainly do that. This was nice for tests and I figure the name of
the function should be foreboding enough that normal users wouldn't call it.
Take a look at the use in js_to_cpp_unittest.cc for example.


>
https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
> File mojo/public/cpp/shell/service.h (left):
> 
>
https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
> mojo/public/cpp/shell/service.h:180: RemotePtr<typename
ServiceInterface::_Peer>
> client_;
> Could we somehow preserve having the client_ stored here optionally? I hate to
> make all implementors that have a client have to define it themselves. Maybe
> default it to NoInterface, and throw an error if accessed when there is none.
> Then we could add the client into the constructor and generate the right code
> when necessary.

We could do that. It would be easy to implement SetClient here and have a
client()
getter. I could give that a try, but ultimately my goal is to eliminate
ServiceConnection in favor of InterfaceImpl. Maybe InterfaceImpl should
implement
SetClient! Yeah, that seems nice. Let me try that :)


>
https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
> File mojo/public/cpp/shell/service.h (right):
> 
>
https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
> mojo/public/cpp/shell/service.h:148: typedef ServiceInterface Interface;
> What uses this?

Nothing anymore. I can remove it. It was needed by an interim version
of this patch.


>
https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
> mojo/public/cpp/shell/service.h:160: delete this;
> I prefer the model where the delete always happens from the owner, so that
this
> would call RemoveConnection() which would then continue to delete what's being
> removed.

OK, I can restore that. This was a side-effect of an interim version of this
code.

[darin (slow to review), 2014-05-08 19:53:35]

>
https://codereview.chromium.org/265793015/diff/530001/mojo/public/cpp/shell/s...
> > mojo/public/cpp/shell/service.h:180: RemotePtr<typename
> ServiceInterface::_Peer>
> > client_;
> > Could we somehow preserve having the client_ stored here optionally? I hate
to
> > make all implementors that have a client have to define it themselves. Maybe
> > default it to NoInterface, and throw an error if accessed when there is
none.
> > Then we could add the client into the constructor and generate the right
code
> > when necessary.
> 
> We could do that. It would be easy to implement SetClient here and have a
> client()
> getter. I could give that a try, but ultimately my goal is to eliminate
> ServiceConnection in favor of InterfaceImpl. Maybe InterfaceImpl should
> implement
> SetClient! Yeah, that seems nice. Let me try that :)

Quick note:

It turns out that adding a client parameter to MakeProxy is less valuable
than I expected. If we are successful at burying the pipes, then it means
you will mostly be passing around FooPtrs. Once the FooPtr arrives at its
intended owner, that owner will want to call SetClient on the FooPtr. The
only time when you want to set a client and call MakeProxy at the same
time is when you are starting with a ScopedMessagePipeHandle. That should
hopefully not be the common case.

[DaveMoore, 2014-05-08 19:59:35]

lgtm

[darin (slow to review), 2014-05-08 22:01:12]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-08 22:08:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/265793015/570001

[commit-bot: I haz the power, 2014-05-09 04:11:42]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 04:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 04:33:54]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[darin (slow to review), 2014-05-09 05:17:32]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-09 05:23:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/265793015/570001

[darin (slow to review), 2014-05-09 05:34:28]

The CQ bit was unchecked by darin@chromium.org

[darin (slow to review), 2014-05-09 05:39:46]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-09 05:42:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/265793015/590001

[darin (slow to review), 2014-05-09 22:36:02]

Committed patchset #34 manually as r269443 (presubmit successful).

[piman, 2014-05-09 23:02:57]

https://codereview.chromium.org/265793015/diff/610001/mojo/gles2/gles2_suppor...
File mojo/gles2/gles2_support_impl.cc (right):

https://codereview.chromium.org/265793015/diff/610001/mojo/gles2/gles2_suppor...
mojo/gles2/gles2_support_impl.cc:66: ScopedMessagePipeHandle
scoped_handle(handle);
drive-by: We couldn't keep the type safety?

[darin (slow to review), 2014-05-09 23:05:54]

It is not needed if we are successful at burying the pipes.

For sync calls in particular, I plan to add machinery so you won't have to
deal with raw pipes.

-Darin


On Fri, May 9, 2014 at 4:02 PM, <piman@chromium.org> wrote:

>
> https://codereview.chromium.org/265793015/diff/610001/
> mojo/gles2/gles2_support_impl.cc
> File mojo/gles2/gles2_support_impl.cc (right):
>
> https://codereview.chromium.org/265793015/diff/610001/
> mojo/gles2/gles2_support_impl.cc#newcode66
> mojo/gles2/gles2_support_impl.cc:66: ScopedMessagePipeHandle
> scoped_handle(handle);
> drive-by: We couldn't keep the type safety?
>
> https://codereview.chromium.org/265793015/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.