Provide a Mojo equivalent of ThreadSafeSender.

Provide a Mojo equivalent of ThreadSafeSender.

Introduces the ThreadSafeAssociatedInterfacePtrProvider class that
provides functionalities similar to the content::ThreadSafeSender.
You create it with a ChannelProxy and you can then retrieve
ThreadSafeInterfacePtr's from it that you can call methods on from any
thread and even before the actual channel is connected.

BUG=668317
Committed: https://crrev.com/315d17fc22a94a23657b7c52df283f5f93795e89
Cr-Commit-Position: refs/heads/master@{#435005}

[Jay Civelli, 2016-11-24 00:28:06]

Description was changed from

==========
Provide a Mojo equivalent of ThreadSafeSender.

Introduces the ThreadSafeAssociatedInterfacePtrProvider class that
provides functionalities similar to the content::ThreadSafeSender.
You create it with a ChannelProxy and you can then retrieve
ThreadSafeInterfacePtr's from it that you can call methods on from any
thread and even before the actual channel is connected.

BUG=668317
==========

to

==========
Provide a Mojo equivalent of ThreadSafeSender.

Introduces the ThreadSafeAssociatedInterfacePtrProvider class that
provides functionalities similar to the content::ThreadSafeSender.
You create it with a ChannelProxy and you can then retrieve
ThreadSafeInterfacePtr's from it that you can call methods on from any
thread and even before the actual channel is connected.

BUG=668317
==========

[Jay Civelli, 2016-11-24 00:28:06]

jcivelli@chromium.org changed reviewers:
+ rockot@chromium.org, yzshen@chromium.org

[Jay Civelli, 2016-11-28 16:49:04]

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

[commit-bot: I haz the power, 2016-11-28 16:49:15]

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, 2016-11-28 17:59:04]

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

[commit-bot: I haz the power, 2016-11-28 17:59:05]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-11-28 18:05:45]

LGTM with one nit.

Thanks!

https://codereview.chromium.org/2522333002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2522333002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:704:
base::MakeUnique<ThreadSafeAssociatedInterfacePtrProvider>(channel());
I thought channel()'s return value didn't pass ownership to the caller?

https://codereview.chromium.org/2522333002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right):

