Mojo C++ bindings: sync call support for associated interfaces and master interfaces

Mojo C++ bindings: sync call support for associated interfaces and master interfaces

BUG=577699
Committed: https://crrev.com/dca5a33b23f119021172866a5975d855f123dfd7
Cr-Commit-Position: refs/heads/master@{#383754}

[yzshen1, 2016-03-28 20:41:42]

Description was changed from

==========
[Draft] Sync Call support for associated interfaces and master interfaces.

BUG=
==========

to

==========
Mojo C++ bindings: sync call support for associated interfaces and master
interfaces

BUG=577699
==========

[yzshen1, 2016-03-28 20:41:42]

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

[yzshen1, 2016-03-28 20:42:27]

Hi, John and Ken.

Would you please take a look at this change? Thanks!

[jam, 2016-03-28 21:03:59]

On 2016/03/28 20:42:27, yzshen1 wrote:
> Hi, John and Ken.
> 
> Would you please take a look at this change? Thanks!

lgtm. i'm not really familiar with multiplex_router.cc, so i'm basing my lgtm
off the unit test :)

[Ken Rockot(use gerrit already), 2016-03-29 06:36:50]

Nice! LGTM with some nits.

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

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:101: //
----------------------------------------------------------------------------
nit: vertical space above?

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (right):

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:76: // It should be called on
the same thread as the corresponding AttachClient()
nit: should = must? Also perhaps "This" or "This method" instead of "It"

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:98:
WriteMessageRaw(sync_message_event_sender_.get(), &dummy_message, 1,
nit: It is perfectly legal to write a zero-length message and pass nullptr for
the bytes. I don't think this was true in the old EDK but it's supported now and
the public API does not forbid it.

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:208: MojoResult result =
ReadMessageRaw(sync_message_event_receiver_.get(),
nit: It's fine to pass nullptr for the read buffer when MAY_DISCARD is set. A
message will be read and discarded if available and the result will be OK.

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.h (right):

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.h:81: // Attaches an client to the
specified endpoint to send and receive messages.
nit: While you're here, "Attaches a client"

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.h:82: // The returned object is
still owned by the router. It should only be used on
nit: should = must?

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/sync_method_unittest.cc (right):

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/sync_method_unittest.cc:135: class
TestSyncAssoImpl : public TestSync, public TestSyncCommonImpl {
nit: This name is a bit weird. Could it just be TestSyncAssociatedImpl?

[yzshen1, 2016-03-29 16:19:01]

Thanks!

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

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:101: //
----------------------------------------------------------------------------
On 2016/03/29 06:36:49, Ken Rockot wrote:
> nit: vertical space above?

Done.

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (right):

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:76: // It should be called on
the same thread as the corresponding AttachClient()
On 2016/03/29 06:36:49, Ken Rockot wrote:
> nit: should = must? Also perhaps "This" or "This method" instead of "It"

Done. English is hard. :)

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:98:
WriteMessageRaw(sync_message_event_sender_.get(), &dummy_message, 1,
On 2016/03/29 06:36:49, Ken Rockot wrote:
> nit: It is perfectly legal to write a zero-length message and pass nullptr for
> the bytes. I don't think this was true in the old EDK but it's supported now
and
> the public API does not forbid it.

Done.

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:208: MojoResult result =
ReadMessageRaw(sync_message_event_receiver_.get(),
On 2016/03/29 06:36:49, Ken Rockot wrote:
> nit: It's fine to pass nullptr for the read buffer when MAY_DISCARD is set. A
> message will be read and discarded if available and the result will be OK.

Done.

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.h (right):

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.h:81: // Attaches an client to the
specified endpoint to send and receive messages.
On 2016/03/29 06:36:49, Ken Rockot wrote:
> nit: While you're here, "Attaches a client"

Done.

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.h:82: // The returned object is
still owned by the router. It should only be used on
On 2016/03/29 06:36:49, Ken Rockot wrote:
> nit: should = must?

Done.

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/sync_method_unittest.cc (right):

https://codereview.chromium.org/1823683006/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/sync_method_unittest.cc:135: class
TestSyncAssoImpl : public TestSync, public TestSyncCommonImpl {
On 2016/03/29 06:36:50, Ken Rockot wrote:
> nit: This name is a bit weird. Could it just be TestSyncAssociatedImpl?

Done.

[yzshen1, 2016-03-29 16:19:07]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-03-29 16:19:07]

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

[commit-bot: I haz the power, 2016-03-29 16:19:17]

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

[commit-bot: I haz the power, 2016-03-29 17:33:14]

Description was changed from

==========
Mojo C++ bindings: sync call support for associated interfaces and master
interfaces

BUG=577699
==========

to

==========
Mojo C++ bindings: sync call support for associated interfaces and master
interfaces

BUG=577699
==========

[commit-bot: I haz the power, 2016-03-29 17:33:15]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-29 17:34:12]

Description was changed from

==========
Mojo C++ bindings: sync call support for associated interfaces and master
interfaces

BUG=577699
==========

to

==========
Mojo C++ bindings: sync call support for associated interfaces and master
interfaces

BUG=577699

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

[commit-bot: I haz the power, 2016-03-29 17:34:13]

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