Mojo C++ Bindings: Eliminate unbound ThreadSafeInterfacePtr

Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarder as a reduced encapsulation of
thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner, where the underlying AssociatedInterfacePtr
must remain owned by the channel's IPC::MessagePipeReader.

Finally, in order to facilitate IPC::Channel exposing ipc.mojom
types through its public interface, the //ipc:mojom target has
been folded into //ipc's component exports. Any prior dependents
on //ipc:mojom have been updated accordingly.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests

Review-Url: https://codereview.chromium.org/2668153003
Cr-Commit-Position: refs/heads/master@{#449239}
Committed: https://chromium.googlesource.com/chromium/src/+/a628d0b45d5ce49a035020d8d67e9cc9a562ecac

[Ken Rockot(use gerrit already), 2017-02-01 05:48:53]

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

[commit-bot: I haz the power, 2017-02-01 05:49:13]

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

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

Description was changed from

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is still usable immediately.

BUG=682334
R=yzshen@chromium.org
==========

to

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

BUG=682334
R=yzshen@chromium.org
==========

[commit-bot: I haz the power, 2017-02-01 07:13:44]

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

[commit-bot: I haz the power, 2017-02-01 07:13:45]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-02-01 18:57:56]

https://codereview.chromium.org/2668153003/diff/1/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2668153003/diff/1/ipc/ipc_channel_proxy.h#new...
ipc/ipc_channel_proxy.h:220: void GetThreadSafeRemoteAssociatedInterface(
I guess the change I proposed is orthogonal:

I am thinking that we could make GetGenericRemoteAssociatedInterface() directly
calls into a
ThreadSafeAssociatedInterfacePtr<Channel>::GetRemoteAssociatedInterface()
instead of posting a task manually. And by doing "early serialization" with the
group controller on the calling thread, the resulting AssociatedInterfacePtrInfo
is immediately usable. WDYT?


Btw, I think after your change to ThreadSafeAssociatedInterfacePtr, we don't
need this GetThreadSafeRemoteAssociatedInterface() method, right? The user could
call GetRemoteAssociatedInterface() and then bind the resulting ptr info to a
ThreadSafeAssociatedInterfacePtr if they want.

[Ken Rockot(use gerrit already), 2017-02-03 21:47:55]

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

[commit-bot: I haz the power, 2017-02-03 21:48:37]

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-02-03 22:50:41]

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

[commit-bot: I haz the power, 2017-02-03 22:50:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ken Rockot(use gerrit already), 2017-02-03 23:30:35]

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

[commit-bot: I haz the power, 2017-02-03 23:31:01]

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-02-03 23:49:13]

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

[commit-bot: I haz the power, 2017-02-03 23:49:14]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2017-02-04 01:42:15]

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

[commit-bot: I haz the power, 2017-02-04 01:42:31]

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-02-04 02:10:18]

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

[commit-bot: I haz the power, 2017-02-04 02:10:18]

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

[Ken Rockot(use gerrit already), 2017-02-06 22:39:06]

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

[commit-bot: I haz the power, 2017-02-06 22:40:26]

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

[Ken Rockot(use gerrit already), 2017-02-06 22:41:11]

Description was changed from

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

BUG=682334
R=yzshen@chromium.org
==========

to

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarded as a reduced encapsulation of
the thread-safe serialization and forwarding logic. This is used
to implement ThreadSafEInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner.

BUG=682334
R=yzshen@chromium.org
==========

[Ken Rockot(use gerrit already), 2017-02-06 22:41:27]

Description was changed from

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarded as a reduced encapsulation of
the thread-safe serialization and forwarding logic. This is used
to implement ThreadSafEInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner.

BUG=682334
R=yzshen@chromium.org
==========

to

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarded as a reduced encapsulation of
the thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests
==========

[Ken Rockot(use gerrit already), 2017-02-06 22:43:20]

I think this may be ready for review now.

I've left GetThreadSafeRemoteAssociatedInterface in place as a matter of
convenience. WDYT?

[Ken Rockot(use gerrit already), 2017-02-06 22:44:33]

Description was changed from

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarded as a reduced encapsulation of
the thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests
==========