https://codereview.chromium.org/2522333002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:69: bool
BindToCurrentThread(InterfacePtrType<Interface> interface_ptr) {
Comment on line 42/62 calls this method "Bind()".
Maybe the name "Bind()" makes more sense: BindToCurrentThread() sounds like this
ThreadSafeInterfacePtr should only be used on this particular thread. WDYT?

[Jay Civelli, 2016-11-28 18:32:45]

jcivelli@chromium.org changed reviewers:
+ jochen@chromium.org, tsepez@chromium.org

[Jay Civelli, 2016-11-28 18:33:51]

Adding @jochen for content/ OWNER review and @tsepez for IPC OWNER review.

Thanks.

[Jay Civelli, 2016-11-28 18:46:10]

https://codereview.chromium.org/2522333002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2522333002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:704:
base::MakeUnique<ThreadSafeAssociatedInterfacePtrProvider>(channel());
On 2016/11/28 18:05:45, yzshen1 wrote:
> I thought channel()'s return value didn't pass ownership to the caller?

It does not pass ownership. Here we are taking ownership of the new
ThreadSafeAssociatedInterfacePtrProvider we create, which takes a Channel* and
does not take ownership of it.

https://codereview.chromium.org/2522333002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right):

https://codereview.chromium.org/2522333002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:69: bool
BindToCurrentThread(InterfacePtrType<Interface> interface_ptr) {
On 2016/11/28 18:05:45, yzshen1 wrote:
> Comment on line 42/62 calls this method "Bind()".
> Maybe the name "Bind()" makes more sense: BindToCurrentThread() sounds like
this
> ThreadSafeInterfacePtr should only be used on this particular thread. WDYT?
> 

Good point, changed to Bind() and added a comment to emphasize this should be
called on the InterfacePtr's thread.

[Tom Sepez, 2016-11-28 18:49:11]

It seems like this is opening a pretty wide hole where we can stuff anything
onto the io thread.  Is there a design where we could have a few, more specific
methods?

https://codereview.chromium.org/2522333002/diff/40001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2522333002/diff/40001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:233: using RunOnIOThreadCallback =
base::Callback<void(Channel*)>;
nit: The name doesn't really describe that it takes a single channel argument,
rather how its used, which is tangential in some sense.

[yzshen1, 2016-11-28 18:50:21]

https://codereview.chromium.org/2522333002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2522333002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:704:
base::MakeUnique<ThreadSafeAssociatedInterfacePtrProvider>(channel());
On 2016/11/28 18:46:10, Jay Civelli wrote:
> On 2016/11/28 18:05:45, yzshen1 wrote:
> > I thought channel()'s return value didn't pass ownership to the caller?
> 
> It does not pass ownership. Here we are taking ownership of the new
> ThreadSafeAssociatedInterfacePtrProvider we create, which takes a Channel* and
> does not take ownership of it.

Ah, I see, it calls the constructor of ThreadSafeAssociatedInterfacePtrProvider
with a channel argument!

At a quick glance, it seems like a cast.

Thanks for explaining!

Still LGTM

[Jay Civelli, 2016-11-28 19:18:23]

On 2016/11/28 18:49:11, Tom Sepez wrote:
> It seems like this is opening a pretty wide hole where we can stuff anything
> onto the io thread.  Is there a design where we could have a few, more
specific
> methods?
Would something like that work?

void ChannelProxy::GetAssociatedInterfaceOnIOThread<Interface>(
    Callback<void<mojo::AssociatedInterfacePtr<Interface>>> callback) {
 ... post to IO thread...
}

void ChannelProxy::GetAssociatedInterfaceOnIOThreadImpl<Interface>(
    Callback<void<mojo::AssociatedInterfacePtr<Interface>>> callback) {
  mojo::AssociatedInterfacePtr<Interface> interface_ptr;
  channel_->GetAssociatedInterfaceSupport()->GetRemoteAssociatedInterface(
      &interface_ptr);
  callback.Run(std::move(interface_ptr));
}



> https://codereview.chromium.org/2522333002/diff/40001/ipc/ipc_channel_proxy.h
> File ipc/ipc_channel_proxy.h (right):
> 
>
https://codereview.chromium.org/2522333002/diff/40001/ipc/ipc_channel_proxy.h...
> ipc/ipc_channel_proxy.h:233: using RunOnIOThreadCallback =
> base::Callback<void(Channel*)>;
> nit: The name doesn't really describe that it takes a single channel argument,
> rather how its used, which is tangential in some sense.

[Tom Sepez, 2016-11-28 20:18:32]

On 2016/11/28 19:18:23, Jay Civelli wrote:
> On 2016/11/28 18:49:11, Tom Sepez wrote:
> > It seems like this is opening a pretty wide hole where we can stuff anything
> > onto the io thread.  Is there a design where we could have a few, more
> specific
> > methods?
> Would something like that work?
> 
> void ChannelProxy::GetAssociatedInterfaceOnIOThread<Interface>(
>     Callback<void<mojo::AssociatedInterfacePtr<Interface>>> callback) {
>  ... post to IO thread...
> }
> 
> void ChannelProxy::GetAssociatedInterfaceOnIOThreadImpl<Interface>(
>     Callback<void<mojo::AssociatedInterfacePtr<Interface>>> callback) {
>   mojo::AssociatedInterfacePtr<Interface> interface_ptr;
>   channel_->GetAssociatedInterfaceSupport()->GetRemoteAssociatedInterface(
>       &interface_ptr);
>   callback.Run(std::move(interface_ptr));
> }
> 
> 
I think that's much better and less prone to abuse.  Thanks!

[Jay Civelli, 2016-11-28 22:49:02]

https://codereview.chromium.org/2522333002/diff/40001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2522333002/diff/40001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:233: using RunOnIOThreadCallback =
base::Callback<void(Channel*)>;
On 2016/11/28 18:49:11, Tom Sepez wrote:
> nit: The name doesn't really describe that it takes a single channel argument,
> rather how its used, which is tangential in some sense.

Renamed the callback as part of making the API more specific.

[Tom Sepez, 2016-11-28 23:11:31]

lgtm

[Jay Civelli, 2016-11-29 00:32:52]

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

[commit-bot: I haz the power, 2016-11-29 00:34:38]

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, 2016-11-29 03:11:29]

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

[commit-bot: I haz the power, 2016-11-29 03:11:30]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-11-29 15:44:20]

lgtm

[Jay Civelli, 2016-11-29 16:10:46]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2016-11-29 16:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2522333002/#ps60001
(title: "Addresses @tsepez comment")

[commit-bot: I haz the power, 2016-11-29 16:11:01]

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, 2016-11-29 16:15:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480435846394570,
"parent_rev": "c4f0e1c80db4646bcc3aec020b52b18fd1a10bb1", "commit_rev":
"653068de6695cac26d3dbbdf68dc32b89b41cbcb"}

