Mojo C++ bindings: Generate InterfaceHandle<> instead of InterfacePtr<>.

Mojo C++ bindings: Generate InterfaceHandle<> instead of InterfacePtr<>.

This includes replacing all consumers of InterfacePtr<> (which were
previously generated; mostly they are implementations of interfaces)
with InterfaceHandle<>.

This was a fairly mechanical process of replacing InterfacePtr with
InterfaceHandle<>, and I've transformed one from the other at the
earliest occurance.  There are places where I could've delayed
converting InterfaceHandle->InterfacePtr to a later point, but
this entire job was very arduous.

This CL needs to follow up with another doing the same thing, but in
Flutter engine's services.

BUG=#662
R=viettrungluu@chromium.org, jamesr@chromium.org

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

[vardhan, 2016-02-10 00:34:19]

PTAL

[viettrungluu, 2016-02-10 01:04:08]

https://codereview.chromium.org/1682113003/diff/1/examples/native_run_app/nat...
File examples/native_run_app/native_run_app.cc (right):

https://codereview.chromium.org/1682113003/diff/1/examples/native_run_app/nat...
examples/native_run_app/native_run_app.cc:49: explicit
TerminalConnection(mojo::files::FilePtr terminal,
You should probably make this take an InterfaceHandle<mojo::files::File> instead
of a FilePtr.

https://codereview.chromium.org/1682113003/diff/1/examples/spinning_cube/spin...
File examples/spinning_cube/spinning_cube_app.cc (right):

https://codereview.chromium.org/1682113003/diff/1/examples/spinning_cube/spin...
examples/spinning_cube/spinning_cube_app.cc:80:
viewport_->SetEventDispatcher(ptr.PassInterfaceHandle());
So, this seems kind of dumb:

GetProxy() makes a message pipe, binds one end to ptr and returns the other end
inside an InterfaceRequest. Then you immediately unbind ptr.

Maybe we should have an overload for GetProxy() that takes an
InterfaceHandle<T>*?

https://codereview.chromium.org/1682113003/diff/1/mojo/gles2/command_buffer_c...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/1/mojo/gles2/command_buffer_c...
mojo/gles2/command_buffer_client_impl.cc:155:
command_buffer_->Initialize(sync_client.PassInterfaceHandle(),
This seems suspicious to me. I wonder if the various ctors above shouldn't take
InterfaceHandle<T>*s instead.

https://codereview.chromium.org/1682113003/diff/1/mojo/gles2/command_buffer_c...
mojo/gles2/command_buffer_client_impl.cc:157:
observer_ptr.PassInterfaceHandle(), duped.Pass());
And what I said about GetProxy applies here to observer_ptr too.

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
File mojo/public/cpp/application/connect.h (right):

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
mojo/public/cpp/application/connect.h:16: inline void
ConnectToService(ServiceProvider* service_provider,
What I said about GetProxy applies to the various ConnectToService()s too (maybe
there should be overloads?).

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
File mojo/public/cpp/application/lib/application_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
mojo/public/cpp/application/lib/application_impl.cc:44:
local_services.PassInterfaceHandle());
GetProxy argh.

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
mojo/public/cpp/application/lib/application_impl.cc:67: shell_ =
ShellPtr::Create(shell.Pass());
std::move, instead of .Pass()?

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
mojo/public/cpp/application/lib/application_impl.cc:87:
ServiceProviderPtr::Create(std::move(exposed_services)),
Possibly ServiceRegistry's ctor should take an InterfaceHandle?

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/bindings/in...
File mojo/public/cpp/bindings/interface_handle.h (right):

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/bindings/in...
mojo/public/cpp/bindings/interface_handle.h:34:
InterfaceHandle(InterfacePtr<Interface> ptr) {
&& probably.

To make this file compilable, probably you'll have to make this ctor templated
(even if in a stupid way).

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/bindings_serialization.h (left):

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/bindings_serialization.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
I got to this file (and haven't reviewed it yet).

[viettrungluu, 2016-02-11 18:26:29]

lgtm w/nits&notes

Please send a PSA to mojo-dev when you land this.

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

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:85: // TODO(vardhan): Considering deprecating
this since it takes |InterfacePtr|.
Deprecating this sounds good. Logically, its interface is kind of busted, since
even if one wants an InterfacePtr one may actually want different async waiters
for the ptr versus the binding.

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:150: // TODO(vardhan):  Deprecate this?
Ditto.

(Also, why are there two spaces after the ':'? To trigger my OCD?)

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_handle.h (right):

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_handle.h:37: template <typename
SameInterfaceAsAbove>
If you want to make it clearer that users probably shouldn't given a type
parameter, I'd give SameInterfaceAsAbove a default value (namely, |Interface|).

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_request.h (right):

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_request.h:125: template <typename Interface>
Could probably use a comment.

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

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/strong_binding.h:78: // TODO(vardhan): Consider
deprecating this constructor in favor of
What I said earlier....

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/strong_binding.h:134: // TODO(vardhan): Consider
deprecating this constructor in favor of
"

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:185:
&{{output}}->{{name}});
nit: fix indentation

https://codereview.chromium.org/1682113003/diff/40001/mojo/ui/gl_renderer_uni...
File mojo/ui/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1682113003/diff/40001/mojo/ui/gl_renderer_uni...
mojo/ui/gl_renderer_unittest.cc:66: 
nit: probably don't add this blank line

https://codereview.chromium.org/1682113003/diff/40001/services/gfx/compositor...
File services/gfx/compositor/compositor_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/40001/services/gfx/compositor...
services/gfx/compositor/compositor_impl.cc:30: engine_->CreateRenderer(
Why doesn't CompositorEnginer::CreateRenderer() take an InterfaceHandle as its
first param instead?

https://codereview.chromium.org/1682113003/diff/40001/services/gfx/compositor...
File services/gfx/compositor/scene_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/40001/services/gfx/compositor...
services/gfx/compositor/scene_impl.cc:32: engine_->SetListener(state_,
mojo::gfx::composition::SceneListenerPtr::Create(
The same question applies here. (It's not obvious to me in general which layer
should be responsible for going from an IH to an IP.)

You may want to consult Jeff.

https://codereview.chromium.org/1682113003/diff/40001/services/http_server/ht...
File services/http_server/http_server_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/40001/services/http_server/ht...
services/http_server/http_server_impl.cc:46: new Handler(path,
HttpHandlerPtr::Create(std::move(http_handler)));
Maybe make Handler's ctor take an IH instead? <shrug>

[vardhan, 2016-02-11 22:47:53]

https://codereview.chromium.org/1682113003/diff/1/examples/native_run_app/nat...
File examples/native_run_app/native_run_app.cc (right):

https://codereview.chromium.org/1682113003/diff/1/examples/native_run_app/nat...
examples/native_run_app/native_run_app.cc:49: explicit
TerminalConnection(mojo::files::FilePtr terminal,
On 2016/02/10 01:04:08, viettrungluu wrote:
> You should probably make this take an InterfaceHandle<mojo::files::File>
instead
> of a FilePtr.

Done.

https://codereview.chromium.org/1682113003/diff/1/examples/spinning_cube/spin...
File examples/spinning_cube/spinning_cube_app.cc (right):

https://codereview.chromium.org/1682113003/diff/1/examples/spinning_cube/spin...
examples/spinning_cube/spinning_cube_app.cc:80:
viewport_->SetEventDispatcher(ptr.PassInterfaceHandle());
On 2016/02/10 01:04:08, viettrungluu wrote:
> So, this seems kind of dumb:
> 
> GetProxy() makes a message pipe, binds one end to ptr and returns the other
end
> inside an InterfaceRequest. Then you immediately unbind ptr.
> 
> Maybe we should have an overload for GetProxy() that takes an
> InterfaceHandle<T>*?

Done.

https://codereview.chromium.org/1682113003/diff/1/mojo/gles2/command_buffer_c...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/1/mojo/gles2/command_buffer_c...
mojo/gles2/command_buffer_client_impl.cc:155:
command_buffer_->Initialize(sync_client.PassInterfaceHandle(),
On 2016/02/10 01:04:08, viettrungluu wrote:
> This seems suspicious to me. I wonder if the various ctors above shouldn't
take
> InterfaceHandle<T>*s instead.

Done.  (I had mojo::Binding<> accept InterfaceHandle*, and added a note to try
and deprecate accepting an InterfacePtr* instead)

https://codereview.chromium.org/1682113003/diff/1/mojo/gles2/command_buffer_c...
mojo/gles2/command_buffer_client_impl.cc:157:
observer_ptr.PassInterfaceHandle(), duped.Pass());
On 2016/02/10 01:04:08, viettrungluu wrote:
> And what I said about GetProxy applies here to observer_ptr too.

Done.

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
File mojo/public/cpp/application/connect.h (right):

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
mojo/public/cpp/application/connect.h:16: inline void
ConnectToService(ServiceProvider* service_provider,
On 2016/02/10 01:04:08, viettrungluu wrote:
> What I said about GetProxy applies to the various ConnectToService()s too
(maybe
> there should be overloads?).
longer term, should we completely replace these InterfacePtr with
InterfaceHandle when calling ConnectToService()? and let mojo apps convert
InterfaceHandle<> to InterfacePtr<> instead?

I'll add the overloads for now, but I don't think I want to track down consumers
of ConnectToService() in this CL and convert them.. there are a lot of
consumers, so I'll add a TODO here instead.

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
File mojo/public/cpp/application/lib/application_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
mojo/public/cpp/application/lib/application_impl.cc:44:
local_services.PassInterfaceHandle());
On 2016/02/10 01:04:08, viettrungluu wrote:
> GetProxy argh.

Done.

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
mojo/public/cpp/application/lib/application_impl.cc:67: shell_ =
ShellPtr::Create(shell.Pass());
On 2016/02/10 01:04:08, viettrungluu wrote:
> std::move, instead of .Pass()?

Already done in patchset #2

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/application...
mojo/public/cpp/application/lib/application_impl.cc:87:
ServiceProviderPtr::Create(std::move(exposed_services)),
On 2016/02/10 01:04:08, viettrungluu wrote:
> Possibly ServiceRegistry's ctor should take an InterfaceHandle?

Done.

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/bindings/in...
File mojo/public/cpp/bindings/interface_handle.h (right):

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/bindings/in...
mojo/public/cpp/bindings/interface_handle.h:34:
InterfaceHandle(InterfacePtr<Interface> ptr) {
On 2016/02/10 01:04:08, viettrungluu wrote:
> && probably.
> 
> To make this file compilable, probably you'll have to make this ctor templated
> (even if in a stupid way).

Done.
(to be clear, it will currently compile even without the ctor templatization,
but it makes  the interfaceptr<->interfacehandle circular dependency less
finiky.

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/bindings_serialization.h (left):

https://codereview.chromium.org/1682113003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/bindings_serialization.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2016/02/10 01:04:08, viettrungluu wrote:
> I got to this file (and haven't reviewed it yet).

hi!

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

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:85: // TODO(vardhan): Considering deprecating
this since it takes |InterfacePtr|.
On 2016/02/11 18:26:29, viettrungluu wrote:
> Deprecating this sounds good. Logically, its interface is kind of busted,
since
> even if one wants an InterfacePtr one may actually want different async
waiters
> for the ptr versus the binding.

oh true! but i guess this won't be a problem once we remove the InterfacePtr<>*
overload.

i'll make this TODO less of a question and address it in a following CL.

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:150: // TODO(vardhan):  Deprecate this?
On 2016/02/11 18:26:29, viettrungluu wrote:
> Ditto.
> 
> (Also, why are there two spaces after the ':'? To trigger my OCD?)

Done.

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_handle.h (right):

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_handle.h:37: template <typename
SameInterfaceAsAbove>
On 2016/02/11 18:26:29, viettrungluu wrote:
> If you want to make it clearer that users probably shouldn't given a type
> parameter, I'd give SameInterfaceAsAbove a default value (namely,
|Interface|).

Done.

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_request.h (right):

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_request.h:125: template <typename Interface>
On 2016/02/11 18:26:29, viettrungluu wrote:
> Could probably use a comment.

Done.

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/1682113003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:185:
&{{output}}->{{name}});
On 2016/02/11 18:26:29, viettrungluu wrote:
> nit: fix indentation

Done.

https://codereview.chromium.org/1682113003/diff/40001/mojo/ui/gl_renderer_uni...
File mojo/ui/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1682113003/diff/40001/mojo/ui/gl_renderer_uni...
mojo/ui/gl_renderer_unittest.cc:66: 
On 2016/02/11 18:26:29, viettrungluu wrote:
> nit: probably don't add this blank line

Done.

https://codereview.chromium.org/1682113003/diff/40001/services/gfx/compositor...
File services/gfx/compositor/compositor_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/40001/services/gfx/compositor...
services/gfx/compositor/compositor_impl.cc:30: engine_->CreateRenderer(
On 2016/02/11 18:26:29, viettrungluu wrote:
> Why doesn't CompositorEnginer::CreateRenderer() take an InterfaceHandle as its
> first param instead?

Done.

https://codereview.chromium.org/1682113003/diff/40001/services/gfx/compositor...
File services/gfx/compositor/scene_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/40001/services/gfx/compositor...
services/gfx/compositor/scene_impl.cc:32: engine_->SetListener(state_,
mojo::gfx::composition::SceneListenerPtr::Create(
On 2016/02/11 18:26:29, viettrungluu wrote:
> The same question applies here. (It's not obvious to me in general which layer
> should be responsible for going from an IH to an IP.)
> 
> You may want to consult Jeff.

Wasn't obvious to me either. Jeff says leave it here, shouldn't be hard to move
it down the stack later if we need to.

https://codereview.chromium.org/1682113003/diff/40001/services/http_server/ht...
File services/http_server/http_server_impl.cc (right):

https://codereview.chromium.org/1682113003/diff/40001/services/http_server/ht...
services/http_server/http_server_impl.cc:46: new Handler(path,
HttpHandlerPtr::Create(std::move(http_handler)));
On 2016/02/11 18:26:29, viettrungluu wrote:
> Maybe make Handler's ctor take an IH instead? <shrug>

Handler more or less just holds data (if you look at how this http handler is
accessed).  I think it makes sense to convert InterfaceHandle<> to
InterfacePtr<> here.

[vardhan, 2016-02-11 23:08:27]

Description was changed from

==========
Mojo C++ bindings: Generate InterfaceHandle<> instead of InterfacePtr<>.

This includes replacing all consumers of InterfacePtr<> (which were
previously generated; mostly they are implementations of interfaces)
with InterfaceHandle<>.

This was a fairly mechanical process of replacing InterfacePtr with
InterfaceHandle<>, and I've transformed one from the other at the
earliest occurance.  There are places where I could've delayed
converting InterfaceHandle->InterfacePtr to a later point, but
this entire job was very arduous.

This CL needs to follow up with another doing the same thing, but in
Flutter engine's services.

BUG=#662
R=viettrungluu@chromium.org,jamesr@chromium.org
==========

to

==========
Mojo C++ bindings: Generate InterfaceHandle<> instead of InterfacePtr<>.

This includes replacing all consumers of InterfacePtr<> (which were
previously generated; mostly they are implementations of interfaces)
with InterfaceHandle<>.

This was a fairly mechanical process of replacing InterfacePtr with
InterfaceHandle<>, and I've transformed one from the other at the
earliest occurance.  There are places where I could've delayed
converting InterfaceHandle->InterfacePtr to a later point, but
this entire job was very arduous.

This CL needs to follow up with another doing the same thing, but in
Flutter engine's services.

BUG=#662
R=viettrungluu@chromium.org, jamesr@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/c3575c4d8a7aa8a877298f1c8a4...
==========

[vardhan, 2016-02-11 23:08:28]

Committed patchset #4 (id:60001) manually as
c3575c4d8a7aa8a877298f1c8a405bb62e256a04 (presubmit successful).