to

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarded as a reduced encapsulation of
the thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner.

Finally, in order to facilitate IPC::Channel exposing ipc.mojom
types through its public interface, the //ipc:mojom target has
been folded into //ipc's component exports. Any prior dependents
on //ipc:mojom have been updated accordingly.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests
==========

[Ken Rockot(use gerrit already), 2017-02-06 22:46:52]

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

[commit-bot: I haz the power, 2017-02-06 22:47:11]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2017-02-06 22:47:43]

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

[Ken Rockot(use gerrit already), 2017-02-06 22:49:55]

Description was changed from

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarded as a reduced encapsulation of
the thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner.

Finally, in order to facilitate IPC::Channel exposing ipc.mojom
types through its public interface, the //ipc:mojom target has
been folded into //ipc's component exports. Any prior dependents
on //ipc:mojom have been updated accordingly.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests
==========

to

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarder as a reduced encapsulation of
the thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner.

Finally, in order to facilitate IPC::Channel exposing ipc.mojom
types through its public interface, the //ipc:mojom target has
been folded into //ipc's component exports. Any prior dependents
on //ipc:mojom have been updated accordingly.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests
==========

[Ken Rockot(use gerrit already), 2017-02-06 22:50:25]

Description was changed from

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarder as a reduced encapsulation of
the thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner.

Finally, in order to facilitate IPC::Channel exposing ipc.mojom
types through its public interface, the //ipc:mojom target has
been folded into //ipc's component exports. Any prior dependents
on //ipc:mojom have been updated accordingly.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests
==========

to

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarder as a reduced encapsulation of
thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner, where the underlying AssociatedInterfacePtr
must remain owned by the channel's IPC::MessagePipeReader.

Finally, in order to facilitate IPC::Channel exposing ipc.mojom
types through its public interface, the //ipc:mojom target has
been folded into //ipc's component exports. Any prior dependents
on //ipc:mojom have been updated accordingly.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests
==========

[commit-bot: I haz the power, 2017-02-06 23:49:37]

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

[commit-bot: I haz the power, 2017-02-06 23:49:38]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yzshen1, 2017-02-07 00:06:27]

Mostly looks good. Just some nits. Thanks!

https://codereview.chromium.org/2668153003/diff/80001/content/renderer/mojo/t...
File content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h
(left):

https://codereview.chromium.org/2668153003/diff/80001/content/renderer/mojo/t...
content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h:20: class
ThreadSafeAssociatedInterfacePtrProvider {
Happy to see that this is gone!

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:227:
context()->thread_safe_channel().GetAssociatedInterface(
nit: you could call GetGenericRemoteAssociatedInterface() instead.

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:361: // A thread-safe mojom::Channel interface we ues to
make remote interface
ues -> use please.

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_mojo_bootstrap_...
File ipc/ipc_mojo_bootstrap_unittest.cc (right):

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_mojo_bootstrap_...
ipc/ipc_mojo_bootstrap_unittest.cc:34: const base::Closure& on_peer_pid_set)
nit: wrong indent.

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_mojo_bootstrap_...
ipc/ipc_mojo_bootstrap_unittest.cc:102: sender->SetPeerPid(kTestPid);
Does it make sense to use a different ID than the one at the server side?

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/associated_interface_unittest.cc (right):

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:1095:
context->connection_ptr->GetSender(std::move(sender_request));
Does it make sense to move this line above 1090 and remove echo_called_event?
Otherwise it actually violates the contract "associated request must be sent
before pointer can be used". And it will soon be broken by my change of
supporting pending associated endpoint pairs.

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

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:21: // Instances of this
class may be used used from any thread to serialize
nit: remove one redundant "used" please.

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:39: ThreadSafeForwarder(
(I could do this in my CL. Just to mention it to get your early feedback.)

This class will need to know about the group controller[1]. So it can convert
associate pending endpoints on the calling thread instead of on the target
thread.

[1]I probably need to use a group controller getter callback. That is needed if
the ThreadSafeForwarder/ThreadSafeInterfacePtr is bound to a pending associated
interface ptr info. In that case the group controller itself is not available at
the beginning.

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:159:
Bind(ptr.PassInterface());
How about "ptr_ = std::move(ptr)"?
This is more efficient and also preserves the internal state of |ptr| (say, if
|ptr| has a pending callback).

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:181: PtrWrapper(const
scoped_refptr<base::SequencedTaskRunner>& task_runner)
explicit, please.

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:206:
base::Unretained(this)));
I think we could use "Unretained" here because no one holds a ref to this object
anymore, right? It seems nice to have a comment on this.