[commit-bot: I haz the power, 2016-11-29 16:15:35]

Description was changed from

==========
Provide a Mojo equivalent of ThreadSafeSender.

Introduces the ThreadSafeAssociatedInterfacePtrProvider class that
provides functionalities similar to the content::ThreadSafeSender.
You create it with a ChannelProxy and you can then retrieve
ThreadSafeInterfacePtr's from it that you can call methods on from any
thread and even before the actual channel is connected.

BUG=668317
==========

to

==========
Provide a Mojo equivalent of ThreadSafeSender.

Introduces the ThreadSafeAssociatedInterfacePtrProvider class that
provides functionalities similar to the content::ThreadSafeSender.
You create it with a ChannelProxy and you can then retrieve
ThreadSafeInterfacePtr's from it that you can call methods on from any
thread and even before the actual channel is connected.

BUG=668317
==========

[commit-bot: I haz the power, 2016-11-29 16:15:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-29 16:17:48]

Description was changed from

==========
Provide a Mojo equivalent of ThreadSafeSender.

Introduces the ThreadSafeAssociatedInterfacePtrProvider class that
provides functionalities similar to the content::ThreadSafeSender.
You create it with a ChannelProxy and you can then retrieve
ThreadSafeInterfacePtr's from it that you can call methods on from any
thread and even before the actual channel is connected.

BUG=668317
==========

to

==========
Provide a Mojo equivalent of ThreadSafeSender.

Introduces the ThreadSafeAssociatedInterfacePtrProvider class that
provides functionalities similar to the content::ThreadSafeSender.
You create it with a ChannelProxy and you can then retrieve
ThreadSafeInterfacePtr's from it that you can call methods on from any
thread and even before the actual channel is connected.

BUG=668317

Committed: https://crrev.com/315d17fc22a94a23657b7c52df283f5f93795e89
Cr-Commit-Position: refs/heads/master@{#435005}
==========

[commit-bot: I haz the power, 2016-11-29 16:17:49]

Patchset 4 (id:??) landed as
https://crrev.com/315d17fc22a94a23657b7c52df283f5f93795e89
Cr-Commit-Position: refs/heads/master@{#435005}

[blundell, 2016-12-29 10:10:16]

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

[blundell, 2016-12-29 10:10:17]

leonhsl@intel.com made the comment that the interface added here seems
problematic: ThreadSafeAssociatedInterfacePtrProvider::CreateInterfacePtr()
returns an unbound mojo::ThreadSafeAssociatedInterfacePtr. Per the documentation
on the latter class no interface methods should be called on an unbound
instance. However the documentation of the interface added here encourages using
the returned object immediately. Is there something that we're missing that
makes this seeming discrepancy actually OK?

If this *is* problematic, one way to resolve it would be by making the
CreateInterfacePtr() method added here asynchronous (i.e., it calls a
client-provided callback from BindInterfacePtr()). I'm not sure if that change
would cripple the intended use case.

[Jay Civelli, 2016-12-29 20:51:13]

Thanks for pointing that out. You are right, that code is not correct.
I have a fix where the ThreadSafeAssociatedInterfacePtr when created with
CreateUnound() takes in the task runner on which the bind will happen.
This allows you to create the ThreadSafeAssociatedInterfacePtr, post a task
to bind and then call on the ThreadSafeAssociatedInterfacePtr immediately
(which are posted to the thread where the bind will happen) .
See https://codereview.chromium.org/2608783003

Jay

On Thu, Dec 29, 2016 at 2:10 AM, <blundell@chromium.org> wrote:

> leonhsl@intel.com made the comment that the interface added here seems
> problematic: ThreadSafeAssociatedInterfacePtrProvider::CreateInterfacePtr
> ()
> returns an unbound mojo::ThreadSafeAssociatedInterfacePtr. Per the
> documentation
> on the latter class no interface methods should be called on an unbound
> instance. However the documentation of the interface added here encourages
> using
> the returned object immediately. Is there something that we're missing that
> makes this seeming discrepancy actually OK?
>
> If this *is* problematic, one way to resolve it would be by making the
> CreateInterfacePtr() method added here asynchronous (i.e., it calls a
> client-provided callback from BindInterfacePtr()). I'm not sure if that
> change
> would cripple the intended use case.
>
> https://codereview.chromium.org/2522333002/
>

-- 
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.