Pass ServiceProvider thru ViewManagerService::Embed() allowing embedder & embeddee to expose servic…

Pass ServiceProvider thru ViewManagerService::Embed() allowing embedder & embeddee to expose services to each other alongside the Embed action.

R=darin@chromium.org,sky@chromium.org
BUG=none

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

[Ben Goodger (Google), 2014-07-31 20:31:01]

darin: core bits in:

- mojo/examples/wm_flow/*
- mojo/services/public/cpp/view_manager
- mojo/services/view_manager

sky: all

[Ben Goodger (Google), 2014-07-31 20:31:51]

On 2014/07/31 20:31:01, Ben Goodger (Google) wrote:
> darin: core bits in:
> 
> - mojo/examples/wm_flow/*
> - mojo/services/public/cpp/view_manager
> - mojo/services/view_manager
> 
> sky: all

oh and darin: mojo/public/cpp/application

[sky, 2014-07-31 21:14:11]

LGTM

https://codereview.chromium.org/433513005/diff/1/mojo/examples/wm_flow/app/ap...
File mojo/examples/wm_flow/app/app.cc (right):

https://codereview.chromium.org/433513005/diff/1/mojo/examples/wm_flow/app/ap...
mojo/examples/wm_flow/app/app.cc:64: //init_svc->Embed("mojo:mojo_wm_flow_app",
base::Bind(&ConnectCallback));
nit: remove early attempts.

https://codereview.chromium.org/433513005/diff/1/mojo/services/public/cpp/vie...
File mojo/services/public/cpp/view_manager/lib/view_manager_client_impl.cc
(right):

https://codereview.chromium.org/433513005/diff/1/mojo/services/public/cpp/vie...
mojo/services/public/cpp/view_manager/lib/view_manager_client_impl.cc:366:
return;
Do we ever really hit this?

https://codereview.chromium.org/433513005/diff/1/mojo/services/public/cpp/vie...
File mojo/services/public/cpp/view_manager/view_manager_delegate.h (right):

https://codereview.chromium.org/433513005/diff/1/mojo/services/public/cpp/vie...
mojo/services/public/cpp/view_manager/view_manager_delegate.h:28: // |registry|
is an object that the delegate can add services to to expose
Update your comment, registry and remote_services aren't parameters.

https://codereview.chromium.org/433513005/diff/1/mojo/services/public/interfa...
File mojo/services/public/interfaces/view_manager/view_manager.mojom (right):

https://codereview.chromium.org/433513005/diff/1/mojo/services/public/interfa...
mojo/services/public/interfaces/view_manager/view_manager.mojom:37: Embed(string
url, ServiceProvider service_provider) => (bool success);
Add comment about see below Embed() below for details of service_provider?

[Ben Goodger (Google), 2014-08-06 18:25:51]

+davemoore for service providery stuff too.

[DaveMoore, 2014-08-06 21:34:59]

lgtm

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

https://codereview.chromium.org/433513005/diff/1/mojo/public/cpp/application/...
mojo/public/cpp/application/exported_service_registry.h:15: 
Nit: Please provide a comment. On quick inspection it looks like the type is
pointless.

[darin (slow to review), 2014-08-06 22:13:29]

some thoughts on the class design:

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

https://codereview.chromium.org/433513005/diff/1/mojo/public/cpp/application/...
mojo/public/cpp/application/exported_service_registry.h:16: class
RemoteServiceProvider : public ServiceProvider {
nit: break this class out into its own .h and .cc files

https://codereview.chromium.org/433513005/diff/1/mojo/public/cpp/application/...
mojo/public/cpp/application/exported_service_registry.h:18: explicit
RemoteServiceProvider(ServiceProvider* service_provider)
technically, this is more of a ServiceProvider thunk or weakly bound
ServiceProvider. I wonder if its name shouldn't be based on that.
WeakServiceProvider or ServiceProviderThunk.

https://codereview.chromium.org/433513005/diff/1/mojo/public/cpp/application/...
mojo/public/cpp/application/exported_service_registry.h:39: class
ExportedServiceRegistry : public InterfaceImpl<ServiceProvider> {
nit: an Impl suffix might be nice here just to preserve the notion of it being
an interface impl. also not sure about Registry versus Provider here. why not
just call this ServiceProviderImpl or ExportedServiceProviderImpl?

https://codereview.chromium.org/433513005/diff/1/mojo/public/cpp/application/...
mojo/public/cpp/application/exported_service_registry.h:50: void
set_remote(RemoteServiceProvider* remote) { remote_ = remote; }
hmm, it's not clear what the remote service provider has to do with this service
provider impl.

instead of the user of this class having to allocate and set a RemoteSP on the
instance, wouldn't it make more sense for the ExportedServiceRegistry to vend a
RemoteSP pointer? then, you might even completely hide the implementation
details of RemoteServiceProvider.

https://codereview.chromium.org/433513005/diff/1/mojo/public/cpp/application/...
mojo/public/cpp/application/exported_service_registry.h:63: if (remote_)
nit:

if (remote_) {
  remote_->Clear();
  remote_ = NULL;
}

[darin (slow to review), 2014-08-07 16:53:04]

LGTM

https://codereview.chromium.org/433513005/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/remote_service_provider.h (right):

https://codereview.chromium.org/433513005/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/remote_service_provider.h:18: class
RemoteServiceProvider : public ServiceProvider {
nit: I might still call this WeakServiceProvider. Just look at the first
sentence of the comment.

https://codereview.chromium.org/433513005/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/view_manager_delegate.h (right):

https://codereview.chromium.org/433513005/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/view_manager_delegate.h:37:
scoped_ptr<ServiceProvider> imported_services) {}
you didn't want to force people to implement this?

[Ben Goodger (Google), 2014-08-07 17:57:22]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2014-08-07 18:09:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ben@chromium.org/433513005/60001

[commit-bot: I haz the power, 2014-08-07 22:02:07]

Change committed as 288148