Pass ServiceProvider and ServiceProvider& params in Connect

Pass ServiceProvider and ServiceProvider& params in Connect

In preperation for removing the Client annotation from ServiceProvider
this passes a second parameter of type ServiceProvider in the shell and
application Connect calls. In this patch the type signatures are updated
but the second parameter is basically unused. The intention is that the
first parameter |services| will be used for the connecting application to
request services from the connected application (as it does currently)
and the second parameter |exported_services| be used for the connecting
application to provide services to the connected application. We have
very few services exported in the second direction today - I'll update
them to use the second parameter in a follow-up patch.

R=darin@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/3217bf35ed48e84b878057973632506a8d337f4b

[jamesr, 2015-01-08 23:07:15]

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

[darin (slow to review), 2015-01-09 17:31:16]

https://codereview.chromium.org/845593003/diff/1/mojo/public/interfaces/appli...
File mojo/public/interfaces/application/application.mojom (right):

https://codereview.chromium.org/845593003/diff/1/mojo/public/interfaces/appli...
mojo/public/interfaces/application/application.mojom:17: ServiceProvider&?
services,
I'm a little confused by the use of & here. Usually that is a way to get
something, and I would expect the thing the caller gets here to be the exported
services. It seems like the signature should be:

  AccceptConnection(string? requestor_url,
                    ServiceProvider? requestor_services,
                    ServiceProvider&? exported_services);

https://codereview.chromium.org/845593003/diff/1/mojo/public/interfaces/appli...
File mojo/public/interfaces/application/shell.mojom (right):

https://codereview.chromium.org/845593003/diff/1/mojo/public/interfaces/appli...
mojo/public/interfaces/application/shell.mojom:19: ServiceProvider&? services,
Ditto

[Aaron Boodman, 2015-01-09 17:53:35]

aa@chromium.org changed reviewers:
+ aa@chromium.org

[Aaron Boodman, 2015-01-09 17:53:36]

https://codereview.chromium.org/845593003/diff/1/mojo/public/interfaces/appli...
File mojo/public/interfaces/application/application.mojom (right):

