Add a Javascript wrapper around ServiceRegistry and expose it to WebUI.

Add a Javascript wrapper around ServiceRegistry and expose it to WebUI.

This enables moving browser connection setup into each individual WebUI
page implementation and thus allows each WebUI Mojo service to be
registered with the ServiceRegsitry using its individual service name
instead of a shared WebUI controller service name.

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

[darin (slow to review), 2014-06-21 04:44:16]

This seems really nice to me. We should eventually be able to replace
serviceRegistry here with navigator.connect()
[https://github.com/dglazkov/tubes/blob/master/API.md].

sky should probably review this as well.

One question I have is about terminology. Does it make sense for the JS files to
include the "service registry" rather than the "service provider"? It seems like
the latter might be a slightly better name since it speaks to the function
(providing) rather than the implementation details (it's a registry).

https://codereview.chromium.org/302573002/diff/230001/chrome/browser/resource...
File chrome/browser/resources/omnibox/omnibox.js (right):

https://codereview.chromium.org/302573002/diff/230001/chrome/browser/resource...
chrome/browser/resources/omnibox/omnibox.js:437:
serviceRegistry.getInterface(browser.OmniboxUIHandlerMojoProxy.NAME_),
I think it would be nice to avoid having to use the NAME_ variable here. The
trailing "_" indicates a private field.

https://codereview.chromium.org/302573002/diff/230001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/mojo_web_ui_controller.h (right):

https://codereview.chromium.org/302573002/diff/230001/chrome/browser/ui/webui...
chrome/browser/ui/webui/mojo_web_ui_controller.h:62: ->GetServiceRegistry()
nit: line break after operators

https://codereview.chromium.org/302573002/diff/230001/content/browser/webui/w...
File content/browser/webui/web_ui_mojo_browsertest.cc (right):

https://codereview.chromium.org/302573002/diff/230001/content/browser/webui/w...
content/browser/webui/web_ui_mojo_browsertest.cc:137: ->GetServiceRegistry()
ditto

https://codereview.chromium.org/302573002/diff/230001/content/public/common/s...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/302573002/diff/230001/content/public/common/s...
content/public/common/service_registry.h:25: : public
base::SupportsWeakPtr<ServiceRegistry> {
Please use WeakPtrFactory instead. There was a chromium-dev@ thread some time
ago arguing that we should not be using SupportsWeakPtr anymore.

https://codereview.chromium.org/302573002/diff/230001/content/renderer/servic...
File content/renderer/service_registry_js_wrapper.cc (right):

https://codereview.chromium.org/302573002/diff/230001/content/renderer/servic...
content/renderer/service_registry_js_wrapper.cc:38: .SetMethod("getInterface",
&ServiceRegistryJsWrapper::GetInterface);
nit: "." operator on the previous line

https://codereview.chromium.org/302573002/diff/230001/content/renderer/web_ui...
File content/renderer/web_ui_mojo_context_state.cc (right):

https://codereview.chromium.org/302573002/diff/230001/content/renderer/web_ui...
content/renderer/web_ui_mojo_context_state.cc:74:
->LoadModule(context_holder->isolate(),
ditto

https://codereview.chromium.org/302573002/diff/230001/content/test/data/web_u...
File content/test/data/web_ui_mojo.js (right):

https://codereview.chromium.org/302573002/diff/230001/content/test/data/web_u...
content/test/data/web_ui_mojo.js:29:
serviceRegistry.getInterface(bindings.BrowserTargetProxy.NAME_),
same nit about having to use NAME_ here. maybe there should just be a TODO in
this CL about figuring out a better solution.

[raymes, 2014-06-24 06:29:25]

https://codereview.chromium.org/302573002/diff/250001/content/public/renderer...
File content/public/renderer/service_registry_js.h (right):

https://codereview.chromium.org/302573002/diff/250001/content/public/renderer...
content/public/renderer/service_registry_js.h:20: v8::Isolate* isolate,
ServiceRegistry* service_registry);
nit: newline between params?

[Sam McNally, 2014-06-24 08:45:02]

On 2014/06/21 04:44:16, darin wrote:
> This seems really nice to me. We should eventually be able to replace
> serviceRegistry here with navigator.connect()
> [https://github.com/dglazkov/tubes/blob/master/API.md].
> 
> sky should probably review this as well.
> 
> One question I have is about terminology. Does it make sense for the JS files
to
> include the "service registry" rather than the "service provider"? It seems
like
> the latter might be a slightly better name since it speaks to the function
> (providing) rather than the implementation details (it's a registry).

Done.

https://codereview.chromium.org/302573002/diff/230001/chrome/browser/resource...
File chrome/browser/resources/omnibox/omnibox.js (right):

https://codereview.chromium.org/302573002/diff/230001/chrome/browser/resource...
chrome/browser/resources/omnibox/omnibox.js:437:
serviceRegistry.getInterface(browser.OmniboxUIHandlerMojoProxy.NAME_),
On 2014/06/21 04:44:15, darin wrote:
> I think it would be nice to avoid having to use the NAME_ variable here. The
> trailing "_" indicates a private field.

Added a TODO.

https://codereview.chromium.org/302573002/diff/230001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/mojo_web_ui_controller.h (right):

https://codereview.chromium.org/302573002/diff/230001/chrome/browser/ui/webui...
chrome/browser/ui/webui/mojo_web_ui_controller.h:62: ->GetServiceRegistry()
On 2014/06/21 04:44:15, darin wrote:
> nit: line break after operators

Done.

https://codereview.chromium.org/302573002/diff/230001/content/browser/webui/w...
File content/browser/webui/web_ui_mojo_browsertest.cc (right):

https://codereview.chromium.org/302573002/diff/230001/content/browser/webui/w...
content/browser/webui/web_ui_mojo_browsertest.cc:137: ->GetServiceRegistry()
On 2014/06/21 04:44:15, darin wrote:
> ditto

Done.

https://codereview.chromium.org/302573002/diff/230001/content/public/common/s...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/302573002/diff/230001/content/public/common/s...
content/public/common/service_registry.h:25: : public
base::SupportsWeakPtr<ServiceRegistry> {
On 2014/06/21 04:44:16, darin wrote:
> Please use WeakPtrFactory instead. There was a chromium-dev@ thread some time
> ago arguing that we should not be using SupportsWeakPtr anymore.

Done.

https://codereview.chromium.org/302573002/diff/230001/content/renderer/servic...
File content/renderer/service_registry_js_wrapper.cc (right):

https://codereview.chromium.org/302573002/diff/230001/content/renderer/servic...
content/renderer/service_registry_js_wrapper.cc:38: .SetMethod("getInterface",
&ServiceRegistryJsWrapper::GetInterface);
On 2014/06/21 04:44:16, darin wrote:
> nit: "." operator on the previous line

Done.

https://codereview.chromium.org/302573002/diff/230001/content/renderer/web_ui...
File content/renderer/web_ui_mojo_context_state.cc (right):

https://codereview.chromium.org/302573002/diff/230001/content/renderer/web_ui...
content/renderer/web_ui_mojo_context_state.cc:74:
->LoadModule(context_holder->isolate(),
On 2014/06/21 04:44:16, darin wrote:
> ditto

Done.

https://codereview.chromium.org/302573002/diff/230001/content/test/data/web_u...
File content/test/data/web_ui_mojo.js (right):

https://codereview.chromium.org/302573002/diff/230001/content/test/data/web_u...
content/test/data/web_ui_mojo.js:29:
serviceRegistry.getInterface(bindings.BrowserTargetProxy.NAME_),
On 2014/06/21 04:44:16, darin wrote:
> same nit about having to use NAME_ here. maybe there should just be a TODO in
> this CL about figuring out a better solution.

Done.

https://codereview.chromium.org/302573002/diff/250001/content/public/renderer...
File content/public/renderer/service_registry_js.h (right):

https://codereview.chromium.org/302573002/diff/250001/content/public/renderer...
content/public/renderer/service_registry_js.h:20: v8::Isolate* isolate,
ServiceRegistry* service_registry);
On 2014/06/24 06:29:24, raymes wrote:
> nit: newline between params?

Done.

[Sam McNally, 2014-06-26 01:51:25]

@sky: ping

[sky, 2014-06-26 17:03:53]

This looks fine to me. Don't you need to update the test js file though?
content/test/data/web_ui_mojo.js?

https://codereview.chromium.org/302573002/diff/370001/content/renderer/web_ui...
File content/renderer/web_ui_mojo_context_state.h (right):

https://codereview.chromium.org/302573002/diff/370001/content/renderer/web_ui...
content/renderer/web_ui_mojo_context_state.h:14: #include
"mojo/public/cpp/system/core.h"
nit: I think you can remove this include now.

[Sam McNally, 2014-06-26 23:43:12]

On 2014/06/26 17:03:53, sky wrote:
> This looks fine to me. Don't you need to update the test js file though?
> content/test/data/web_ui_mojo.js?

https://codereview.chromium.org/302573002/diff/390001/content/test/data/web_u...

https://codereview.chromium.org/302573002/diff/370001/content/renderer/web_ui...
File content/renderer/web_ui_mojo_context_state.h (right):

https://codereview.chromium.org/302573002/diff/370001/content/renderer/web_ui...
content/renderer/web_ui_mojo_context_state.h:14: #include
"mojo/public/cpp/system/core.h"
On 2014/06/26 17:03:52, sky wrote:
> nit: I think you can remove this include now.

Done.

[sky, 2014-06-26 23:50:53]

I think you forgot, are you blind!?!

LGTM

[Sam McNally, 2014-06-27 03:22:19]

darin: Can I get OWNERS approval for content/?

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

https://codereview.chromium.org/302573002/diff/390001/content/browser/webui/w...
File content/browser/webui/web_ui_mojo_browsertest.cc (right):

https://codereview.chromium.org/302573002/diff/390001/content/browser/webui/w...
content/browser/webui/web_ui_mojo_browsertest.cc:65:
renderer_.Bind(request.PassMessagePipe());
It is kind of awkward to be connecting an InterfaceRequest<BrowserTarget> to a
InterfacePtr<RendererTarget>. This use of PassMessagePipe() loses type
information, which makes this code potentially a bit fragile. I wonder if there
is a way to change BrowserTargetImpl to instead receive a RendererTargetPtr or
if BrowserTargetImpl should instead be an InterfaceImpl<BrowserTarget>? The
latter seems right given how ServiceRegistry works.

https://codereview.chromium.org/302573002/diff/390001/content/public/common/s...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/302573002/diff/390001/content/public/common/s...
content/public/common/service_registry.h:62: virtual
base::WeakPtr<ServiceRegistry> GetWeakPtr() = 0;
hmm, it seems a bit uncommon to expose a WeakPtr to an interface like this. i
wonder if that suggests an anti-pattern. hmm...

https://codereview.chromium.org/302573002/diff/390001/content/public/renderer...
File content/public/renderer/service_registry_js.h (right):

https://codereview.chromium.org/302573002/diff/390001/content/public/renderer...
content/public/renderer/service_registry_js.h:19: CONTENT_EXPORT
v8::Handle<v8::Value> WrapServiceRegistry(
Why does this file need to be in content/public/renderer/? It looks like this
method and the constant only used from within content/renderer/.

Perhaps you do not need to expose the GetWeakPtr() method as well if we can
assume that ServiceRegistry's implementation is ServiceRegistryImpl?

https://codereview.chromium.org/302573002/diff/390001/content/renderer/servic...
File content/renderer/service_registry_js_wrapper.h (right):

https://codereview.chromium.org/302573002/diff/390001/content/renderer/servic...
content/renderer/service_registry_js_wrapper.h:6: #define
CONTENT_RENDERER_SERVICE_REGISTRY_JS_WRAPPER_H_
should this be in content/renderer/mojo/ ?

https://codereview.chromium.org/302573002/diff/390001/content/test/data/web_u...
File content/test/data/web_ui_mojo.js (right):

https://codereview.chromium.org/302573002/diff/390001/content/test/data/web_u...
content/test/data/web_ui_mojo.js:30:
serviceProvider.connectToService(bindings.BrowserTargetProxy.NAME_),
Hmm, it is odd that the NAME_ variable is on BrowserTargetProxy rather than
BrowserTarget.

[Sam McNally, 2014-06-30 01:26:14]

https://codereview.chromium.org/302573002/diff/390001/content/browser/webui/w...
File content/browser/webui/web_ui_mojo_browsertest.cc (right):

https://codereview.chromium.org/302573002/diff/390001/content/browser/webui/w...
content/browser/webui/web_ui_mojo_browsertest.cc:65:
renderer_.Bind(request.PassMessagePipe());
On 2014/06/28 21:10:38, darin wrote:
> It is kind of awkward to be connecting an InterfaceRequest<BrowserTarget> to a
> InterfacePtr<RendererTarget>. This use of PassMessagePipe() loses type
> information, which makes this code potentially a bit fragile. I wonder if
there
> is a way to change BrowserTargetImpl to instead receive a RendererTargetPtr or
> if BrowserTargetImpl should instead be an InterfaceImpl<BrowserTarget>? The
> latter seems right given how ServiceRegistry works.

Done.

https://codereview.chromium.org/302573002/diff/390001/content/public/common/s...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/302573002/diff/390001/content/public/common/s...
content/public/common/service_registry.h:62: virtual
base::WeakPtr<ServiceRegistry> GetWeakPtr() = 0;
On 2014/06/28 21:10:38, darin wrote:
> hmm, it seems a bit uncommon to expose a WeakPtr to an interface like this. i
> wonder if that suggests an anti-pattern. hmm...

Removed it.

https://codereview.chromium.org/302573002/diff/390001/content/public/renderer...
File content/public/renderer/service_registry_js.h (right):

https://codereview.chromium.org/302573002/diff/390001/content/public/renderer...
content/public/renderer/service_registry_js.h:19: CONTENT_EXPORT
v8::Handle<v8::Value> WrapServiceRegistry(
On 2014/06/28 21:10:38, darin wrote:
> Why does this file need to be in content/public/renderer/? It looks like this
> method and the constant only used from within content/renderer/.
> 
> Perhaps you do not need to expose the GetWeakPtr() method as well if we can
> assume that ServiceRegistry's implementation is ServiceRegistryImpl?

Done.

https://codereview.chromium.org/302573002/diff/390001/content/renderer/servic...
File content/renderer/service_registry_js_wrapper.h (right):

https://codereview.chromium.org/302573002/diff/390001/content/renderer/servic...
content/renderer/service_registry_js_wrapper.h:6: #define
CONTENT_RENDERER_SERVICE_REGISTRY_JS_WRAPPER_H_
On 2014/06/28 21:10:38, darin wrote:
> should this be in content/renderer/mojo/ ?

Done.

https://codereview.chromium.org/302573002/diff/390001/content/test/data/web_u...
File content/test/data/web_ui_mojo.js (right):

https://codereview.chromium.org/302573002/diff/390001/content/test/data/web_u...
content/test/data/web_ui_mojo.js:30:
serviceProvider.connectToService(bindings.BrowserTargetProxy.NAME_),
On 2014/06/28 21:10:38, darin wrote:
> Hmm, it is odd that the NAME_ variable is on BrowserTargetProxy rather than
> BrowserTarget.

BrowserTarget doesn't actually exist in the current bindings - there's only
BrowserTargetProxy and BrowserTargetStub.

[Sam McNally, 2014-07-09 00:24:12]

darin: ping

[Sam McNally, 2014-07-18 08:29:24]

darin: ping

[Sam McNally, 2014-07-24 08:47:07]

darin: ping

[Sam McNally, 2014-07-29 11:34:17]

ping

[darin (slow to review), 2014-07-29 16:22:05]

LGTM with just a couple concerns:

https://codereview.chromium.org/302573002/diff/490001/content/browser/webui/w...
File content/browser/webui/web_ui_mojo_browsertest.cc (right):

https://codereview.chromium.org/302573002/diff/490001/content/browser/webui/w...
content/browser/webui/web_ui_mojo_browsertest.cc:72: virtual void
OnConnectionError() OVERRIDE {}
hmm, why bother overriding this? the default implementation on InterfaceImpl
doesn't do anything either.

https://codereview.chromium.org/302573002/diff/490001/content/browser/webui/w...
content/browser/webui/web_ui_mojo_browsertest.cc:141: mojo::BindToRequest(new
PingBrowserTargetImpl(run_loop_), &request));
did you want to use WeakBindToRequest here? BindToRequest takes an owning
reference to the instance and browser_target_ also takes an owning reference to
it.

[Sam McNally, 2014-07-30 00:18:08]

https://codereview.chromium.org/302573002/diff/490001/content/browser/webui/w...
File content/browser/webui/web_ui_mojo_browsertest.cc (right):

https://codereview.chromium.org/302573002/diff/490001/content/browser/webui/w...
content/browser/webui/web_ui_mojo_browsertest.cc:72: virtual void
OnConnectionError() OVERRIDE {}
On 2014/07/29 16:22:05, darin wrote:
> hmm, why bother overriding this? the default implementation on InterfaceImpl
> doesn't do anything either.

Done.

https://codereview.chromium.org/302573002/diff/490001/content/browser/webui/w...
content/browser/webui/web_ui_mojo_browsertest.cc:141: mojo::BindToRequest(new
PingBrowserTargetImpl(run_loop_), &request));
On 2014/07/29 16:22:05, darin wrote:
> did you want to use WeakBindToRequest here? BindToRequest takes an owning
> reference to the instance and browser_target_ also takes an owning reference
to
> it.

Done.

[Sam McNally, 2014-07-30 00:18:23]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-07-30 00:21:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/302573002/550001

[commit-bot: I haz the power, 2014-07-30 09:54:19]

Change committed as 286459