Add blink::ServiceRegistry and expose it from LocalFrame and Platform.

Add blink::ServiceRegistry and expose it from LocalFrame and Platform.

BUG=596559
Committed: https://crrev.com/bebeb608346c93d77f496c47cf0df62da80c517e
Cr-Commit-Position: refs/heads/master@{#385388}

[Sam McNally, 2016-03-24 05:04:55]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-03-24 05:05:05]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-03-29 03:02:32]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-03-29 03:02:45]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-03-29 04:30:55]

sammc@chromium.org changed reviewers:
+ esprehn@chromium.org, rockot@chromium.org

[dcheng, 2016-03-29 04:51:55]

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

[dcheng, 2016-03-29 04:51:56]

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLocalFrame.h:35: BLINK_EXPORT static
WebLocalFrame* create(WebTreeScopeType, WebFrameClient*, WebFrame* opener =
nullptr, ServiceRegistry* = nullptr);
Hmm. Would it make more sense to have a WebFrameClient::serviceRegistry() method
instead? Then you don't need to plumb this pointer everywhere in
WebLocalFrameImpl and WebRemoteFrameImpl.

[esprehn, 2016-03-29 05:26:20]

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
File content/renderer/mojo/blink_service_registry_wrapper.cc (right):

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
content/renderer/mojo/blink_service_registry_wrapper.cc:12: #include
"device/battery/battery_monitor.mojom.h"
this is a really bad layering violation that the core content/ service registry
knows about battery_monitor.

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
content/renderer/mojo/blink_service_registry_wrapper.cc:35:
base::StringPiece(name) == device::BatteryMonitor::Name_) {
woah, this is a crazy hack. We really should fix this asap. Also this used to be
guarded with !OS(ANDROID) but you removed it. Is that right?

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
File content/renderer/mojo/blink_service_registry_wrapper.h (right):

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
content/renderer/mojo/blink_service_registry_wrapper.h:18: class
BlinkServiceRegistryWrapper : public blink::ServiceRegistry {
BlinkServiceRegistryImpl

that matches other platform Impl things in content/

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:255: RenderThread::Get() ? new
BlinkServiceRegistryWrapper(
There's a RenderThreadImpl::current() check inside new
BlinkServiceRegistryWrapper, but we check RenderThread::Get() here, how can
::current() be null but RenderThread::Get() not be?

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy.cc
(right):

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy.cc:24: if
(!Platform::current()->serviceRegistry())
how do we get here with a null registry? I think that should be a fatal error.
The old code certainly assumed it wasn't possible.

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/ServiceRegistry.h (right):

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/ServiceRegistry.h:15: virtual void
connectToRemoteService(const char* name, mojo::ScopedMessagePipeHandle) {}
= 0 ?

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLocalFrame.h:35: BLINK_EXPORT static
WebLocalFrame* create(WebTreeScopeType, WebFrameClient*, WebFrame* opener =
nullptr, ServiceRegistry* = nullptr);
On 2016/03/29 at 04:51:56, dcheng wrote:
> Hmm. Would it make more sense to have a WebFrameClient::serviceRegistry()
method instead? Then you don't need to plumb this pointer everywhere in
WebLocalFrameImpl and WebRemoteFrameImpl.

In general I want WebFrameClient to die, it's a god class that knows about
everything everywhere. Instead we should plumb the right dependencies to the
right layers. That said, this might make more sense there for now.

[Sam McNally, 2016-03-30 00:20:53]

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
File content/renderer/mojo/blink_service_registry_wrapper.cc (right):

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
content/renderer/mojo/blink_service_registry_wrapper.cc:35:
base::StringPiece(name) == device::BatteryMonitor::Name_) {
On 2016/03/29 05:26:20, esprehn wrote:
> woah, this is a crazy hack. We really should fix this asap. Also this used to
be
> guarded with !OS(ANDROID) but you removed it. Is that right?

I'm going to change battery status to avoid this:
https://codereview.chromium.org/1836203002/.

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
File content/renderer/mojo/blink_service_registry_wrapper.h (right):

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/mojo/b...
content/renderer/mojo/blink_service_registry_wrapper.h:18: class
BlinkServiceRegistryWrapper : public blink::ServiceRegistry {
On 2016/03/29 05:26:20, esprehn wrote:
> BlinkServiceRegistryImpl
> 
> that matches other platform Impl things in content/

Done.

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1830883002/diff/80001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:255: RenderThread::Get() ? new
BlinkServiceRegistryWrapper(
On 2016/03/29 05:26:20, esprehn wrote:
> There's a RenderThreadImpl::current() check inside new
> BlinkServiceRegistryWrapper, but we check RenderThread::Get() here, how can
> ::current() be null but RenderThread::Get() not be?

If a MockRenderThread is used, RenderThread::Get() will return it, but
RenderThreadImpl::current() will return nullptr.

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy.cc
(right):

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy.cc:24: if
(!Platform::current()->serviceRegistry())
On 2016/03/29 05:26:20, esprehn wrote:
> how do we get here with a null registry? I think that should be a fatal error.
> The old code certainly assumed it wasn't possible.

The equivalent of a null registry here would have been a no-op
Platform::connectToRemoteService() implementation.

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/ServiceRegistry.h (right):

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/ServiceRegistry.h:15: virtual void
connectToRemoteService(const char* name, mojo::ScopedMessagePipeHandle) {}
On 2016/03/29 05:26:20, esprehn wrote:
> = 0 ?

Done.

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1830883002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLocalFrame.h:35: BLINK_EXPORT static
WebLocalFrame* create(WebTreeScopeType, WebFrameClient*, WebFrame* opener =
nullptr, ServiceRegistry* = nullptr);
On 2016/03/29 05:26:20, esprehn wrote:
> On 2016/03/29 at 04:51:56, dcheng wrote:
> > Hmm. Would it make more sense to have a WebFrameClient::serviceRegistry()
> method instead? Then you don't need to plumb this pointer everywhere in
> WebLocalFrameImpl and WebRemoteFrameImpl.
> 
> In general I want WebFrameClient to die, it's a god class that knows about
> everything everywhere. Instead we should plumb the right dependencies to the
> right layers. That said, this might make more sense there for now.

Done.

[esprehn, 2016-03-31 05:10:43]

lgtm w/ a comment. If dcheng@ and rockot@ are happy lets do it. :)

https://codereview.chromium.org/1830883002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp (right):

https://codereview.chromium.org/1830883002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp:57: if
(!Platform::current()->serviceRegistry())
So one thing we could do is have a static EmptyServiceRegistry and make platform
return that by default. Then you're assured it's never null.

[dcheng, 2016-03-31 05:34:26]

https://codereview.chromium.org/1830883002/diff/180001/content/renderer/mojo/...
File content/renderer/mojo/blink_service_registry_impl.cc (right):

https://codereview.chromium.org/1830883002/diff/180001/content/renderer/mojo/...
content/renderer/mojo/blink_service_registry_impl.cc:16:
static_cast<ServiceRegistryImpl*>(service_registry)->GetWeakPtr()) {}
Why not just take a ServiceRegistryImpl?

https://codereview.chromium.org/1830883002/diff/180001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1830883002/diff/180001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:255: RenderThread::Get() ? new
BlinkServiceRegistryImpl(
Is this "null for testing" check still needed?

[Sam McNally, 2016-03-31 09:26:27]

https://codereview.chromium.org/1830883002/diff/180001/content/renderer/mojo/...
File content/renderer/mojo/blink_service_registry_impl.cc (right):

https://codereview.chromium.org/1830883002/diff/180001/content/renderer/mojo/...
content/renderer/mojo/blink_service_registry_impl.cc:16:
static_cast<ServiceRegistryImpl*>(service_registry)->GetWeakPtr()) {}
On 2016/03/31 05:34:26, dcheng wrote:
> Why not just take a ServiceRegistryImpl?

Changed it to take a WeakPtr<ServiceRegistry> instead.

https://codereview.chromium.org/1830883002/diff/180001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1830883002/diff/180001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:255: RenderThread::Get() ? new
BlinkServiceRegistryImpl(
On 2016/03/31 05:34:26, dcheng wrote:
> Is this "null for testing" check still needed?

Refactored to avoid needing to deal with this here. It's only in RenderViewTest
where this comes up.

https://codereview.chromium.org/1830883002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp (right):

https://codereview.chromium.org/1830883002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp:57: if
(!Platform::current()->serviceRegistry())
On 2016/03/31 05:10:43, esprehn wrote:
> So one thing we could do is have a static EmptyServiceRegistry and make
platform
> return that by default. Then you're assured it's never null.

Done.

[dcheng, 2016-03-31 20:16:08]

lgtm

https://codereview.chromium.org/1830883002/diff/240001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1830883002/diff/240001/content/public/test/re...
content/public/test/render_view_test.cc:175: :
RendererBlinkPlatformImpl(scheduler, base::WeakPtr<ServiceRegistry>()) {
Nit: nullptr?

https://codereview.chromium.org/1830883002/diff/240001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1830883002/diff/240001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1235:
static_cast<ServiceRegistryImpl*>(service_registry())->GetWeakPtr()));
Unrelated to this patch, but maybe ChildThreadImpl::service_registry() should
just return a ServiceRegistryImp, downcasting is ugly.

[esprehn, 2016-04-04 17:04:45]

Can we land this now? :D

[Sam McNally, 2016-04-05 02:34:50]

sammc@chromium.org changed reviewers:
+ rouslan@chromium.org, sky@chromium.org, timvolodine@chromium.org

[Sam McNally, 2016-04-05 02:34:51]

+timvolodine for third_party/WebKit/Source/modules/battery/

+rouslan for third_party/WebKit/Source/modules/payments/

+sky for content/public/test/

https://codereview.chromium.org/1830883002/diff/240001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1830883002/diff/240001/content/public/test/re...
content/public/test/render_view_test.cc:175: :
RendererBlinkPlatformImpl(scheduler, base::WeakPtr<ServiceRegistry>()) {
On 2016/03/31 20:16:08, dcheng wrote:
> Nit: nullptr?

WeakPtr doesn't have a nullptr_t constructor.

[please use gerrit instead, 2016-04-05 02:47:12]

third_party/WebKit/Source/modules/payments/PaymentRequest.cpp lgtm

[sky, 2016-04-05 16:54:13]

LGTM

https://codereview.chromium.org/1830883002/diff/340001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1830883002/diff/340001/content/public/test/re...
content/public/test/render_view_test.cc:175: :
RendererBlinkPlatformImpl(scheduler, base::WeakPtr<ServiceRegistry>()) {
I'm surprised nullptr doesn't work here.

[timvolodine, 2016-04-05 16:58:04]

modules/battery -- lgtm

https://codereview.chromium.org/1830883002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp (right):

https://codereview.chromium.org/1830883002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp:59: // m_monitor
can be null during testing.
not really related to this path: I was wondering if this is still true after the
recent patches, i.e. that m_monitor can be null during testing..

[esprehn, 2016-04-05 23:49:14]

FYI, no need to upload a new patch.

https://codereview.chromium.org/1830883002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/1830883002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.h:84: static LocalFrame*
create(FrameLoaderClient*, FrameHost*, FrameOwner*, ServiceRegistry*);
You could use a default argument = nullptr to avoid needing to pass this in all
over.

[Sam McNally, 2016-04-06 00:02:42]

Patchset #17 (id:400001) has been deleted

[Sam McNally, 2016-04-06 01:04:22]

Patchset #17 (id:420001) has been deleted

[Sam McNally, 2016-04-06 01:32:53]

https://codereview.chromium.org/1830883002/diff/340001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1830883002/diff/340001/content/public/test/re...
content/public/test/render_view_test.cc:175: :
RendererBlinkPlatformImpl(scheduler, base::WeakPtr<ServiceRegistry>()) {
On 2016/04/05 16:54:13, sky wrote:
> I'm surprised nullptr doesn't work here.

It does now :)

https://codereview.chromium.org/1830883002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp (right):

https://codereview.chromium.org/1830883002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp:59: // m_monitor
can be null during testing.
On 2016/04/05 16:58:04, timvolodine wrote:
> not really related to this path: I was wondering if this is still true after
the
> recent patches, i.e. that m_monitor can be null during testing..

I don't think this ever was true. In tests m_monitor never connected to
anything, but it was still non-null. Removed all the checks for that.

https://codereview.chromium.org/1830883002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/1830883002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.h:84: static LocalFrame*
create(FrameLoaderClient*, FrameHost*, FrameOwner*, ServiceRegistry*);
On 2016/04/05 23:49:14, esprehn wrote:
> You could use a default argument = nullptr to avoid needing to pass this in
all
> over.

Done.

[Sam McNally, 2016-04-06 06:33:47]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-04-06 06:33:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, dcheng@chromium.org, sky@chromium.org,
rouslan@chromium.org, timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/1830883002/#ps440001
(title: " ")

[commit-bot: I haz the power, 2016-04-06 06:34:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830883002/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830883002/440001

[commit-bot: I haz the power, 2016-04-06 06:43:07]

Committed patchset #17 (id:440001)

[commit-bot: I haz the power, 2016-04-06 06:44:35]

Description was changed from

==========
Add blink::ServiceRegistry and expose it from LocalFrame and Platform.

BUG=596559
==========

to

==========
Add blink::ServiceRegistry and expose it from LocalFrame and Platform.

BUG=596559

Committed: https://crrev.com/bebeb608346c93d77f496c47cf0df62da80c517e
Cr-Commit-Position: refs/heads/master@{#385388}
==========

[commit-bot: I haz the power, 2016-04-06 06:44:37]

Patchset 17 (id:??) landed as
https://crrev.com/bebeb608346c93d77f496c47cf0df62da80c517e
Cr-Commit-Position: refs/heads/master@{#385388}