Add FlushForTesting to InterfacePtr and Binding.

Add FlushForTesting to InterfacePtr and Binding.

Previously, when unit testing a class that is a client of a mojo
interface, there was no reliable way to detect that no message was sent.
This adds a FlushForTesting() method to mojo message pipe bindings,
which can be used to wait for a round trip, after which any messages
already sent will have been delivered; this can be used to test for the
absence of messages sent in response to some stimulus.

Committed: https://crrev.com/462458f4b533d44bfad95ea07dc99257eaf028cb
Cr-Commit-Position: refs/heads/master@{#415920}

[Sam McNally, 2016-08-25 00:06:02]

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

[commit-bot: I haz the power, 2016-08-25 00:07:00]

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-08-25 00:11:28]

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

[commit-bot: I haz the power, 2016-08-25 00:11:29]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sam McNally, 2016-08-25 00:19:23]

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

[commit-bot: I haz the power, 2016-08-25 00:19:58]

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-08-25 01:04:00]

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

[commit-bot: I haz the power, 2016-08-25 01:04:01]

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

[Sam McNally, 2016-08-25 16:29:53]

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

[commit-bot: I haz the power, 2016-08-25 16:30:13]

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

[Sam McNally, 2016-08-25 18:07:28]

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

[commit-bot: I haz the power, 2016-08-25 18:08:06]

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

[Sam McNally, 2016-08-25 18:54:00]

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

[commit-bot: I haz the power, 2016-08-25 18:54:25]

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-08-25 20:37:46]

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

[commit-bot: I haz the power, 2016-08-25 20:37:47]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-08-25 20:42:24]

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

[commit-bot: I haz the power, 2016-08-25 20:43:40]

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-08-25 23:53:20]

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

[commit-bot: I haz the power, 2016-08-25 23:53:21]

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

[Sam McNally, 2016-08-26 16:31:58]

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

[commit-bot: I haz the power, 2016-08-26 16:32:53]

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

[Sam McNally, 2016-08-26 16:43:01]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-08-26 16:43:05]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-08-26 16:43:08]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-08-26 16:43:11]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-08-26 16:43:17]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2016-08-26 16:43:19]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2016-08-26 17:59:15]

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

[Sam McNally, 2016-08-26 17:59:31]

Patchset #1 (id:120001) has been deleted

[Sam McNally, 2016-08-26 17:59:35]

Patchset #1 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-08-26 17:59:55]

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

[Sam McNally, 2016-08-26 18:26:02]

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

[commit-bot: I haz the power, 2016-08-26 18:26:47]

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

[Sam McNally, 2016-08-26 19:26:18]

Patchset #1 (id:160001) has been deleted

[Sam McNally, 2016-08-26 20:22:22]

Description was changed from

==========
Add FlushForTesting to InterfacePtr and Binding.
==========

to

==========
Add FlushForTesting to InterfacePtr and Binding.

Previously, when unit testing a class that is a client of a mojo
interface, there was no reliable way to detect that no message was sent.
This adds a FlushForTesting() method to mojo message pipe bindings,
which can be used to wait for a round trip, after which any messages
already sent will have been delivered; this can be used to test for the
absence of messages sent in response to some stimulus.
==========

[Sam McNally, 2016-08-26 20:23:18]

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

[yzshen1, 2016-08-26 21:07:07]

https://codereview.chromium.org/2280483002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/binding_state.cc (right):

https://codereview.chromium.org/2280483002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/binding_state.cc:129: ControlMessageProxy
control_message_proxy(endpoint_client_.get());
Previously we only handled interface control messages at the same side of the
interface. So I put the proxy/handler inside the interface proxy/stub.

Now that we also have to handle it at the opposite side, does it make sense to
move control message proxy/handler into interface_endpoint_client (and Router in
the non-associated case, although I wish I could remove Router soon).

[commit-bot: I haz the power, 2016-08-26 21:16:26]

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

[commit-bot: I haz the power, 2016-08-26 21:16:27]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-08-29 05:05:03]

Patchset #2 (id:200001) has been deleted

[Sam McNally, 2016-08-29 05:06:19]

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