[Ken Rockot(use gerrit already), 2017-02-07 00:48:50]

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

[commit-bot: I haz the power, 2017-02-07 00:49:41]

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

[Ken Rockot(use gerrit already), 2017-02-07 01:25:32]

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:227:
context()->thread_safe_channel().GetAssociatedInterface(
On 2017/02/07 at 00:06:26, yzshen1 wrote:
> nit: you could call GetGenericRemoteAssociatedInterface() instead.

done (called GetRemoteAssociatedInterface instead), even shorter

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:361: // A thread-safe mojom::Channel interface we ues to
make remote interface
On 2017/02/07 at 00:06:26, yzshen1 wrote:
> ues -> use please.

done

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_mojo_bootstrap_...
File ipc/ipc_mojo_bootstrap_unittest.cc (right):

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_mojo_bootstrap_...
ipc/ipc_mojo_bootstrap_unittest.cc:34: const base::Closure& on_peer_pid_set)
On 2017/02/07 at 00:06:26, yzshen1 wrote:
> nit: wrong indent.

done

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_mojo_bootstrap_...
ipc/ipc_mojo_bootstrap_unittest.cc:102: sender->SetPeerPid(kTestPid);
On 2017/02/07 at 00:06:26, yzshen1 wrote:
> Does it make sense to use a different ID than the one at the server side?

done

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/associated_interface_unittest.cc (right):

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:1095:
context->connection_ptr->GetSender(std::move(sender_request));
On 2017/02/07 at 00:06:27, yzshen1 wrote:
> Does it make sense to move this line above 1090 and remove echo_called_event?
Otherwise it actually violates the contract "associated request must be sent
before pointer can be used". And it will soon be broken by my change of
supporting pending associated endpoint pairs.

Done

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

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:21: // Instances of this
class may be used used from any thread to serialize
On 2017/02/07 at 00:06:27, yzshen1 wrote:
> nit: remove one redundant "used" please.

Done

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:39: ThreadSafeForwarder(
On 2017/02/07 at 00:06:27, yzshen1 wrote:
> (I could do this in my CL. Just to mention it to get your early feedback.)
> 
> This class will need to know about the group controller[1]. So it can convert
associate pending endpoints on the calling thread instead of on the target
thread.
> 
> [1]I probably need to use a group controller getter callback. That is needed
if the ThreadSafeForwarder/ThreadSafeInterfacePtr is bound to a pending
associated interface ptr info. In that case the group controller itself is not
available at the beginning.

OK - Seems reasonable to me. Almost makes me want to change this to use a
delegate interface rather than ad hoc callbacks, but that seems like it would
only make it harder to use.

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:159:
Bind(ptr.PassInterface());
On 2017/02/07 at 00:06:27, yzshen1 wrote:
> How about "ptr_ = std::move(ptr)"?
> This is more efficient and also preserves the internal state of |ptr| (say, if
|ptr| has a pending callback).

Done

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:181: PtrWrapper(const
scoped_refptr<base::SequencedTaskRunner>& task_runner)
On 2017/02/07 at 00:06:27, yzshen1 wrote:
> explicit, please.

Done

https://codereview.chromium.org/2668153003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/thread_safe_interface_ptr.h:206:
base::Unretained(this)));
On 2017/02/07 at 00:06:27, yzshen1 wrote:
> I think we could use "Unretained" here because no one holds a ref to this
object anymore, right? It seems nice to have a comment on this.

Yep, done

[commit-bot: I haz the power, 2017-02-07 01:47:57]

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

[commit-bot: I haz the power, 2017-02-07 01:47:58]

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

[yzshen1, 2017-02-07 06:05:52]

LGTM with one nit.