https://codereview.chromium.org/845593003/diff/1/mojo/public/interfaces/appli...
mojo/public/interfaces/application/application.mojom:16:
AcceptConnection(string? requestor_url,
We should really remove the question mark. requestor_url is not optional. I can
do it in a separate change though.

https://codereview.chromium.org/845593003/diff/1/mojo/public/interfaces/appli...
mojo/public/interfaces/application/application.mojom:17: ServiceProvider&?
services,
On 2015/01/09 17:31:16, darin wrote:
> I'm a little confused by the use of & here. Usually that is a way to get
> something, and I would expect the thing the caller gets here to be the
exported
> services. It seems like the signature should be:
> 
>   AccceptConnection(string? requestor_url,
>                     ServiceProvider? requestor_services,
>                     ServiceProvider&? exported_services);

These names are always confusing because they can be interpreted both ways:

'exported_services' can mean either services exported from the app receiving
this call, or services exported from the app making the call.

Perhaps we should say connector and connectee, or requestor and requestee, or
client and provider...

[jamesr, 2015-01-09 20:39:32]

On 2015/01/09 17:53:36, Aaron Boodman wrote:
> These names are always confusing because they can be interpreted both ways:
> 
> 'exported_services' can mean either services exported from the app receiving
> this call, or services exported from the app making the call.

I intended the latter here, but Darin thought the former (which is also
reasonable).  It's definitely a problem.

> 
> Perhaps we should say connector and connectee, or requestor and requestee, or
> client and provider...

How about

  AcceptConnection(string requestor_url,
                   ServiceProvider&? requested_services,
                   ServiceProvider? requestor_services);

?

[darin (slow to review), 2015-01-09 20:44:50]

On 2015/01/09 20:39:32, jamesr wrote:
> On 2015/01/09 17:53:36, Aaron Boodman wrote:
> > These names are always confusing because they can be interpreted both ways:
> > 
> > 'exported_services' can mean either services exported from the app receiving
> > this call, or services exported from the app making the call.
> 
> I intended the latter here, but Darin thought the former (which is also
> reasonable).  It's definitely a problem.
> 
> > 
> > Perhaps we should say connector and connectee, or requestor and requestee,
or
> > client and provider...
> 
> How about
> 
>   AcceptConnection(string requestor_url,
>                    ServiceProvider&? requested_services,
>                    ServiceProvider? requestor_services);
> 
> ?

Sorry, I realize I just repeated my earlier suggestion :-P

[darin (slow to review), 2015-01-09 20:47:13]

On 2015/01/09 20:44:50, darin wrote:
...

Oops, I meant to reply to my email here:

> I like requestor_services. It is nice how that parallels
> requestor_url. I'm not as happy with requested_services
> since not all of the services exposed there will be requested
> by the caller. How about we stick with exported_services
> instead of requested_services, but swap the order of the
> parameters so the input args are together and the output arg
> is listed last?
>
>   AcceptConnection(
>       string requestor_url,
>       ServiceProvider? requestor_services,
>       ServiceProvider&? exported_services);
>


"Sorry, I realize I just repeated my earlier suggestion :-P"

[jamesr, 2015-01-09 20:48:47]

The reason I went with this parameter order is that currently the
ServiceProvider& parameter is used way more commonly, so it seemed weird to
stick it at the end.  In other words the actor initiating the connection is
normally the one that wants to initiate service requests.  If I was writing
javascript, I'd want the less commonly used parameters at the end so I can just
leave them out.

[darin (slow to review), 2015-01-09 20:54:16]

On 2015/01/09 20:48:47, jamesr wrote:
> The reason I went with this parameter order is that currently the
> ServiceProvider& parameter is used way more commonly, so it seemed weird to
> stick it at the end.  In other words the actor initiating the connection is
> normally the one that wants to initiate service requests.  If I was writing
> javascript, I'd want the less commonly used parameters at the end so I can
just
> leave them out.

But we are talking about AcceptConnection, right? JS wouldn't be able to
ignore any of the parameters.

[jamesr, 2015-01-09 22:12:59]

We expose the same thing on mojo.Shell, which is exposed to JS/etc.  The
signatures for these could be different but that'd be hard to understand, I
think.

Trung pointed out a very important but subtle point.  When connecting to an
application, the meaning of the interface names for both of these
ServiceProviders must be up to the application being connected to.  In other
words, if I connect to "mojo:tracing" then the interface names I request and the
interface names I provide both have meaning in the context of the "mojo:tracing"
URL, even though other things might use the same interface names in other
contexts.  Specifically I might expose interfaces with colliding names to other
actors that connect to me that are unrelated to tracing.  Thus we should try to
make it clear that this pair is specific to the connection either through naming
or comments or other documentation.

[jamesr, 2015-01-12 21:48:57]

jamesr@chromium.org changed reviewers:
+ viettrungluu@chromium.org
- darin@chromium.org

[jamesr, 2015-01-12 21:48:58]

This uses ppi@'s suggestion of |services| and |exposed_services|.

[darin (slow to review), 2015-01-13 07:54:50]

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

[darin (slow to review), 2015-01-13 07:54:51]

LGTM

https://codereview.chromium.org/845593003/diff/20001/examples/bitmap_uploader...
File examples/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/845593003/diff/20001/examples/bitmap_uploader...
examples/bitmap_uploader/bitmap_uploader.cc:49: ServiceProviderPtr());
note for a future CL: we could make nullptr work in these cases :)

https://codereview.chromium.org/845593003/diff/20001/mojo/public/interfaces/a...
File mojo/public/interfaces/application/application.mojom (right):

https://codereview.chromium.org/845593003/diff/20001/mojo/public/interfaces/a...
mojo/public/interfaces/application/application.mojom:15: // Called in response
to a call to mojo.Shell.ConnectToApplication().
I think it'd be worth documenting the meaning of the parameters :)

[jamesr, 2015-01-13 21:48:03]

Thanks, added some comments and made the url parameters non-nullable.  I'll work
on the nullptr thing in parallel - that'd be super useful :)

[hansmuller1, 2015-01-13 22:17:16]

I think the classes in mojo/services/public/js will need to be updated per this
change.

[jamesr, 2015-01-14 00:35:34]

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

[jamesr, 2015-01-14 00:35:34]

Hans - could you take a look at the mojo/services/public/js and
services/js/echo_apptest.cc changes?

[hansmuller1, 2015-01-14 22:27:31]

On 2015/01/14 00:35:34, jamesr wrote:
> Hans - could you take a look at the mojo/services/public/js and
> services/js/echo_apptest.cc changes?

The changes LGTM. Sorry to have taken so long.

[jamesr, 2015-01-14 22:33:12]

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

[hansmuller1, 2015-01-15 18:58:45]

On 2015/01/14 22:33:12, jamesr wrote:
> Committed patchset #5 (id:80001) manually as
> 3217bf35ed48e84b878057973632506a8d337f4b (presubmit successful).

A JS bindings for null/unspecified handles is here:
https://codereview.chromium.org/825613004/