Refactor Blink's ServiceConnector and add ability to mock in layout tests

Refactor Blink's ServiceConnector and add ability to mock in layout tests

This CL refactors the implementation of Blink's ServiceConnector so that
layout tests can mock remote interfaces of arbitrary services in JS.
Specifically, it changes the organization to be roughly parallel to
Platform::interfaceProvider().

- Blink has a Connector interface.
- Platform has a connector() method.
- //content/renderer has a BlinkConnectorImpl backed directly by
  //content/renderer's Connector.
- BlinkConnectorImpl also has the ability to override interfaces with local
  implementations, similar to service_manager::InterfaceProvider.
- We add a JS wrapper of BlinkConnectorImpl to expose that ability to JS.

Review-Url: https://codereview.chromium.org/2643063002
Cr-Commit-Position: refs/heads/master@{#459064}
Committed: https://chromium.googlesource.com/chromium/src/+/43dd7b3dd9c675d5f9df67feae7662ee196517fa

[blundell, 2017-01-20 09:59:19]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 09:59:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 10:03:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 10:03:04]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[blundell, 2017-01-20 10:13:44]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 10:14:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 10:26:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 10:26:34]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[blundell, 2017-01-20 11:43:13]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 11:43:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[blundell, 2017-01-20 12:47:18]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 12:47:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 14:23:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 14:23:44]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-01-20 14:40:34]

blundell@chromium.org changed reviewers:
+ esprehn@chromium.org

[blundell, 2017-01-20 14:40:36]

esprehn@: For review (whole CL).

haraken@, dcheng@, rockot@, Ke He: FYI.

