Mojo: bindings connector

Adds a class named Connector that connects a MessagePipe to a MessageReceiver.

MessageReceiver is the interface used by the generated proxies and stubs to transmit messages. Connector is responsible for reading and writing those messages on a MessagePipe. A Connector "is a" MessageReceiver so that it can accept outbound messages, and "has a" MessageReceiver so it can dispatch incoming messages.

R=viettrungluu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233372

[darin (slow to review), 2013-11-04 23:30:48]

I'll revise the CL description shortly.

[viettrungluu, 2013-11-05 22:45:29]

Just so I understand correctly, this doesn't include a "real" implementation of
BindingsSupport (that actually waits, etc.), right?

Otherwise (apart from nits), I think the code looks pretty reasonable. (I'm not
sure about the way |Message| works -- it looks like it'd be very easy to make
mistakes for it. If we had C++11, we could make it movable-but-not-copyable and
still use it with the STL containers, which would be nicer. Alas.)

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/bindings_support.cc (right):

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
mojo/public/bindings/lib/bindings_support.cc:8: #include <stdlib.h>
stddef.h instead?

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/connector.cc (right):

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
mojo/public/bindings/lib/connector.cc:80: rv = ReadMessage(message_pipe_,
I wonder if it wouldn't be better (mostly for performance) to start off with a
default size (# of bytes and handles) and possibly complete the read
immediately. (It'd waste some memory, but then we wouldn't need to make two
"system calls". For further optimization, we could preallocate default-size
chunks, and even return them to that pool when done with them. In that case,
probably Accept() should be made to take ownership of the message.)

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/message.cc (right):

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
mojo/public/bindings/lib/message.cc:7: #include <stdlib.h>
stddef.h for NULL?

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
mojo/public/bindings/lib/message.cc:20: std::swap(data, other->data);
#include <algorithm> for swap?

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/message.h (right):

https://codereview.chromium.org/54743003/diff/470001/mojo/public/bindings/lib...
mojo/public/bindings/lib/message.h:38: };
Probably we want to make Message noncopyable, right?

https://codereview.chromium.org/54743003/diff/470001/mojo/public/tests/bindin...
File mojo/public/tests/bindings_connector_unittest.cc (right):

https://codereview.chromium.org/54743003/diff/470001/mojo/public/tests/bindin...
mojo/public/tests/bindings_connector_unittest.cc:8: #include <list>
<queue> instead?

https://codereview.chromium.org/54743003/diff/470001/mojo/public/tests/bindin...
mojo/public/tests/bindings_connector_unittest.cc:31:
queue_.front().Swap(message);
assert nonempty?

[darin (slow to review), 2013-11-06 05:05:46]

On Tue, Nov 5, 2013 at 2:45 PM, <viettrungluu@chromium.org> wrote:

> Just so I understand correctly, this doesn't include a "real"
> implementation of
> BindingsSupport (that actually waits, etc.), right?
>

Yes, that will be based on mojo::shell::HandleWatcher, which needs to be
moved out of mojo/shell.



>
> Otherwise (apart from nits), I think the code looks pretty reasonable.
> (I'm not
> sure about the way |Message| works -- it looks like it'd be very easy to
> make
> mistakes for it. If we had C++11, we could make it
> movable-but-not-copyable and
> still use it with the STL containers, which would be nicer. Alas.)
>

Yeah, exactly. I share your concerns.


>
>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/bindings_support.cc
> File mojo/public/bindings/lib/bindings_support.cc (right):
>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/bindings_support.cc#newcode8
> mojo/public/bindings/lib/bindings_support.cc:8: #include <stdlib.h>
> stddef.h instead?
>
>
Done.


> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/connector.cc
> File mojo/public/bindings/lib/connector.cc (right):
>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/connector.cc#newcode80
> mojo/public/bindings/lib/connector.cc:80: rv =
> ReadMessage(message_pipe_,
> I wonder if it wouldn't be better (mostly for performance) to start off
> with a default size (# of bytes and handles) and possibly complete the
> read immediately. (It'd waste some memory, but then we wouldn't need to
> make two "system calls". For further optimization, we could preallocate
> default-size chunks, and even return them to that pool when done with
> them. In that case, probably Accept() should be made to take ownership
> of the message.)
>

Good ideas. I'd like to explore this at a later time once more of the
system is in place and its performance can be measured.



>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/message.cc
> File mojo/public/bindings/lib/message.cc (right):
>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/message.cc#newcode7
> mojo/public/bindings/lib/message.cc:7: #include <stdlib.h>
> stddef.h for NULL?
>

Also need definition of free().


>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/message.cc#newcode20
> mojo/public/bindings/lib/message.cc:20: std::swap(data, other->data);
> #include <algorithm> for swap?
>
>
Done.



> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/message.h
> File mojo/public/bindings/lib/message.h (right):
>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/bindings/lib/message.h#newcode38
> mojo/public/bindings/lib/message.h:38: };
> Probably we want to make Message noncopyable, right?
>

Good catch. Done.


>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/tests/bindings_connector_unittest.cc
> File mojo/public/tests/bindings_connector_unittest.cc (right):
>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/tests/bindings_connector_unittest.cc#newcode8
> mojo/public/tests/bindings_connector_unittest.cc:8: #include <list>
> <queue> instead?
>

Deleted that line.


>
> https://codereview.chromium.org/54743003/diff/470001/mojo/
> public/tests/bindings_connector_unittest.cc#newcode31
> mojo/public/tests/bindings_connector_unittest.cc:31:
> queue_.front().Swap(message);
> assert nonempty?
>

Done.

Note, I realized that connector.cc and bindings_connector_unittest.cc had
some common code for keeping a queue of Message objects. I factored that
out into bindings/lib/message_queue.{h,cc} so it could be shared.

Thanks for the review,
-Darin



>
> https://codereview.chromium.org/54743003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2013-11-06 18:29:33]

LGTM w/nits.

https://codereview.chromium.org/54743003/diff/650001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/message_queue.h (right):

https://codereview.chromium.org/54743003/diff/650001/mojo/public/bindings/lib...
mojo/public/bindings/lib/message_queue.h:13: class Message;
nit: blank line above?

https://codereview.chromium.org/54743003/diff/650001/mojo/public/bindings/lib...
mojo/public/bindings/lib/message_queue.h:21: Message* Peek() const;
Should this return a const Message* (or const Message&?), since it'd be weird
for the caller to be able to modify/swap out the contents of the peeked message.

https://codereview.chromium.org/54743003/diff/650001/mojo/public/bindings/lib...
mojo/public/bindings/lib/message_queue.h:23: void Push(Message* message);
It might be good to have a comment indicating that |Push()| takes ownership of
the contents of |message| (but not of |message| itself), though maybe that's
clear due to the way that |Message| works.

https://codereview.chromium.org/54743003/diff/650001/mojo/public/bindings/lib...
mojo/public/bindings/lib/message_queue.h:29: std::queue<Message*> queue_;
It's kind of sad that with Message being noncopyable, you end up having to
heap-allocate them to put them into a C++03 STL container. I don't see a good
way to avoid this though (short of C++11).... :(

[darin (slow to review), 2013-11-06 18:47:33]

On Wed, Nov 6, 2013 at 10:29 AM, <viettrungluu@chromium.org> wrote:

> LGTM w/nits.
>
>
> https://codereview.chromium.org/54743003/diff/650001/mojo/
> public/bindings/lib/message_queue.h
> File mojo/public/bindings/lib/message_queue.h (right):
>
> https://codereview.chromium.org/54743003/diff/650001/mojo/
> public/bindings/lib/message_queue.h#newcode13
> mojo/public/bindings/lib/message_queue.h:13: class Message;
> nit: blank line above?
>
>
Fixed.



> https://codereview.chromium.org/54743003/diff/650001/mojo/
> public/bindings/lib/message_queue.h#newcode21
> mojo/public/bindings/lib/message_queue.h:21: Message* Peek() const;
> Should this return a const Message* (or const Message&?), since it'd be
> weird for the caller to be able to modify/swap out the contents of the
> peeked message.
>

Yes, good idea.


>
> https://codereview.chromium.org/54743003/diff/650001/mojo/
> public/bindings/lib/message_queue.h#newcode23
> mojo/public/bindings/lib/message_queue.h:23: void Push(Message*
> message);
> It might be good to have a comment indicating that |Push()| takes
> ownership of the contents of |message| (but not of |message| itself),
> though maybe that's clear due to the way that |Message| works.
>

Will do.


>
> https://codereview.chromium.org/54743003/diff/650001/mojo/
> public/bindings/lib/message_queue.h#newcode29
> mojo/public/bindings/lib/message_queue.h:29: std::queue<Message*>
> queue_;
> It's kind of sad that with Message being noncopyable, you end up having
> to heap-allocate them to put them into a C++03 STL container. I don't
> see a good way to avoid this though (short of C++11).... :(
>
>
Yeah, indeed. It was a brain-fart that I didn't write the code this way
from the start :-/

-Darin



> https://codereview.chromium.org/54743003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2013-11-06 21:23:33]

Committed patchset #13 manually as r233372 (presubmit successful).