Mojo C++ bindings: InterfacePtr<T> and Binding<T> use MultiplexRouter when T passes associated inte…

Mojo C++ bindings: InterfacePtr<T> and Binding<T> use MultiplexRouter when T passes associated interface pointers/requests.

BUG=546067
Committed: https://crrev.com/0b5628a30a249184528900b0d3f3eb81a83f8c39
Cr-Commit-Position: refs/heads/master@{#361590}

[yzshen1, 2015-11-24 18:34:54]

yzshen@chromium.org changed reviewers:
+ sky@chromium.org

[yzshen1, 2015-11-24 18:34:54]

Hi, Scott.

Would you please take a look? Thanks!

[sky, 2015-11-24 22:20:46]

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/binding_state.h (right):

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:30: template <typename Interface,
bool kUsesMultiplexRouter>
kUsesMultiplexRouter isn't a constant. use_multiple_router would be better.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:34: class BindingState<Interface,
false> {
Add a description.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:57: void
PauseIncomingMethodCallProcessing() {
What will the semantics of pausing/resuming be with associated pipes. You should
document this.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right):

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_ptr_state.h:31: template <typename
Interface, bool kUsesMultiplexRouter>
Same naming comment here.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_ptr_state.h:35: class
InterfacePtrState<Interface, false> {
Again, please add descriptions for all classes.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_ptr_state.h:69:
static_cast<ControlMessageProxy*>(proxy_)->QueryVersion(callback_wrapper);
How do you know the callback happens while this is still valid?
I assume this code should only be called once as well. It would be nice if you
DCHECKd to that effect.

[yzshen1, 2015-11-25 00:26:36]

Thanks, Scott!

Please take another look.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/binding_state.h (right):

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:30: template <typename Interface,
bool kUsesMultiplexRouter>
On 2015/11/24 22:20:45, sky wrote:
> kUsesMultiplexRouter isn't a constant. use_multiple_router would be better.

Done.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:34: class BindingState<Interface,
false> {
On 2015/11/24 22:20:45, sky wrote:
> Add a description.

Done.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:57: void
PauseIncomingMethodCallProcessing() {
On 2015/11/24 22:20:45, sky wrote:
> What will the semantics of pausing/resuming be with associated pipes. You
should
> document this.

Documented in binding.h and also interface_ptr.h that they are not supported if
|Interface| has methods to pass associated interface pointers/requests.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right):

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_ptr_state.h:31: template <typename
Interface, bool kUsesMultiplexRouter>
On 2015/11/24 22:20:45, sky wrote:
> Same naming comment here.

Done.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_ptr_state.h:35: class
InterfacePtrState<Interface, false> {
On 2015/11/24 22:20:45, sky wrote:
> Again, please add descriptions for all classes.

Done.

https://codereview.chromium.org/1473273003/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_ptr_state.h:69:
static_cast<ControlMessageProxy*>(proxy_)->QueryVersion(callback_wrapper);
On 2015/11/24 22:20:45, sky wrote:
> How do you know the callback happens while this is still valid?
If this object goes away, the callback is not called because the router (as well
as the underlying MP) is closed. I have comments on line 60.

> I assume this code should only be called once as well. It would be nice if you
> DCHECKd to that effect.
This method is exposed to the user, the user is free to call it multiple times.

https://codereview.chromium.org/1473273003/diff/60001/mojo/logging/init_loggi...
File mojo/logging/init_logging.h (left):

https://codereview.chromium.org/1473273003/diff/60001/mojo/logging/init_loggi...
mojo/logging/init_logging.h:9: namespace logging {
I have to include this change. Otherwise, whenever this file is included, the
subsequent code inside the mojo namespace cannot use base/logging.h.

The reason is that the code in base/logging.h uses "logging::" which is
interpreted as "mojo::logging" if this file is included.

[sky, 2015-11-25 00:52:32]

LGTM

[yzshen1, 2015-11-25 05:09:27]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2015-11-25 05:09:49]

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

[commit-bot: I haz the power, 2015-11-25 05:58:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-25 05:59:20]

Patchset 5 (id:??) landed as
https://crrev.com/0b5628a30a249184528900b0d3f3eb81a83f8c39
Cr-Commit-Position: refs/heads/master@{#361590}