[commit-bot: I haz the power, 2016-08-29 05:06:37]

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

[Sam McNally, 2016-08-29 06:39:24]

https://codereview.chromium.org/2280483002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/binding_state.cc (right):

https://codereview.chromium.org/2280483002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/binding_state.cc:129: ControlMessageProxy
control_message_proxy(endpoint_client_.get());
On 2016/08/26 21:07:07, yzshen1 wrote:
> Previously we only handled interface control messages at the same side of the
> interface. So I put the proxy/handler inside the interface proxy/stub.
> 
> Now that we also have to handle it at the opposite side, does it make sense to
> move control message proxy/handler into interface_endpoint_client (and Router
in
> the non-associated case, although I wish I could remove Router soon). 

Done.

[commit-bot: I haz the power, 2016-08-29 06:47:27]

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

[commit-bot: I haz the power, 2016-08-29 06:47:28]

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

[yzshen1, 2016-08-29 23:24:58]

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/binding_set.h (right):

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/binding_set.h:101: binding->FlushForTesting();
I guess this won't work because bindings_ is a map so |binding| is a pair.

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h (right):

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h:72: if
(encountered_error() || !proxy_)
Does it make sense to remove this check? (Currently it is done at multiple
places.)

If |endpoint_client_| is null, then it is the user's responsibility to avoid
calling this method, just like RequireVersion above.

And we can let control_message_proxy() itself checks whether error has been
encountered.

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h:96:
std::move(runner), 0u));
Please comment on why we use 0 here. It might not be obvious to the readers.

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h:140: void
OnConnectionError() {
I think this is not used.

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/interface_endpoint_client.cc (right):

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:156:
filters_.Append(base::MakeUnique<internal::ControlMessageValidator>());
I took a different approach for pipe-control messages: I didn't add a validator
because that means one extra layer of call for every message, while control
messages are extremely rare.

Instead I explicitly validate control message payload in
pipe_control_message_handler (didn't do it also in pipe_control_message_proxy
because we don't have any response message in that case).

Does that make sense? (I might be a little paranoid. :))

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:294: if
(!incoming_receiver_ &&
nit: maybe we could remove this check and let it crash in that case. It should
never happen. (and also line 332)

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/router.cc:261: if (!incoming_receiver_ &&
Maybe we don't need this check (and also line 303)

[Sam McNally, 2016-08-30 01:07:31]

Patchset #4 (id:260001) has been deleted

[Sam McNally, 2016-08-30 01:15:24]

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

[Sam McNally, 2016-08-30 01:19:17]

Patchset #4 (id:280001) has been deleted

[commit-bot: I haz the power, 2016-08-30 01:59:09]

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

[Sam McNally, 2016-08-30 03:05:52]

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/binding_set.h (right):

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/binding_set.h:101: binding->FlushForTesting();
On 2016/08/29 23:24:58, yzshen1 wrote:
> I guess this won't work because bindings_ is a map so |binding| is a pair.

It worked just fine last week ;)

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h (right):

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h:72: if
(encountered_error() || !proxy_)
On 2016/08/29 23:24:58, yzshen1 wrote:
> Does it make sense to remove this check? (Currently it is done at multiple
> places.)
> 
> If |endpoint_client_| is null, then it is the user's responsibility to avoid
> calling this method, just like RequireVersion above.
> 
> And we can let control_message_proxy() itself checks whether error has been
> encountered.

Done.

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h:96:
std::move(runner), 0u));
On 2016/08/29 23:24:58, yzshen1 wrote:
> Please comment on why we use 0 here. It might not be obvious to the readers.