I verified on a followup patch (https://codereview.chromium.org/2643713002/)
that the mocking via JS works. I'm also happy to separate out the JS wrapper
into its own patch...I left it in this CL at least for now so that you can see
the whole picture.

[blundell, 2017-01-24 11:37:40]

Friendly ping.

[Ken Rockot(use gerrit already), 2017-01-25 16:37:58]

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

[Ken Rockot(use gerrit already), 2017-01-25 16:37:59]

Ping? It would be nice to get this CL landed.

[esprehn, 2017-02-01 06:02:21]

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

[esprehn, 2017-02-01 06:02:23]

So the main thread specific stuff I see all over here is a big red flag. In
general though I think we're trying to stop having this virtual interface, I
think the plan is to pass the raw mojo connection into blink and then rebind it
to our own stuff internally.

I also don't understand why this thing is pulling in generated JS and evaling it
directly. I don't think we want that.

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
File content/renderer/mojo/blink_connector_impl.cc (right):

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:35:
base::Bind(&BlinkConnectorImpl::getInterface, GetWeakPtr(),
Woah, this round trips through main to connect to a service? Why would a worker
thread want to do that?

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:44: auto it =
overrides_for_service->find(std::string(interface_name));
this should implicitly construct, no need to write std::string here.

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
File content/renderer/mojo/blink_connector_impl.h (right):

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.h:64:
scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
Hmm, what scheduler queue is this?

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
File content/renderer/mojo/blink_connector_js_wrapper.cc (right):

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_js_wrapper.cc:36:
.SetMethod("getInterface", &BlinkConnectorJsWrapper::GetInterface)
Can we just do all of this inside blink instead?

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_js_wrapper.cc:92:
blink::WebLocalFrame::frameForContext(context)
Does this work inside workers? This looks suspicious to me. I don't understand
why we need to inject JS directly like this. The tests should script src the
things they need.

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2647: registry->AddBuiltinModule(
This is exposing this new feature to all chrome:// urls, is that on purpose?

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

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:267:
blink_connector_->SetConnector(RenderThreadImpl::current()
Lets just do this inside blink itself. Pass the ServiceManager connection into
blink and implement all of the machinery down there.

[dcheng, 2017-02-01 07:45:16]

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
File content/renderer/mojo/blink_connector_impl.cc (right):

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:29: if (!connector_.get())
Nit: no .get(). Also... it would be best to just fix the tests so we don't have
to null check like this.

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:35:
base::Bind(&BlinkConnectorImpl::getInterface, GetWeakPtr(),
On 2017/02/01 06:02:23, esprehn wrote:
> Woah, this round trips through main to connect to a service? Why would a
worker
> thread want to do that?

FWIW, this already happens today in
https://cs.chromium.org/chromium/src/content/renderer/mojo/blink_interface_pr....
for that class, it's because we hold a weak pointer to the interface provider.

(It's not clear to me that service_manager::InterfaceProvider is thread-safe
either)

[dcheng, 2017-02-01 07:46:06]

Sorry for the delay on this review =(

I'll mostly defer to esprehn@ since he's already looked, but two random things I
noticed.

[Ken Rockot(use gerrit already), 2017-02-01 19:16:00]

We certainly want to stick to using mojom-generated bindings for Service Manager
APIs, which means using mojom-generated JS.

Maybe we need a better way of exposing generated resources to tests so they can
in fact script src them, but that's not something we have today AFAICT. This CL
seems to follow all the existing patterns used for InterfaceProvider API
exposure for better or worse.

Are you proposing that we first change the way InterfaceProvider is exposed and
then follow the new pattern for Connector?

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
File content/renderer/mojo/blink_connector_impl.cc (right):

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:35:
base::Bind(&BlinkConnectorImpl::getInterface, GetWeakPtr(),
On 2017/02/01 at 07:45:16, dcheng wrote:
> On 2017/02/01 06:02:23, esprehn wrote:
> > Woah, this round trips through main to connect to a service? Why would a
worker
> > thread want to do that?
> 
> FWIW, this already happens today in
https://cs.chromium.org/chromium/src/content/renderer/mojo/blink_interface_pr....
for that class, it's because we hold a weak pointer to the interface provider.
> 
> (It's not clear to me that service_manager::InterfaceProvider is thread-safe
either)

Correct, we already tolerate this behavior. It's not ideal, but doesn't seem
like a deal-breaker.

Unlike InterfaceProvider, we could solve this for Connector today by taking
advantage of the Clone() interface, which essentially allows you to bind
arbitrary new ConnectorPtrs to the same impl.

That is more complexity for this CL though and I think it's reasonable to land
that change later.

https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:35:
base::Bind(&BlinkConnectorImpl::getInterface, GetWeakPtr(),
On 2017/02/01 at 07:45:16, dcheng wrote:
> On 2017/02/01 06:02:23, esprehn wrote:
> > Woah, this round trips through main to connect to a service? Why would a
worker
> > thread want to do that?
> 
> FWIW, this already happens today in
https://cs.chromium.org/chromium/src/content/renderer/mojo/blink_interface_pr....
for that class, it's because we hold a weak pointer to the interface provider.
> 
> (It's not clear to me that service_manager::InterfaceProvider is thread-safe
either)

Correct, we already tolerate this behavior. It's not ideal, but doesn't seem
like a deal-breaker.

Unlike InterfaceProvider, we could solve this for Connector today by taking
advantage of the Clone() interface, which essentially allows you to bind
arbitrary new ConnectorPtrs to the same impl, so we could have a thread-local
Connector if we wanted.

That is more complexity for this CL though and I think it's reasonable to land
that change later. This is certainly not currently on a critical path.

[Ken Rockot(use gerrit already), 2017-02-01 19:16:33]

On 2017/02/01 at 19:16:00, Ken Rockot wrote:
> We certainly want to stick to using mojom-generated bindings for Service
Manager APIs, which means using mojom-generated JS.
> 
> Maybe we need a better way of exposing generated resources to tests so they
can in fact script src them, but that's not something we have today AFAICT. This
CL seems to follow all the existing patterns used for InterfaceProvider API
exposure for better or worse.
> 
> Are you proposing that we first change the way InterfaceProvider is exposed
and then follow the new pattern for Connector?
> 
>
https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
> File content/renderer/mojo/blink_connector_impl.cc (right):
> 
>
https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
> content/renderer/mojo/blink_connector_impl.cc:35:
base::Bind(&BlinkConnectorImpl::getInterface, GetWeakPtr(),
> On 2017/02/01 at 07:45:16, dcheng wrote:
> > On 2017/02/01 06:02:23, esprehn wrote:
> > > Woah, this round trips through main to connect to a service? Why would a
worker
> > > thread want to do that?
> > 
> > FWIW, this already happens today in
https://cs.chromium.org/chromium/src/content/renderer/mojo/blink_interface_pr....
for that class, it's because we hold a weak pointer to the interface provider.
> > 
> > (It's not clear to me that service_manager::InterfaceProvider is thread-safe
either)
> 
> Correct, we already tolerate this behavior. It's not ideal, but doesn't seem
like a deal-breaker.
> 
> Unlike InterfaceProvider, we could solve this for Connector today by taking
advantage of the Clone() interface, which essentially allows you to bind
arbitrary new ConnectorPtrs to the same impl.
> 
> That is more complexity for this CL though and I think it's reasonable to land
that change later.
> 
>
https://codereview.chromium.org/2643063002/diff/160001/content/renderer/mojo/...
> content/renderer/mojo/blink_connector_impl.cc:35:
base::Bind(&BlinkConnectorImpl::getInterface, GetWeakPtr(),
> On 2017/02/01 at 07:45:16, dcheng wrote:
> > On 2017/02/01 06:02:23, esprehn wrote:
> > > Woah, this round trips through main to connect to a service? Why would a
worker
> > > thread want to do that?
> > 
> > FWIW, this already happens today in
https://cs.chromium.org/chromium/src/content/renderer/mojo/blink_interface_pr....
for that class, it's because we hold a weak pointer to the interface provider.
> > 
> > (It's not clear to me that service_manager::InterfaceProvider is thread-safe
either)
> 
> Correct, we already tolerate this behavior. It's not ideal, but doesn't seem
like a deal-breaker.
> 
> Unlike InterfaceProvider, we could solve this for Connector today by taking
advantage of the Clone() interface, which essentially allows you to bind
arbitrary new ConnectorPtrs to the same impl, so we could have a thread-local
Connector if we wanted.
> 
> That is more complexity for this CL though and I think it's reasonable to land
that change later. This is certainly not currently on a critical path.

Wheee, duplicate comments plz ignore

[esprehn, 2017-02-15 02:10:17]

So where are we on this?

[blundell, 2017-02-20 16:10:54]

On 2017/02/15 02:10:17, esprehn wrote:
> So where are we on this?

I'm curious for your response to Ken's question above, as I developed this CL
simply by following the existing interfaceProvider() structure:

"This CL seems to follow all the existing patterns used for InterfaceProvider
API
exposure for better or worse.

Are you proposing that we first change the way InterfaceProvider is exposed and
then follow the new pattern for Connector?"

[blundell, 2017-03-14 12:16:58]

dcheng@: ping :).

[blundell, 2017-03-17 20:48:17]

On 2017/03/14 12:16:58, blundell wrote:
> dcheng@: ping :).

bump :)

[esprehn, 2017-03-17 23:40:13]

esprehn@chromium.org changed reviewers:
- esprehn@chromium.org

[esprehn, 2017-03-20 23:17:32]

esprehn@chromium.org changed reviewers:
+ dglazkov@chromium.org, esprehn@chromium.org

[esprehn, 2017-03-20 23:17:39]

esprehn@chromium.org changed reviewers:
- esprehn@chromium.org

[dglazkov, 2017-03-21 14:42:41]

On 2017/02/20 at 16:10:54, blundell wrote:
> On 2017/02/15 02:10:17, esprehn wrote:
> > So where are we on this?
> 
> I'm curious for your response to Ken's question above, as I developed this CL
simply by following the existing interfaceProvider() structure:
> 
> "This CL seems to follow all the existing patterns used for InterfaceProvider
API
> exposure for better or worse.
> 
> Are you proposing that we first change the way InterfaceProvider is exposed
and
> then follow the new pattern for Connector?"

I think it's okay as a first step. Could you file a bug to keep sight of the
problem under Blink>Internals>Modularization?

[blundell, 2017-03-21 14:52:39]

On 2017/03/21 14:42:41, dglazkov wrote:
> On 2017/02/20 at 16:10:54, blundell wrote:
> > On 2017/02/15 02:10:17, esprehn wrote:
> > > So where are we on this?
> > 
> > I'm curious for your response to Ken's question above, as I developed this
CL
> simply by following the existing interfaceProvider() structure:
> > 
> > "This CL seems to follow all the existing patterns used for
InterfaceProvider
> API
> > exposure for better or worse.
> > 
> > Are you proposing that we first change the way InterfaceProvider is exposed
> and
> > then follow the new pattern for Connector?"
> 
> I think it's okay as a first step. Could you file a bug to keep sight of the
> problem under Blink>Internals>Modularization?

Thanks! Filed crbug.com/703656.

[Ken Rockot(use gerrit already), 2017-03-21 15:09:21]

One final comment now that this has been resurrected: I would prefer we called
the Connector API bindInterface instead of getInterface since this mirrors the
C++ and mojom APIs.

[blundell, 2017-03-21 16:30:20]

Patchset #10 (id:180001) has been deleted

[blundell, 2017-03-21 16:34:20]

blundell@chromium.org changed reviewers:
+ jam@chromium.org

[blundell, 2017-03-21 16:34:21]

s/getInterface/bindInterface done. Also changed a mistaken usage of
std::map::insert() that leon.han@intel.com helpfully pointed out.

Dimitri, PTAL!

+jam@ for //content as well

[blundell, 2017-03-21 16:34:32]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 16:34:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 16:44:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 16:44:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[blundell, 2017-03-21 16:48:28]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 16:49:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dglazkov, 2017-03-21 17:31:04]

On 2017/03/21 at 16:34:21, blundell wrote:
> s/getInterface/bindInterface done. Also changed a mistaken usage of
std::map::insert() that leon.han@intel.com helpfully pointed out.

Thank you for filing a bug! Could you also file one about removing the need to
go to main thread to route getInterface calls?

> 
> Dimitri, PTAL!
> 
> +jam@ for //content as well

[dglazkov, 2017-03-21 17:39:04]

lgtm

[commit-bot: I haz the power, 2017-03-21 18:44:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 18:44:39]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-21 18:56:18]

LGTM

(Sorry for the long review latency)

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/...
File content/renderer/mojo/blink_connector_impl.cc (right):

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:29: if (!connector_.get())
Nit: no .get()

(Also, is this 'null in some testing contexts' something we'd be able to
eventually fix? It seems like we should also be trying to fix the tests so that
we don't have test-only branches)

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:64:
(*(service_binders_[service_name].get()))[interface_name] = binder;
I think this can be simplified slightly to:

(*(service_binders_[service_name]))[interface_name] = binder;

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

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:267: if
(RenderThreadImpl::current()) {
Ah... bah. This issue again...

I guess you can ignore my comment about null for testing for now, since it's
related to the long-standing issue of RenderThreadImpl::current() in tests =/

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.h (right):

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.h:189: blink::Connector*
connector() override;
Optional nit: one alternative to consider is using a covariant return type here.
It means we need the full definition of BlinkConnectorImpl in this header, but
then we can have connector() directly return BlinkConnectorImpl* here,
eliminating the need for the downcast in RenderFrameImpl.

https://codereview.chromium.org/2643063002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/mojo-helpers.js (right):

https://codereview.chromium.org/2643063002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/mojo-helpers.js:45:
connector.clearInterfaceOverridesForTesting();
Do we have any documentation about the differences between these three things?
If I wanted to write a layout test and mock out interfaces, which one of these
things should I be mocking interfaces against?

I guess connector is to let us mock interfaces for things that Blink connects
*to*, while frameInterfaces / interfaces are for mocking interfaces that Blink
implements itself? Do I have this roughly correct? =)

[jam, 2017-03-21 22:02:17]

Can you explain why we have BlinkConnectorImpl which wraps
service_manager::Connector, instead of simply exposing the latter to Blink (and
adding test hooks either in Blink, or in service_manager::Connector)? We're
moving in the direction of having Blink call mojo directly, instead of through
content, so this seems like adding code to content/renderer which we'll have to
remove in the future.

If the concern is about the usage of stl strings, given that it's still
happening but hidden inside content, then it's not a performance issue
(especially since these methods aren't called often). If the concern is about
using std::string from blink, blink won't have to explicitly create any local
std::strings, it can just call
service_manager::Connector::BindInterface(device::mojom::blink::kServiceName,
...) as it does today. Seems like equivalent to what this cl does today, but
with one less interface in the middle. wdyt?

[blundell, 2017-03-22 11:33:15]

On 2017/03/21 22:02:17, jam wrote:
> Can you explain why we have BlinkConnectorImpl which wraps
> service_manager::Connector, instead of simply exposing the latter to Blink
(and
> adding test hooks either in Blink, or in service_manager::Connector)? We're
> moving in the direction of having Blink call mojo directly, instead of through
> content, so this seems like adding code to content/renderer which we'll have
to
> remove in the future.
> 
> If the concern is about the usage of stl strings, given that it's still
> happening but hidden inside content, then it's not a performance issue
> (especially since these methods aren't called often). If the concern is about
> using std::string from blink, blink won't have to explicitly create any local
> std::strings, it can just call
> service_manager::Connector::BindInterface(device::mojom::blink::kServiceName,
> ...) as it does today. Seems like equivalent to what this cl does today, but
> with one less interface in the middle. wdyt?

Hi John,

service_manager::Connector directly and transitively exposes the chromium mojom
variant types, so exposing service_manager::Connector to Blink would be
introducing mixing of the Chromium and Blink mojom variant types in Blink. That
seems like a step that would have to be taken very thoughtfully if at all.

The good news here is that once crbug.com/703656 is fixed, all of this
infrastructure should be internal to Blink, with the only embedder touchpoint
being that the embedder has to bind the connector (as one would expect).
Currently we can only implement the glue between JS and C++ in //content for the
reasons outlined in that bug.

[jam, 2017-03-22 15:15:52]

On 2017/03/22 11:33:15, blundell wrote:
> On 2017/03/21 22:02:17, jam wrote:
> > Can you explain why we have BlinkConnectorImpl which wraps
> > service_manager::Connector, instead of simply exposing the latter to Blink
> (and
> > adding test hooks either in Blink, or in service_manager::Connector)? We're
> > moving in the direction of having Blink call mojo directly, instead of
through
> > content, so this seems like adding code to content/renderer which we'll have
> to
> > remove in the future.
> > 
> > If the concern is about the usage of stl strings, given that it's still
> > happening but hidden inside content, then it's not a performance issue
> > (especially since these methods aren't called often). If the concern is
about
> > using std::string from blink, blink won't have to explicitly create any
local
> > std::strings, it can just call
> >
service_manager::Connector::BindInterface(device::mojom::blink::kServiceName,
> > ...) as it does today. Seems like equivalent to what this cl does today, but
> > with one less interface in the middle. wdyt?
> 
> Hi John,
> 
> service_manager::Connector directly and transitively exposes the chromium
mojom
> variant types, 

Just to make sure I'm following, you're referring to these includes:
#include "services/service_manager/public/interfaces/connector.mojom.h"
#include "services/service_manager/public/interfaces/service.mojom.h"
#include "services/service_manager/public/interfaces/service_manager.mojom.h"
?

> so exposing service_manager::Connector to Blink would be
> introducing mixing of the Chromium and Blink mojom variant types in Blink.
That
> seems like a step that would have to be taken very thoughtfully if at all.

(assuming the answer to my question above is yes)
Is the worry that code would accidentally use service manager mojom directly?
Given that afaik no one uses it directly, even in chromium, and instead go
through the client library, that seems like it wouldn't be likely? Is it
possible to forward declare the objects and not have the includes there?


> 
> The good news here is that once crbug.com/703656 is fixed, all of this
> infrastructure should be internal to Blink, with the only embedder touchpoint
> being that the embedder has to bind the connector (as one would expect).
> Currently we can only implement the glue between JS and C++ in //content for
the
> reasons outlined in that bug.

[Ken Rockot(use gerrit already), 2017-03-22 15:29:22]

On Wed, Mar 22, 2017 at 8:15 AM, <jam@chromium.org> wrote:

> On 2017/03/22 11:33:15, blundell wrote:
> > On 2017/03/21 22:02:17, jam wrote:
> > > Can you explain why we have BlinkConnectorImpl which wraps
> > > service_manager::Connector, instead of simply exposing the latter to
> Blink
> > (and
> > > adding test hooks either in Blink, or in service_manager::Connector)?
> We're
> > > moving in the direction of having Blink call mojo directly, instead of
> through
> > > content, so this seems like adding code to content/renderer which
> we'll have
> > to
> > > remove in the future.
> > >
> > > If the concern is about the usage of stl strings, given that it's still
> > > happening but hidden inside content, then it's not a performance issue
> > > (especially since these methods aren't called often). If the concern is
> about
> > > using std::string from blink, blink won't have to explicitly create any
> local
> > > std::strings, it can just call
> > >
> service_manager::Connector::BindInterface(device::mojom::
> blink::kServiceName,
> > > ...) as it does today. Seems like equivalent to what this cl does
> today, but
> > > with one less interface in the middle. wdyt?
> >
> > Hi John,
> >
> > service_manager::Connector directly and transitively exposes the chromium
> mojom
> > variant types,
>
> Just to make sure I'm following, you're referring to these includes:
> #include "services/service_manager/public/interfaces/connector.mojom.h"
> #include "services/service_manager/public/interfaces/service.mojom.h"
> #include "services/service_manager/public/interfaces/service_
> manager.mojom.h"
> ?
>
> > so exposing service_manager::Connector to Blink would be
> > introducing mixing of the Chromium and Blink mojom variant types in
> Blink.
> That
> > seems like a step that would have to be taken very thoughtfully if at
> all.
>
> (assuming the answer to my question above is yes)
> Is the worry that code would accidentally use service manager mojom
> directly?
> Given that afaik no one uses it directly, even in chromium, and instead go
> through the client library, that seems like it wouldn't be likely? Is it
> possible to forward declare the objects and not have the includes there?
>
>
> >
> > The good news here is that once crbug.com/703656 is fixed, all of this
> > infrastructure should be internal to Blink, with the only embedder
> touchpoint
> > being that the embedder has to bind the connector (as one would expect).
> > Currently we can only implement the glue between JS and C++ in //content
> for
> the
> > reasons outlined in that bug.
>

Hmm I think we'd actually be fine to expose uses of the "chromium" typemap
configuration selectively in Blink. In practice, as a fundamental component
of the system, the service manager's client library (and therefore its
chromium typemap) must maintain strictly minimal dependencies on other
targets (essentially just Mojo and //base). And although the typemap
configuration is itself viral and applied globally, any given bindings
dependency will only pull in the transitive closure of dependencies
required for that bindings target.


>
>
> https://codereview.chromium.org/2643063002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-03-22 15:29:22]

On Wed, Mar 22, 2017 at 8:15 AM, <jam@chromium.org> wrote:

> On 2017/03/22 11:33:15, blundell wrote:
> > On 2017/03/21 22:02:17, jam wrote:
> > > Can you explain why we have BlinkConnectorImpl which wraps
> > > service_manager::Connector, instead of simply exposing the latter to
> Blink
> > (and
> > > adding test hooks either in Blink, or in service_manager::Connector)?
> We're
> > > moving in the direction of having Blink call mojo directly, instead of
> through
> > > content, so this seems like adding code to content/renderer which
> we'll have
> > to
> > > remove in the future.
> > >
> > > If the concern is about the usage of stl strings, given that it's still
> > > happening but hidden inside content, then it's not a performance issue
> > > (especially since these methods aren't called often). If the concern is
> about
> > > using std::string from blink, blink won't have to explicitly create any
> local
> > > std::strings, it can just call
> > >
> service_manager::Connector::BindInterface(device::mojom::
> blink::kServiceName,
> > > ...) as it does today. Seems like equivalent to what this cl does
> today, but
> > > with one less interface in the middle. wdyt?
> >
> > Hi John,
> >
> > service_manager::Connector directly and transitively exposes the chromium
> mojom
> > variant types,
>
> Just to make sure I'm following, you're referring to these includes:
> #include "services/service_manager/public/interfaces/connector.mojom.h"
> #include "services/service_manager/public/interfaces/service.mojom.h"
> #include "services/service_manager/public/interfaces/service_
> manager.mojom.h"
> ?
>
> > so exposing service_manager::Connector to Blink would be
> > introducing mixing of the Chromium and Blink mojom variant types in
> Blink.
> That
> > seems like a step that would have to be taken very thoughtfully if at
> all.
>
> (assuming the answer to my question above is yes)
> Is the worry that code would accidentally use service manager mojom
> directly?
> Given that afaik no one uses it directly, even in chromium, and instead go
> through the client library, that seems like it wouldn't be likely? Is it
> possible to forward declare the objects and not have the includes there?
>
>
> >
> > The good news here is that once crbug.com/703656 is fixed, all of this
> > infrastructure should be internal to Blink, with the only embedder
> touchpoint
> > being that the embedder has to bind the connector (as one would expect).
> > Currently we can only implement the glue between JS and C++ in //content
> for
> the
> > reasons outlined in that bug.
>

Hmm I think we'd actually be fine to expose uses of the "chromium" typemap
configuration selectively in Blink. In practice, as a fundamental component
of the system, the service manager's client library (and therefore its
chromium typemap) must maintain strictly minimal dependencies on other
targets (essentially just Mojo and //base). And although the typemap
configuration is itself viral and applied globally, any given bindings
dependency will only pull in the transitive closure of dependencies
required for that bindings target.


>
>
> https://codereview.chromium.org/2643063002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2017-03-22 15:30:49]

On 2017/03/22 15:15:52, jam wrote:
> On 2017/03/22 11:33:15, blundell wrote:
> > On 2017/03/21 22:02:17, jam wrote:
> > > Can you explain why we have BlinkConnectorImpl which wraps
> > > service_manager::Connector, instead of simply exposing the latter to Blink
> > (and
> > > adding test hooks either in Blink, or in service_manager::Connector)?
We're
> > > moving in the direction of having Blink call mojo directly, instead of
> through
> > > content, so this seems like adding code to content/renderer which we'll
have
> > to
> > > remove in the future.
> > > 
> > > If the concern is about the usage of stl strings, given that it's still
> > > happening but hidden inside content, then it's not a performance issue
> > > (especially since these methods aren't called often). If the concern is
> about
> > > using std::string from blink, blink won't have to explicitly create any
> local
> > > std::strings, it can just call
> > >
> service_manager::Connector::BindInterface(device::mojom::blink::kServiceName,
> > > ...) as it does today. Seems like equivalent to what this cl does today,
but
> > > with one less interface in the middle. wdyt?
> > 
> > Hi John,
> > 
> > service_manager::Connector directly and transitively exposes the chromium
> mojom
> > variant types, 
> 
> Just to make sure I'm following, you're referring to these includes:
> #include "services/service_manager/public/interfaces/connector.mojom.h"
> #include "services/service_manager/public/interfaces/service.mojom.h"
> #include "services/service_manager/public/interfaces/service_manager.mojom.h"
> ?

These includes also wrap Chromium mojom types:

#include "services/service_manager/public/cpp/connection.h"
#include "services/service_manager/public/cpp/identity.h"

> 
> > so exposing service_manager::Connector to Blink would be
> > introducing mixing of the Chromium and Blink mojom variant types in Blink.
> That
> > seems like a step that would have to be taken very thoughtfully if at all.
> 
> (assuming the answer to my question above is yes)
> Is the worry that code would accidentally use service manager mojom directly?
> Given that afaik no one uses it directly, even in chromium, and instead go
> through the client library, that seems like it wouldn't be likely? Is it
> possible to forward declare the objects and not have the includes there?

It's more that there's a pervasive assumption of usage of the Chromium mojom
variants throughout //services/service_manager/public/cpp (witness the above
includes that I listed, which also wrap and expose Chromium mojom variants),
whereas there's a pervasive assumption of non-usage of those variants in Blink.
For example, if there was a need to address a service by identity (e.g., in a
call to BindInterface()), it would then have to be the Chromium typemapped type
(//services/service_manager/public/cpp/identity.h), whereas Blink by definition
doesn't use the Chromium typemaps. Given that the indirection of
ServiceConnectorImpl should naturally fall away once crbug.com/703656 is fixed,
I'd like to avoid introducing the complexity of exposing Chromium mojom variants
to Blink here. I'd like us to more fully consider the benefits and pitfalls of
allowing the mojom variants to mix before introducing that possibility.

> 
> 
> > 
> > The good news here is that once crbug.com/703656 is fixed, all of this
> > infrastructure should be internal to Blink, with the only embedder
touchpoint
> > being that the embedder has to bind the connector (as one would expect).
> > Currently we can only implement the glue between JS and C++ in //content for
> the
> > reasons outlined in that bug.

[Ken Rockot(use gerrit already), 2017-03-22 15:35:03]

On Wed, Mar 22, 2017 at 8:30 AM, <blundell@chromium.org> wrote:

> On 2017/03/22 15:15:52, jam wrote:
> > On 2017/03/22 11:33:15, blundell wrote:
> > > On 2017/03/21 22:02:17, jam wrote:
> > > > Can you explain why we have BlinkConnectorImpl which wraps
> > > > service_manager::Connector, instead of simply exposing the latter to
> Blink
> > > (and
> > > > adding test hooks either in Blink, or in service_manager::Connector)?
> We're
> > > > moving in the direction of having Blink call mojo directly, instead
> of
> > through
> > > > content, so this seems like adding code to content/renderer which
> we'll
> have
> > > to
> > > > remove in the future.
> > > >
> > > > If the concern is about the usage of stl strings, given that it's
> still
> > > > happening but hidden inside content, then it's not a performance
> issue
> > > > (especially since these methods aren't called often). If the concern
> is
> > about
> > > > using std::string from blink, blink won't have to explicitly create
> any
> > local
> > > > std::strings, it can just call
> > > >
> > service_manager::Connector::BindInterface(device::mojom::
> blink::kServiceName,
> > > > ...) as it does today. Seems like equivalent to what this cl does
> today,
> but
> > > > with one less interface in the middle. wdyt?
> > >
> > > Hi John,
> > >
> > > service_manager::Connector directly and transitively exposes the
> chromium
> > mojom
> > > variant types,
> >
> > Just to make sure I'm following, you're referring to these includes:
> > #include "services/service_manager/public/interfaces/connector.mojom.h"
> > #include "services/service_manager/public/interfaces/service.mojom.h"
> > #include "services/service_manager/public/interfaces/service_
> manager.mojom.h"
> > ?
>
> These includes also wrap Chromium mojom types:
>
> #include "services/service_manager/public/cpp/connection.h"
> #include "services/service_manager/public/cpp/identity.h"
>
> >
> > > so exposing service_manager::Connector to Blink would be
> > > introducing mixing of the Chromium and Blink mojom variant types in
> Blink.
> > That
> > > seems like a step that would have to be taken very thoughtfully if at
> all.
> >
> > (assuming the answer to my question above is yes)
> > Is the worry that code would accidentally use service manager mojom
> directly?
> > Given that afaik no one uses it directly, even in chromium, and instead
> go
> > through the client library, that seems like it wouldn't be likely? Is it
> > possible to forward declare the objects and not have the includes there?
>
> It's more that there's a pervasive assumption of usage of the Chromium
> mojom
> variants throughout //services/service_manager/public/cpp (witness the
> above
> includes that I listed, which also wrap and expose Chromium mojom
> variants),
> whereas there's a pervasive assumption of non-usage of those variants in
> Blink.
> For example, if there was a need to address a service by identity (e.g.,
> in a
> call to BindInterface()), it would then have to be the Chromium typemapped
> type
> (//services/service_manager/public/cpp/identity.h), whereas Blink by
> definition
> doesn't use the Chromium typemaps. Given that the indirection of
> ServiceConnectorImpl should naturally fall away once crbug.com/703656 is
> fixed,
> I'd like to avoid introducing the complexity of exposing Chromium mojom
> variants
> to Blink here. I'd like us to more fully consider the benefits and
> pitfalls of
> allowing the mojom variants to mix before introducing that possibility.
>

I agree that this would be problematic. If any new mojom wants to refer to
service_manager.mojom.Identity, Blink code would normally have to use
service_manager::blink::mojom::Identity when using that interface, but
Blink code would otherwise use service_manager::mojom::Identity when using
a connector. This would be sad and confusing.


>
> >
> >
> > >
> > > The good news here is that once crbug.com/703656 is fixed, all of this
> > > infrastructure should be internal to Blink, with the only embedder
> touchpoint
> > > being that the embedder has to bind the connector (as one would
> expect).
> > > Currently we can only implement the glue between JS and C++ in
> //content for
> > the
> > > reasons outlined in that bug.
>
>
>
> https://codereview.chromium.org/2643063002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-03-22 15:35:05]

On Wed, Mar 22, 2017 at 8:30 AM, <blundell@chromium.org> wrote:

> On 2017/03/22 15:15:52, jam wrote:
> > On 2017/03/22 11:33:15, blundell wrote:
> > > On 2017/03/21 22:02:17, jam wrote:
> > > > Can you explain why we have BlinkConnectorImpl which wraps
> > > > service_manager::Connector, instead of simply exposing the latter to
> Blink
> > > (and
> > > > adding test hooks either in Blink, or in service_manager::Connector)?
> We're
> > > > moving in the direction of having Blink call mojo directly, instead
> of
> > through
> > > > content, so this seems like adding code to content/renderer which
> we'll
> have
> > > to
> > > > remove in the future.
> > > >
> > > > If the concern is about the usage of stl strings, given that it's
> still
> > > > happening but hidden inside content, then it's not a performance
> issue
> > > > (especially since these methods aren't called often). If the concern
> is
> > about
> > > > using std::string from blink, blink won't have to explicitly create
> any
> > local
> > > > std::strings, it can just call
> > > >
> > service_manager::Connector::BindInterface(device::mojom::
> blink::kServiceName,
> > > > ...) as it does today. Seems like equivalent to what this cl does
> today,
> but
> > > > with one less interface in the middle. wdyt?
> > >
> > > Hi John,
> > >
> > > service_manager::Connector directly and transitively exposes the
> chromium
> > mojom
> > > variant types,
> >
> > Just to make sure I'm following, you're referring to these includes:
> > #include "services/service_manager/public/interfaces/connector.mojom.h"
> > #include "services/service_manager/public/interfaces/service.mojom.h"
> > #include "services/service_manager/public/interfaces/service_
> manager.mojom.h"
> > ?
>
> These includes also wrap Chromium mojom types:
>
> #include "services/service_manager/public/cpp/connection.h"
> #include "services/service_manager/public/cpp/identity.h"
>
> >
> > > so exposing service_manager::Connector to Blink would be
> > > introducing mixing of the Chromium and Blink mojom variant types in
> Blink.
> > That
> > > seems like a step that would have to be taken very thoughtfully if at
> all.
> >
> > (assuming the answer to my question above is yes)
> > Is the worry that code would accidentally use service manager mojom
> directly?
> > Given that afaik no one uses it directly, even in chromium, and instead
> go
> > through the client library, that seems like it wouldn't be likely? Is it
> > possible to forward declare the objects and not have the includes there?
>
> It's more that there's a pervasive assumption of usage of the Chromium
> mojom
> variants throughout //services/service_manager/public/cpp (witness the
> above
> includes that I listed, which also wrap and expose Chromium mojom
> variants),
> whereas there's a pervasive assumption of non-usage of those variants in
> Blink.
> For example, if there was a need to address a service by identity (e.g.,
> in a
> call to BindInterface()), it would then have to be the Chromium typemapped
> type
> (//services/service_manager/public/cpp/identity.h), whereas Blink by
> definition
> doesn't use the Chromium typemaps. Given that the indirection of
> ServiceConnectorImpl should naturally fall away once crbug.com/703656 is
> fixed,
> I'd like to avoid introducing the complexity of exposing Chromium mojom
> variants
> to Blink here. I'd like us to more fully consider the benefits and
> pitfalls of
> allowing the mojom variants to mix before introducing that possibility.
>

I agree that this would be problematic. If any new mojom wants to refer to
service_manager.mojom.Identity, Blink code would normally have to use
service_manager::blink::mojom::Identity when using that interface, but
Blink code would otherwise use service_manager::mojom::Identity when using
a connector. This would be sad and confusing.


>
> >
> >
> > >
> > > The good news here is that once crbug.com/703656 is fixed, all of this
> > > infrastructure should be internal to Blink, with the only embedder
> touchpoint
> > > being that the embedder has to bind the connector (as one would
> expect).
> > > Currently we can only implement the glue between JS and C++ in
> //content for
> > the
> > > reasons outlined in that bug.
>
>
>
> https://codereview.chromium.org/2643063002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2017-03-22 15:44:25]

On 2017/03/22 15:35:05, Ken Rockot wrote:
> On Wed, Mar 22, 2017 at 8:30 AM, <mailto:blundell@chromium.org> wrote:
> 
> > On 2017/03/22 15:15:52, jam wrote:
> > > On 2017/03/22 11:33:15, blundell wrote:
> > > > On 2017/03/21 22:02:17, jam wrote:
> > > > > Can you explain why we have BlinkConnectorImpl which wraps
> > > > > service_manager::Connector, instead of simply exposing the latter to
> > Blink
> > > > (and
> > > > > adding test hooks either in Blink, or in service_manager::Connector)?
> > We're
> > > > > moving in the direction of having Blink call mojo directly, instead
> > of
> > > through
> > > > > content, so this seems like adding code to content/renderer which
> > we'll
> > have
> > > > to
> > > > > remove in the future.
> > > > >
> > > > > If the concern is about the usage of stl strings, given that it's
> > still
> > > > > happening but hidden inside content, then it's not a performance
> > issue
> > > > > (especially since these methods aren't called often). If the concern
> > is
> > > about
> > > > > using std::string from blink, blink won't have to explicitly create
> > any
> > > local
> > > > > std::strings, it can just call
> > > > >
> > > service_manager::Connector::BindInterface(device::mojom::
> > blink::kServiceName,
> > > > > ...) as it does today. Seems like equivalent to what this cl does
> > today,
> > but
> > > > > with one less interface in the middle. wdyt?
> > > >
> > > > Hi John,
> > > >
> > > > service_manager::Connector directly and transitively exposes the
> > chromium
> > > mojom
> > > > variant types,
> > >
> > > Just to make sure I'm following, you're referring to these includes:
> > > #include "services/service_manager/public/interfaces/connector.mojom.h"
> > > #include "services/service_manager/public/interfaces/service.mojom.h"
> > > #include "services/service_manager/public/interfaces/service_
> > manager.mojom.h"
> > > ?
> >
> > These includes also wrap Chromium mojom types:
> >
> > #include "services/service_manager/public/cpp/connection.h"
> > #include "services/service_manager/public/cpp/identity.h"
> >
> > >
> > > > so exposing service_manager::Connector to Blink would be
> > > > introducing mixing of the Chromium and Blink mojom variant types in
> > Blink.
> > > That
> > > > seems like a step that would have to be taken very thoughtfully if at
> > all.
> > >
> > > (assuming the answer to my question above is yes)
> > > Is the worry that code would accidentally use service manager mojom
> > directly?
> > > Given that afaik no one uses it directly, even in chromium, and instead
> > go
> > > through the client library, that seems like it wouldn't be likely? Is it
> > > possible to forward declare the objects and not have the includes there?
> >
> > It's more that there's a pervasive assumption of usage of the Chromium
> > mojom
> > variants throughout //services/service_manager/public/cpp (witness the
> > above
> > includes that I listed, which also wrap and expose Chromium mojom
> > variants),
> > whereas there's a pervasive assumption of non-usage of those variants in
> > Blink.
> > For example, if there was a need to address a service by identity (e.g.,
> > in a
> > call to BindInterface()), it would then have to be the Chromium typemapped
> > type
> > (//services/service_manager/public/cpp/identity.h), whereas Blink by
> > definition
> > doesn't use the Chromium typemaps. Given that the indirection of
> > ServiceConnectorImpl should naturally fall away once crbug.com/703656 is
> > fixed,
> > I'd like to avoid introducing the complexity of exposing Chromium mojom
> > variants
> > to Blink here. I'd like us to more fully consider the benefits and
> > pitfalls of
> > allowing the mojom variants to mix before introducing that possibility.
> >
> 
> I agree that this would be problematic. If any new mojom wants to refer to
> service_manager.mojom.Identity, Blink code would normally have to use
> service_manager::blink::mojom::Identity when using that interface, but
> Blink code would otherwise use service_manager::mojom::Identity when using
> a connector. This would be sad and confusing.

This is if blink code called service_manager::Connector::BindConnectorRequest()?

I was assuming the blink code only needs to call
service_manager::Connector::BindToInterface, since that's all it gets in this
cl. Yes that does mean service_manager::Identity is used, but that is already
happening in this cl but through an extra interface in content/. I'm having a
hard time seeing why it's so bad if we allow that client library class to be
used directly. Regarding the mojom includes in connector.h, I'm still curious if
they can be replaced by forward declarations?

[Ken Rockot(use gerrit already), 2017-03-22 15:46:53]

On Wed, Mar 22, 2017 at 8:44 AM, <jam@chromium.org> wrote:

> On 2017/03/22 15:35:05, Ken Rockot wrote:
>
> > On Wed, Mar 22, 2017 at 8:30 AM, <mailto:blundell@chromium.org> wrote:
> >
> > > On 2017/03/22 15:15:52, jam wrote:
> > > > On 2017/03/22 11:33:15, blundell wrote:
> > > > > On 2017/03/21 22:02:17, jam wrote:
> > > > > > Can you explain why we have BlinkConnectorImpl which wraps
> > > > > > service_manager::Connector, instead of simply exposing the
> latter to
> > > Blink
> > > > > (and
> > > > > > adding test hooks either in Blink, or in
> service_manager::Connector)?
> > > We're
> > > > > > moving in the direction of having Blink call mojo directly,
> instead
> > > of
> > > > through
> > > > > > content, so this seems like adding code to content/renderer which
> > > we'll
> > > have
> > > > > to
> > > > > > remove in the future.
> > > > > >
> > > > > > If the concern is about the usage of stl strings, given that it's
> > > still
> > > > > > happening but hidden inside content, then it's not a performance
> > > issue
> > > > > > (especially since these methods aren't called often). If the
> concern
> > > is
> > > > about
> > > > > > using std::string from blink, blink won't have to explicitly
> create
> > > any
> > > > local
> > > > > > std::strings, it can just call
> > > > > >
> > > > service_manager::Connector::BindInterface(device::mojom::
> > > blink::kServiceName,
> > > > > > ...) as it does today. Seems like equivalent to what this cl does
> > > today,
> > > but
> > > > > > with one less interface in the middle. wdyt?
> > > > >
> > > > > Hi John,
> > > > >
> > > > > service_manager::Connector directly and transitively exposes the
> > > chromium
> > > > mojom
> > > > > variant types,
> > > >
> > > > Just to make sure I'm following, you're referring to these includes:
> > > > #include "services/service_manager/public/interfaces/connector.
> mojom.h"
> > > > #include "services/service_manager/public/interfaces/service.
> mojom.h"
> > > > #include "services/service_manager/public/interfaces/service_
> > > manager.mojom.h"
> > > > ?
> > >
> > > These includes also wrap Chromium mojom types:
> > >
> > > #include "services/service_manager/public/cpp/connection.h"
> > > #include "services/service_manager/public/cpp/identity.h"
> > >
> > > >
> > > > > so exposing service_manager::Connector to Blink would be
> > > > > introducing mixing of the Chromium and Blink mojom variant types in
> > > Blink.
> > > > That
> > > > > seems like a step that would have to be taken very thoughtfully if
> at
> > > all.
> > > >
> > > > (assuming the answer to my question above is yes)
> > > > Is the worry that code would accidentally use service manager mojom
> > > directly?
> > > > Given that afaik no one uses it directly, even in chromium, and
> instead
> > > go
> > > > through the client library, that seems like it wouldn't be likely?
> Is it
> > > > possible to forward declare the objects and not have the includes
> there?
> > >
> > > It's more that there's a pervasive assumption of usage of the Chromium
> > > mojom
> > > variants throughout //services/service_manager/public/cpp (witness the
> > > above
> > > includes that I listed, which also wrap and expose Chromium mojom
> > > variants),
> > > whereas there's a pervasive assumption of non-usage of those variants
> in
> > > Blink.
> > > For example, if there was a need to address a service by identity
> (e.g.,
> > > in a
> > > call to BindInterface()), it would then have to be the Chromium
> typemapped
> > > type
> > > (//services/service_manager/public/cpp/identity.h), whereas Blink by
> > > definition
> > > doesn't use the Chromium typemaps. Given that the indirection of
> > > ServiceConnectorImpl should naturally fall away once crbug.com/703656
> is
> > > fixed,
> > > I'd like to avoid introducing the complexity of exposing Chromium mojom
> > > variants
> > > to Blink here. I'd like us to more fully consider the benefits and
> > > pitfalls of
> > > allowing the mojom variants to mix before introducing that possibility.
> > >
> >
> > I agree that this would be problematic. If any new mojom wants to refer
> to
> > service_manager.mojom.Identity, Blink code would normally have to use
> > service_manager::blink::mojom::Identity when using that interface, but
> > Blink code would otherwise use service_manager::mojom::Identity when
> using
> > a connector. This would be sad and confusing.
>
> This is if blink code called service_manager::Connector::
> BindConnectorRequest()?
>
> I was assuming the blink code only needs to call
> service_manager::Connector::BindToInterface, since that's all it gets in
> this
> cl. Yes that does mean service_manager::Identity is used, but that is
> already
> happening in this cl but through an extra interface in content/. I'm
> having a
> hard time seeing why it's so bad if we allow that client library class to
> be
> used directly. Regarding the mojom includes in connector.h, I'm still
> curious if
> they can be replaced by forward declarations?
>
>
Again, the problem is not that it would necessarily be bad for Blink to use
service_manager::Identity, but that it can easily end up using
service_manager::Identity in some places while needing to use
service_manager::mojom::blink::Identity in others. Because of how typemaps
work.

https://codereview.chromium.org/2643063002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-03-22 15:46:53]

On Wed, Mar 22, 2017 at 8:44 AM, <jam@chromium.org> wrote:

> On 2017/03/22 15:35:05, Ken Rockot wrote:
>
> > On Wed, Mar 22, 2017 at 8:30 AM, <mailto:blundell@chromium.org> wrote:
> >
> > > On 2017/03/22 15:15:52, jam wrote:
> > > > On 2017/03/22 11:33:15, blundell wrote:
> > > > > On 2017/03/21 22:02:17, jam wrote:
> > > > > > Can you explain why we have BlinkConnectorImpl which wraps
> > > > > > service_manager::Connector, instead of simply exposing the
> latter to
> > > Blink
> > > > > (and
> > > > > > adding test hooks either in Blink, or in
> service_manager::Connector)?
> > > We're
> > > > > > moving in the direction of having Blink call mojo directly,
> instead
> > > of
> > > > through
> > > > > > content, so this seems like adding code to content/renderer which
> > > we'll
> > > have
> > > > > to
> > > > > > remove in the future.
> > > > > >
> > > > > > If the concern is about the usage of stl strings, given that it's
> > > still
> > > > > > happening but hidden inside content, then it's not a performance
> > > issue
> > > > > > (especially since these methods aren't called often). If the
> concern
> > > is
> > > > about
> > > > > > using std::string from blink, blink won't have to explicitly
> create
> > > any
> > > > local
> > > > > > std::strings, it can just call
> > > > > >
> > > > service_manager::Connector::BindInterface(device::mojom::
> > > blink::kServiceName,
> > > > > > ...) as it does today. Seems like equivalent to what this cl does
> > > today,
> > > but
> > > > > > with one less interface in the middle. wdyt?
> > > > >
> > > > > Hi John,
> > > > >
> > > > > service_manager::Connector directly and transitively exposes the
> > > chromium
> > > > mojom
> > > > > variant types,
> > > >
> > > > Just to make sure I'm following, you're referring to these includes:
> > > > #include "services/service_manager/public/interfaces/connector.
> mojom.h"
> > > > #include "services/service_manager/public/interfaces/service.
> mojom.h"
> > > > #include "services/service_manager/public/interfaces/service_
> > > manager.mojom.h"
> > > > ?
> > >
> > > These includes also wrap Chromium mojom types:
> > >
> > > #include "services/service_manager/public/cpp/connection.h"
> > > #include "services/service_manager/public/cpp/identity.h"
> > >
> > > >
> > > > > so exposing service_manager::Connector to Blink would be
> > > > > introducing mixing of the Chromium and Blink mojom variant types in
> > > Blink.
> > > > That
> > > > > seems like a step that would have to be taken very thoughtfully if
> at
> > > all.
> > > >
> > > > (assuming the answer to my question above is yes)
> > > > Is the worry that code would accidentally use service manager mojom
> > > directly?
> > > > Given that afaik no one uses it directly, even in chromium, and
> instead
> > > go
> > > > through the client library, that seems like it wouldn't be likely?
> Is it
> > > > possible to forward declare the objects and not have the includes
> there?
> > >
> > > It's more that there's a pervasive assumption of usage of the Chromium
> > > mojom
> > > variants throughout //services/service_manager/public/cpp (witness the
> > > above
> > > includes that I listed, which also wrap and expose Chromium mojom
> > > variants),
> > > whereas there's a pervasive assumption of non-usage of those variants
> in
> > > Blink.
> > > For example, if there was a need to address a service by identity
> (e.g.,
> > > in a
> > > call to BindInterface()), it would then have to be the Chromium
> typemapped
> > > type
> > > (//services/service_manager/public/cpp/identity.h), whereas Blink by
> > > definition
> > > doesn't use the Chromium typemaps. Given that the indirection of
> > > ServiceConnectorImpl should naturally fall away once crbug.com/703656
> is
> > > fixed,
> > > I'd like to avoid introducing the complexity of exposing Chromium mojom
> > > variants
> > > to Blink here. I'd like us to more fully consider the benefits and
> > > pitfalls of
> > > allowing the mojom variants to mix before introducing that possibility.
> > >
> >
> > I agree that this would be problematic. If any new mojom wants to refer
> to
> > service_manager.mojom.Identity, Blink code would normally have to use
> > service_manager::blink::mojom::Identity when using that interface, but
> > Blink code would otherwise use service_manager::mojom::Identity when
> using
> > a connector. This would be sad and confusing.
>
> This is if blink code called service_manager::Connector::
> BindConnectorRequest()?
>
> I was assuming the blink code only needs to call
> service_manager::Connector::BindToInterface, since that's all it gets in
> this
> cl. Yes that does mean service_manager::Identity is used, but that is
> already
> happening in this cl but through an extra interface in content/. I'm
> having a
> hard time seeing why it's so bad if we allow that client library class to
> be
> used directly. Regarding the mojom includes in connector.h, I'm still
> curious if
> they can be replaced by forward declarations?
>
>
Again, the problem is not that it would necessarily be bad for Blink to use
service_manager::Identity, but that it can easily end up using
service_manager::Identity in some places while needing to use
service_manager::mojom::blink::Identity in others. Because of how typemaps
work.

https://codereview.chromium.org/2643063002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2017-03-22 17:21:35]

lgtm

Colin/Ken/I chatted, and since this change blocks other cls and Dimitri is gone
for a week, Colin/Ken will investigate having a service_manager::Connector class
that doesn't depend on chromium bindings in the meantime.

[Ken Rockot(use gerrit already), 2017-03-23 04:56:04]

https://codereview.chromium.org/2643063002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/mojo-helpers.js (right):

https://codereview.chromium.org/2643063002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/mojo-helpers.js:45:
connector.clearInterfaceOverridesForTesting();
On 2017/03/21 at 18:56:18, dcheng wrote:
> Do we have any documentation about the differences between these three things?
If I wanted to write a layout test and mock out interfaces, which one of these
things should I be mocking interfaces against?
> 
> I guess connector is to let us mock interfaces for things that Blink connects
*to*, while frameInterfaces / interfaces are for mocking interfaces that Blink
implements itself? Do I have this roughly correct? =)

No, they're all different ways for the client (blink) to connect *to* things,
each with their own API for locally intercepting those outgoing connections and
routing them back to fake local impls. e.g. you have a web API with a C++ impl
that talks to some mojom interface normally implemented in the browser, and
within a layout test you can intercept its requests and route them instead to a
JS mock impl of the target interface.

interfaces is the original thing - we didn't have services way back then, just a
big open interface registry in the RPH, and this is an interface provider which
routes interface requests to that with no additional context. This is
essentially deprecated but still in use (of course). We'll delete it soon.

frameInterfaces is similarly an interface provider, but scoped to an RFH. I
expect that this will stick around in some form, as we still have no better way
of arranging for some frame hosting/navigation service to broker interface
requests on a frame's behalf.

connector is the generic service manager connector API that services should
generally use to connect to each other. It binds requests in the form of
(target_service, interface_request), brokered through the service manager. There
is no reason why existing uses of the RPH interface registry cannot be trivially
ported to uses of connector which bind to ("content_browser",
whatever_interface), for example, which is how we plan to get rid of the first
thing.

[blundell, 2017-03-23 09:43:50]

On 2017/03/21 17:31:04, dglazkov OOO till March 27 wrote:
> On 2017/03/21 at 16:34:21, blundell wrote:
> > s/getInterface/bindInterface done. Also changed a mistaken usage of
> std::map::insert() that mailto:leon.han@intel.com helpfully pointed out.
> 
> Thank you for filing a bug! Could you also file one about removing the need to
> go to main thread to route getInterface calls?

Filed crbug.com/704482.

> 
> > 
> > Dimitri, PTAL!
> > 
> > +jam@ for //content as well

[blundell, 2017-03-23 10:40:34]

Thanks, everyone!

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/...
File content/renderer/mojo/blink_connector_impl.cc (right):

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:29: if (!connector_.get())
On 2017/03/21 18:56:18, dcheng wrote:
> Nit: no .get()

Done.

> 
> (Also, is this 'null in some testing contexts' something we'd be able to
> eventually fix? It seems like we should also be trying to fix the tests so
that
> we don't have test-only branches)

Part of the larger RenderThread::current() issue, as you noted.

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/mojo/...
content/renderer/mojo/blink_connector_impl.cc:64:
(*(service_binders_[service_name].get()))[interface_name] = binder;
On 2017/03/21 18:56:18, dcheng wrote:
> I think this can be simplified slightly to:
> 
> (*(service_binders_[service_name]))[interface_name] = binder;

Done.

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.h (right):

https://codereview.chromium.org/2643063002/diff/240001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.h:189: blink::Connector*
connector() override;
On 2017/03/21 18:56:18, dcheng wrote:
> Optional nit: one alternative to consider is using a covariant return type
here.
> It means we need the full definition of BlinkConnectorImpl in this header, but
> then we can have connector() directly return BlinkConnectorImpl* here,
> eliminating the need for the downcast in RenderFrameImpl.

Done.

[blundell, 2017-03-23 10:40:42]

The CQ bit was checked by blundell@chromium.org

[blundell, 2017-03-23 10:40:43]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dglazkov@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2643063002/#ps260001
(title: "Response to review")

[commit-bot: I haz the power, 2017-03-23 10:40:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 10:50:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 10:50:19]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[blundell, 2017-03-23 11:48:36]

The CQ bit was checked by blundell@chromium.org

[blundell, 2017-03-23 11:48:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dglazkov@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2643063002/#ps280001
(title: "Fix build")

[commit-bot: I haz the power, 2017-03-23 11:48:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 13:25:28]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1490269716884500,
"parent_rev": "70fc37e145f7d413492aab8f817f358f4925dabe", "commit_rev":
"43dd7b3dd9c675d5f9df67feae7662ee196517fa"}

[commit-bot: I haz the power, 2017-03-23 13:26:16]

Description was changed from

==========
Refactor Blink's ServiceConnector and add ability to mock in layout tests

This CL refactors the implementation of Blink's ServiceConnector so that
layout tests can mock remote interfaces of arbitrary services in JS.
Specifically, it changes the organization to be roughly parallel to
Platform::interfaceProvider().

- Blink has a Connector interface.
- Platform has a connector() method.
- //content/renderer has a BlinkConnectorImpl backed directly by
  //content/renderer's Connector.
- BlinkConnectorImpl also has the ability to override interfaces with local
  implementations, similar to service_manager::InterfaceProvider.
- We add a JS wrapper of BlinkConnectorImpl to expose that ability to JS.
==========

to

==========
Refactor Blink's ServiceConnector and add ability to mock in layout tests

This CL refactors the implementation of Blink's ServiceConnector so that
layout tests can mock remote interfaces of arbitrary services in JS.
Specifically, it changes the organization to be roughly parallel to
Platform::interfaceProvider().

- Blink has a Connector interface.
- Platform has a connector() method.
- //content/renderer has a BlinkConnectorImpl backed directly by
  //content/renderer's Connector.
- BlinkConnectorImpl also has the ability to override interfaces with local
  implementations, similar to service_manager::InterfaceProvider.
- We add a JS wrapper of BlinkConnectorImpl to expose that ability to JS.

Review-Url: https://codereview.chromium.org/2643063002
Cr-Commit-Position: refs/heads/master@{#459064}
Committed:
https://chromium.googlesource.com/chromium/src/+/43dd7b3dd9c675d5f9df67feae76...
==========

[commit-bot: I haz the power, 2017-03-23 13:26:20]

Committed patchset #14 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/43dd7b3dd9c675d5f9df67feae76...