Thanks!

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:227:
context()->thread_safe_channel().GetAssociatedInterface(
On 2017/02/07 01:25:32, Ken Rockot wrote:
> On 2017/02/07 at 00:06:26, yzshen1 wrote:
> > nit: you could call GetGenericRemoteAssociatedInterface() instead.
> 
> done (called GetRemoteAssociatedInterface instead), even shorter

Using GetRemoteAssociatedInterface() will setup an AssociatedInterfacePtr and
immediately abandon it (by call PassInterface on it). Because unlike
InterfacePtr, we don't do lazy initialization with AssociatedInterfacePtr, it
seems a little less efficient. WDYT?

[Ken Rockot(use gerrit already), 2017-02-08 00:36:46]

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

[Ken Rockot(use gerrit already), 2017-02-08 00:37:12]

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2668153003/diff/80001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:227:
context()->thread_safe_channel().GetAssociatedInterface(
On 2017/02/07 at 06:05:52, yzshen1 wrote:
> On 2017/02/07 01:25:32, Ken Rockot wrote:
> > On 2017/02/07 at 00:06:26, yzshen1 wrote:
> > > nit: you could call GetGenericRemoteAssociatedInterface() instead.
> > 
> > done (called GetRemoteAssociatedInterface instead), even shorter
> 
> Using GetRemoteAssociatedInterface() will setup an AssociatedInterfacePtr and
immediately abandon it (by call PassInterface on it). Because unlike
InterfacePtr, we don't do lazy initialization with AssociatedInterfacePtr, it
seems a little less efficient. WDYT?

Ah good point. Changed to use GetGenericRemoteAssociatedInterface.

[commit-bot: I haz the power, 2017-02-08 00:37:34]

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-02-08 01:23:42]

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

[commit-bot: I haz the power, 2017-02-08 01:23:43]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Ken Rockot(use gerrit already), 2017-02-08 06:12:56]

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

[Ken Rockot(use gerrit already), 2017-02-08 06:12:57]

+jam for content

Note that the redness on try jobs all seems to be caused by some issue with
steps, specifically for this CL, and none of the output appears to be useful.
Still looking into that.

[jam, 2017-02-08 17:08:13]

lgtm

[Ken Rockot(use gerrit already), 2017-02-08 17:36:08]

Patchset #2 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2017-02-08 17:36:17]

Patchset #2 (id:40001) has been deleted

[Ken Rockot(use gerrit already), 2017-02-08 17:36:23]

Patchset #2 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2017-02-08 17:36:29]

Patchset #3 (id:100001) has been deleted

[Ken Rockot(use gerrit already), 2017-02-08 17:36:34]

Patchset #3 (id:120001) has been deleted

[Ken Rockot(use gerrit already), 2017-02-09 00:47:17]

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

[commit-bot: I haz the power, 2017-02-09 00:47:40]

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

[Ken Rockot(use gerrit already), 2017-02-09 00:49:44]

OK - The red was ultimately caused by several targets in the tree missing a
dependency on //ipc. Should go green now...

[commit-bot: I haz the power, 2017-02-09 01:57:10]

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

[commit-bot: I haz the power, 2017-02-09 01:57:11]

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

[Ken Rockot(use gerrit already), 2017-02-09 02:54:19]

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

[commit-bot: I haz the power, 2017-02-09 02:54:43]

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

[Ken Rockot(use gerrit already), 2017-02-09 03:40:31]

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

[commit-bot: I haz the power, 2017-02-09 03:41:04]

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

[Ken Rockot(use gerrit already), 2017-02-09 05:56:46]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2017-02-09 05:56:48]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2017-02-09 05:56:49]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2668153003/#ps180001
(title: ".")

[commit-bot: I haz the power, 2017-02-09 05:57:25]

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-02-09 06:34:35]

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

[commit-bot: I haz the power, 2017-02-09 06:34:37]

Failed to apply patch for content/child/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: content/child/BUILD.gn:234
  error: content/child/BUILD.gn: patch does not apply

