Make embedder.serviceRegistry return a ServiceRegistry instead of ServiceRegistryProxy.

Make embedder.serviceRegistry return a ServiceRegistry instead of ServiceRegistryProxy.

The proxy has methods like close() that I think we're not intending to expose to the
caller. The ServiceRegistry is just the interface itself. This also avoids users of
the API from having to add a .ptr to their usages of the registry.

BUG=
R=eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/60f07c5317dda07603a876bc7f0165b7152bbcbb

[tonyg, 2015-04-01 22:41:05]

tonyg@chromium.org changed reviewers:
+ eseidel@chromium.org

[tonyg, 2015-04-01 22:42:23]

afaict, (via build, test and grep) nothing in the mojo repo calls this.

My main motivation was just to make the API slightly cleaner. I'm happy to
abandon this if it isn't a good idea.

https://codereview.chromium.org/1053883002/diff/1/sky/framework/embedder.dart
File sky/framework/embedder.dart (left):

https://codereview.chromium.org/1053883002/diff/1/sky/framework/embedder.dart...
sky/framework/embedder.dart:17: ServiceRegistry _serviceRegistry;
Not quite sure how this worked before. Does `mojob.py build` run the analyzer on
these files?

[eseidel, 2015-04-01 22:43:24]

https://codereview.chromium.org/1053883002/diff/1/sky/framework/embedder.dart
File sky/framework/embedder.dart (left):

https://codereview.chromium.org/1053883002/diff/1/sky/framework/embedder.dart...
sky/framework/embedder.dart:17: ServiceRegistry _serviceRegistry;
On 2015/04/01 at 22:42:23, tonyg wrote:
> Not quite sure how this worked before. Does `mojob.py build` run the analyzer
on these files?

This line was changed earlier today, iirc?

[tonyg, 2015-04-01 22:45:26]

On 2015/04/01 22:43:24, eseidel wrote:
> https://codereview.chromium.org/1053883002/diff/1/sky/framework/embedder.dart
> File sky/framework/embedder.dart (left):
> 
>
https://codereview.chromium.org/1053883002/diff/1/sky/framework/embedder.dart...
> sky/framework/embedder.dart:17: ServiceRegistry _serviceRegistry;
> On 2015/04/01 at 22:42:23, tonyg wrote:
> > Not quite sure how this worked before. Does `mojob.py build` run the
analyzer
> on these files?
> 
> This line was changed earlier today, iirc?

hmmm.... not seeing it
https://github.com/domokit/mojo/commits/master/sky/framework/embedder.dart

[eseidel, 2015-04-01 22:48:38]

https://codereview.chromium.org/1047363002

[tonyg, 2015-04-01 22:57:44]

On 2015/04/01 22:48:38, eseidel wrote:
> https://codereview.chromium.org/1047363002

Ah, gotcha. I see that wasn't landed yet. Anyway, this patch makes that same
change plus changes the type returned from a ServiceRegistryProxy to a
ServiceRegistry. I checked the commit box on Hans' patch and can rebase this one
if it lands first. But I'm still curious for your thoughts on the changed return
type.

[eseidel, 2015-04-01 23:27:15]

This code seems otherwise unused.  I'm not the original author, but the original
author is out on leave.  lgtm to change, we can always change it back later.

[tonyg, 2015-04-01 23:33:19]

Committed patchset #1 (id:1) manually as
60f07c5317dda07603a876bc7f0165b7152bbcbb (presubmit successful).

[hansmuller1, 2015-04-02 14:19:47]

On 2015/04/01 23:33:19, tonyg wrote:
> Committed patchset #1 (id:1) manually as
> 60f07c5317dda07603a876bc7f0165b7152bbcbb (presubmit successful).

I would have preferred that you hadn't made this change; I'd prefer not to have
APIs that return foo.ptr.

I'd had a different pending change - https://codereview.chromium.org/10473630

[tonyg, 2015-04-02 14:49:45]

On 2015/04/02 14:19:47, hansmuller1 wrote:
> On 2015/04/01 23:33:19, tonyg wrote:
> > Committed patchset #1 (id:1) manually as
> > 60f07c5317dda07603a876bc7f0165b7152bbcbb (presubmit successful).
> 
> I would have preferred that you hadn't made this change; I'd prefer not to
have
> APIs that return foo.ptr.

You have your head around the API a lot better than I so I'm happy to revert if
you prefer. But I would like to understand the reasoning for not having APIs
that return a proxy's ptr.

In that mega-patch where we had all those clean-shutdown issues, Jim and I
talked a lot about passing around proxies in our code and came to the conclusion
that if you pass the full proxy, it either means the caller must take some
responsibility over its lifetime (like needing to call close()) or more
practically speaking it leaves it ambiguous to the caller whether they're
supposed to. However, to make it clear that the caller is not responsible for
the lifetime because the vending class controls it, an api can pass only the
pointer. This makes it obvious that the caller caller isn't responsible for
closing because it doesn't have access to the lifetime related methods.
Thoughts?

> I'd had a different pending change - https://codereview.chromium.org/10473630

So this patch fixed that type bug in the same way (because I just happened to
notice it), but it was orthogonal to the API change intended in my patch.

[hansmuller1, 2015-04-02 15:03:55]

On 2015/04/02 14:49:45, tonyg wrote:
> On 2015/04/02 14:19:47, hansmuller1 wrote:
> > On 2015/04/01 23:33:19, tonyg wrote:
> > > Committed patchset #1 (id:1) manually as
> > > 60f07c5317dda07603a876bc7f0165b7152bbcbb (presubmit successful).
> > 
> > I would have preferred that you hadn't made this change; I'd prefer not to
> have
> > APIs that return foo.ptr.
> 
> You have your head around the API a lot better than I so I'm happy to revert
if
> you prefer. But I would like to understand the reasoning for not having APIs
> that return a proxy's ptr.
> 
> In that mega-patch where we had all those clean-shutdown issues, Jim and I
> talked a lot about passing around proxies in our code and came to the
conclusion
> that if you pass the full proxy, it either means the caller must take some
> responsibility over its lifetime (like needing to call close()) or more
> practically speaking it leaves it ambiguous to the caller whether they're
> supposed to. However, to make it clear that the caller is not responsible for
> the lifetime because the vending class controls it, an api can pass only the
> pointer. This makes it obvious that the caller caller isn't responsible for
> closing because it doesn't have access to the lifetime related methods.
> Thoughts?

I'd prefer that the type of Proxy references be FooProxy and Stub references
be  FooStub. That's a rather important distinction; not worth losing for
the convenience of leaving out ".ptr" when referring to a proxy method.

I agree that, by default, the lifetime of Mojo resources be
the responsibility of the code that created them, not whomever
uses them last. I think that the problem we have with managing lifetimes
is not errant closing of shared resources, it's never closing them.


> 
> > I'd had a different pending change -
https://codereview.chromium.org/10473630
> 
> So this patch fixed that type bug in the same way (because I just happened to
> notice it), but it was orthogonal to the API change intended in my patch.