Done.

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h:140: void
OnConnectionError() {
On 2016/08/29 23:24:58, yzshen1 wrote:
> I think this is not used.

Done.

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/interface_endpoint_client.cc (right):

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:156:
filters_.Append(base::MakeUnique<internal::ControlMessageValidator>());
On 2016/08/29 23:24:58, yzshen1 wrote:
> I took a different approach for pipe-control messages: I didn't add a
validator
> because that means one extra layer of call for every message, while control
> messages are extremely rare.
> 
> Instead I explicitly validate control message payload in
> pipe_control_message_handler (didn't do it also in pipe_control_message_proxy
> because we don't have any response message in that case).
> 
> Does that make sense? (I might be a little paranoid. :))

Done.

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:294: if
(!incoming_receiver_ &&
On 2016/08/29 23:24:58, yzshen1 wrote:
> nit: maybe we could remove this check and let it crash in that case. It should
> never happen. (and also line 332)

Done. This should never happen because validation will block messages that are
neither responses nor control messages when |!incoming_receiver_|, right?

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/2280483002/diff/240001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/router.cc:261: if (!incoming_receiver_ &&
On 2016/08/29 23:24:58, yzshen1 wrote:
> Maybe we don't need this check (and also line 303)

Done.

[commit-bot: I haz the power, 2016-08-30 04:51:30]

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

[commit-bot: I haz the power, 2016-08-30 04:51:31]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-08-31 17:06:47]

LGTM with a few nits. 


Thanks for making this change!

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h:94: // The version
is only queried from the client so the value passed here
style nit: please move to comment out of the statement ( above line 90)

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/control_message_handler.cc (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/control_message_handler.cc:26: case
interface_control::kRunMessageId:
We should also do ValidateMessageIsRequestExpectingResponse

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/control_message_proxy.cc:29: switch
(message->header()->name) {
we should also do ValidateMessageIsResponse

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_ptr_state.h:164: // The version is only
queried from the client so the value passed here
style nit: please move it out of the statement. (and line 323)

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/associated_interface_unittest.cc (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:749: // Bind to
the same pipe two associated interfaces, whose implementation lives
This comment doesn't apply.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:788: // at
different ends. Test that the two don't interfere with each other.
ditto

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:815: // Bind to
the same pipe two associated interfaces, whose implementation lives
ditto

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:845: // Bind to
the same pipe two associated interfaces, whose implementation lives
ditto

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:906:
binding.FlushForTesting();
Out of curiosity: why flush twice here (and line 878)?

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:749: // Suppose this is
instantiated in a process that has pipe1_.
I guess this is an outdated comment (here and elsewhere in this file)

[Ken Rockot(use gerrit already), 2016-08-31 19:05:00]

lgtm % yuzhu's nits

[Sam McNally, 2016-09-01 00:08:07]

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

[commit-bot: I haz the power, 2016-09-01 00:08:28]

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

[Sam McNally, 2016-09-01 00:44:35]

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

[Sam McNally, 2016-09-01 00:44:37]

+dcheng for the mojom

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h:94: // The version
is only queried from the client so the value passed here
On 2016/08/31 17:06:46, yzshen1 wrote:
> style nit: please move to comment out of the statement ( above line 90)

Done.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/control_message_handler.cc (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/control_message_handler.cc:26: case
interface_control::kRunMessageId:
On 2016/08/31 17:06:46, yzshen1 wrote:
> We should also do ValidateMessageIsRequestExpectingResponse
> 

Done.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/control_message_proxy.cc:29: switch
(message->header()->name) {
On 2016/08/31 17:06:47, yzshen1 wrote:
> we should also do ValidateMessageIsResponse

Done.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_ptr_state.h:164: // The version is only
queried from the client so the value passed here
On 2016/08/31 17:06:47, yzshen1 wrote:
> style nit: please move it out of the statement. (and line 323)

Done.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/associated_interface_unittest.cc (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:749: // Bind to
the same pipe two associated interfaces, whose implementation lives
On 2016/08/31 17:06:47, yzshen1 wrote:
> This comment doesn't apply.

Done.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:788: // at
different ends. Test that the two don't interfere with each other.
On 2016/08/31 17:06:47, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:815: // Bind to
the same pipe two associated interfaces, whose implementation lives
On 2016/08/31 17:06:47, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:845: // Bind to
the same pipe two associated interfaces, whose implementation lives
On 2016/08/31 17:06:47, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/associated_interface_unittest.cc:906:
binding.FlushForTesting();
On 2016/08/31 17:06:47, yzshen1 wrote:
> Out of curiosity: why flush twice here (and line 878)?

The flush and the request are sent from opposite ends of the pipe so after the
first flush, we are only guaranteed that the request has been delivered.

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc (right):

https://codereview.chromium.org/2280483002/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:749: // Suppose this is
instantiated in a process that has pipe1_.
On 2016/08/31 17:06:47, yzshen1 wrote:
> I guess this is an outdated comment (here and elsewhere in this file)

Done.

[commit-bot: I haz the power, 2016-09-01 01:26:55]

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

[commit-bot: I haz the power, 2016-09-01 01:26:56]

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

[dcheng, 2016-09-01 08:19:00]

mojom lgtm

I looked over the other files as well, and the changes seem reasonable. That
being said, I feel like I don't really understand what it's doing and how all
the different mojo validation bits work together. IPC was "simpler" in this
respect (if you ignore the fact that it was a soup of macros and templates). So
maybe it'd be nice to document the internal generated bindings a little more,
not sure... then again, it might just get out of date really quickly. But maybe
it'd be of benefit to future maintenance?

https://codereview.chromium.org/2280483002/diff/320001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right):

https://codereview.chromium.org/2280483002/diff/320001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/control_message_proxy.cc:167:
run_loop_quit_closure_.Run();
This could use base::ResetAndReturn().Run()

[Sam McNally, 2016-09-01 08:27:36]

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

[commit-bot: I haz the power, 2016-09-01 08:27:46]

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

[Sam McNally, 2016-09-01 08:35:34]

https://codereview.chromium.org/2280483002/diff/320001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right):

https://codereview.chromium.org/2280483002/diff/320001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/control_message_proxy.cc:167:
run_loop_quit_closure_.Run();
On 2016/09/01 08:19:00, dcheng wrote:
> This could use base::ResetAndReturn().Run()

Done.

[commit-bot: I haz the power, 2016-09-01 09:44:08]

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

[commit-bot: I haz the power, 2016-09-01 09:44:09]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-01 09:48:22]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-09-01 09:48:22]

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

[commit-bot: I haz the power, 2016-09-01 09:48:33]

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-09-01 09:53:16]

Description was changed from

==========
Add FlushForTesting to InterfacePtr and Binding.

Previously, when unit testing a class that is a client of a mojo
interface, there was no reliable way to detect that no message was sent.
This adds a FlushForTesting() method to mojo message pipe bindings,
which can be used to wait for a round trip, after which any messages
already sent will have been delivered; this can be used to test for the
absence of messages sent in response to some stimulus.
==========

to

==========
Add FlushForTesting to InterfacePtr and Binding.

Previously, when unit testing a class that is a client of a mojo
interface, there was no reliable way to detect that no message was sent.
This adds a FlushForTesting() method to mojo message pipe bindings,
which can be used to wait for a round trip, after which any messages
already sent will have been delivered; this can be used to test for the
absence of messages sent in response to some stimulus.
==========

[commit-bot: I haz the power, 2016-09-01 09:53:18]

Committed patchset #6 (id:340001)

[commit-bot: I haz the power, 2016-09-01 09:55:23]

Description was changed from

==========
Add FlushForTesting to InterfacePtr and Binding.

Previously, when unit testing a class that is a client of a mojo
interface, there was no reliable way to detect that no message was sent.
This adds a FlushForTesting() method to mojo message pipe bindings,
which can be used to wait for a round trip, after which any messages
already sent will have been delivered; this can be used to test for the
absence of messages sent in response to some stimulus.
==========

to

==========
Add FlushForTesting to InterfacePtr and Binding.

Previously, when unit testing a class that is a client of a mojo
interface, there was no reliable way to detect that no message was sent.
This adds a FlushForTesting() method to mojo message pipe bindings,
which can be used to wait for a round trip, after which any messages
already sent will have been delivered; this can be used to test for the
absence of messages sent in response to some stimulus.

Committed: https://crrev.com/462458f4b533d44bfad95ea07dc99257eaf028cb
Cr-Commit-Position: refs/heads/master@{#415920}
==========

[commit-bot: I haz the power, 2016-09-01 09:55:25]

Patchset 6 (id:??) landed as
https://crrev.com/462458f4b533d44bfad95ea07dc99257eaf028cb
Cr-Commit-Position: refs/heads/master@{#415920}