Patch:       content/child/BUILD.gn
Index: content/child/BUILD.gn
diff --git a/content/child/BUILD.gn b/content/child/BUILD.gn
index
f360e88c924763416f74bd7dacaf6ae10755044b..60811f7a5d45903cdf1d7cb92a96f2e7ebe970dd
100644
--- a/content/child/BUILD.gn
+++ b/content/child/BUILD.gn
@@ -234,7 +234,6 @@ target(link_target_type, "child") {
     "//device/power_monitor/public/cpp",
     "//gpu/command_buffer/client",
     "//ipc",
-    "//ipc:mojom",
     "//media",
     "//mojo/common",
     "//mojo/edk/system",

[Ken Rockot(use gerrit already), 2017-02-09 06:44:51]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2017-02-09 06:45:12]

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-02-09 06:49:31]

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

[commit-bot: I haz the power, 2017-02-09 06:49:34]

Failed to apply patch for content/child/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: content/child/BUILD.gn:234
  error: content/child/BUILD.gn: patch does not apply

Patch:       content/child/BUILD.gn
Index: content/child/BUILD.gn
diff --git a/content/child/BUILD.gn b/content/child/BUILD.gn
index
f360e88c924763416f74bd7dacaf6ae10755044b..60811f7a5d45903cdf1d7cb92a96f2e7ebe970dd
100644
--- a/content/child/BUILD.gn
+++ b/content/child/BUILD.gn
@@ -234,7 +234,6 @@ target(link_target_type, "child") {
     "//device/power_monitor/public/cpp",
     "//gpu/command_buffer/client",
     "//ipc",
-    "//ipc:mojom",
     "//media",
     "//mojo/common",
     "//mojo/edk/system",

[Ken Rockot(use gerrit already), 2017-02-09 06:54:39]

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

[commit-bot: I haz the power, 2017-02-09 06:55:05]

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-02-09 08:18:03]

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

[commit-bot: I haz the power, 2017-02-09 08:18:04]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-02-09 08:21:50]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2017-02-09 08:21:50]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2668153003/#ps200001
(title: "format and rebase...")

[commit-bot: I haz the power, 2017-02-09 08:22:16]

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-02-09 08:40:23]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1486628510268120,
"parent_rev": "b68fa9e4f675ec4f158c40297a5638bbb7089f29", "commit_rev":
"a628d0b45d5ce49a035020d8d67e9cc9a562ecac"}

[commit-bot: I haz the power, 2017-02-09 08:41:04]

Description was changed from

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarder as a reduced encapsulation of
thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner, where the underlying AssociatedInterfacePtr
must remain owned by the channel's IPC::MessagePipeReader.

Finally, in order to facilitate IPC::Channel exposing ipc.mojom
types through its public interface, the //ipc:mojom target has
been folded into //ipc's component exports. Any prior dependents
on //ipc:mojom have been updated accordingly.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests
==========

to

==========
Mojo C++ Bindings: Eliminiate unbound ThreadSafeInterfacePtr

Changes ThreadSafeInterfacePtr such that it is no longer possible
to create one which is unbound. Instead, it is now possible to
create one which will imminently be bound on a specific TaskRunner
but which is usable immediately.

Also introduces ThreadSafeForwarder as a reduced encapsulation of
thread-safe serialization and forwarding logic. This is used
to implement ThreadSafeInterfacePtrBase, as well as to support
immediate associated request forwarding on IPC::ChannelProxy in
a thread-safe manner, where the underlying AssociatedInterfacePtr
must remain owned by the channel's IPC::MessagePipeReader.

Finally, in order to facilitate IPC::Channel exposing ipc.mojom
types through its public interface, the //ipc:mojom target has
been folded into //ipc's component exports. Any prior dependents
on //ipc:mojom have been updated accordingly.

BUG=682334
R=yzshen@chromium.org
TEST=ipc_tests, mojo_public_bindings_unittests

Review-Url: https://codereview.chromium.org/2668153003
Cr-Commit-Position: refs/heads/master@{#449239}
Committed:
https://chromium.googlesource.com/chromium/src/+/a628d0b45d5ce49a035020d8d67e...
==========

[commit-bot: I haz the power, 2017-02-09 08:41:09]

Committed patchset #6 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/a628d0b45d5ce49a035020d8d67e...