HostMessageDispatcher to parse control messages

HostMessageDispatcher to parse control messages

Changed MessageReader and MessageDecoder to support parsing in
HostMessageDispatcher.

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64283

[Alpha Left Google, 2010-10-21 00:55:11]

Finished the code that decoders / parses messages and have a dispatch method.

I'm not sure if this is the right way to do things.. but this looks really ugly
to me.. :(

[Sergey Ulanov, 2010-10-21 01:31:43]

Some nits.

I think it would be much uglier if you were using macros here.

http://codereview.chromium.org/4017002/diff/5001/6007
File remoting/protocol/chromoting_connection.h (right):

http://codereview.chromium.org/4017002/diff/5001/6007#newcode47
remoting/protocol/chromoting_connection.h:47: virtual net::Socket*
GetControlChannel() = 0;
Can you please put it first: control, event, video, in this order.

http://codereview.chromium.org/4017002/diff/5001/6010
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/4017002/diff/5001/6010#newcode54
remoting/protocol/host_message_dispatcher.cc:54: message_loop_proxy_->PostTask(
Do we really need to dispatch messages on a different thread? I think that in
most cases message handlers will just post a task on another thread anyway, so
it should be ok to just call message handler from the jingle thread.

http://codereview.chromium.org/4017002/diff/5001/6012
File remoting/protocol/jingle_chromoting_connection.cc (right):

http://codereview.chromium.org/4017002/diff/5001/6012#newcode89
remoting/protocol/jingle_chromoting_connection.cc:89: return NULL;
I think will make it crash when you create MessageDispatcher that tries to use
this channel.

http://codereview.chromium.org/4017002/diff/5001/6013
File remoting/protocol/jingle_chromoting_connection.h (right):

http://codereview.chromium.org/4017002/diff/5001/6013#newcode1
remoting/protocol/jingle_chromoting_connection.h:1: 
remove this empty line.

http://codereview.chromium.org/4017002/diff/5001/6013#newcode42
remoting/protocol/jingle_chromoting_connection.h:42: virtual net::Socket*
GetControlChannel();
order: control, event, video

http://codereview.chromium.org/4017002/diff/5001/6014
File remoting/protocol/jingle_chromoting_connection_unittest.cc (right):

http://codereview.chromium.org/4017002/diff/5001/6014#newcode247
remoting/protocol/jingle_chromoting_connection_unittest.cc:247: return
connection->GetEventChannel();
I have a change pending where I also renamed events->event.

http://codereview.chromium.org/4017002/diff/5001/6016
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4017002/diff/5001/6016#newcode25
remoting/protocol/message_decoder.h:25: class HostEventMessage;
I don't think we really need HostEventMessage. What will host send in the event
channel?

[awong, 2010-10-21 22:22:29]

Between the last CL and this one, I'm starting to get a little lost on the final
design of the classes and how the data flows.  Maybe we should diagram this out
and document it in a comment?

http://codereview.chromium.org/4017002/diff/5001/6004
File remoting/host/client_connection.h (right):

http://codereview.chromium.org/4017002/diff/5001/6004#newcode110
remoting/host/client_connection.h:110: ChromotingClientMessageReader
events_reader_;
event_reader_

http://codereview.chromium.org/4017002/diff/5001/6006
File remoting/proto/control.proto (right):

http://codereview.chromium.org/4017002/diff/5001/6006#newcode21
remoting/proto/control.proto:21: optional SuggestScreenResolutionRequest
suggest_screen_resolution_request = 1;
Should we just use a single ScreenResolutionRequest on both sides?

http://codereview.chromium.org/4017002/diff/5001/6010
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/4017002/diff/5001/6010#newcode53
remoting/protocol/host_message_dispatcher.cc:53: new
ControlMessageWrapper(message);
I would have expected the wrapper to be created one level lower, in the
DispatchControlMessageTask.  If we do this, we won't even need to expose the
wrappers in the header files.

http://codereview.chromium.org/4017002/diff/5001/6010#newcode54
remoting/protocol/host_message_dispatcher.cc:54: message_loop_proxy_->PostTask(
On 2010/10/21 01:31:43, sergeyu wrote:
> Do we really need to dispatch messages on a different thread? I think that in
> most cases message handlers will just post a task on another thread anyway, so
> it should be ok to just call message handler from the jingle thread. 

I think I agree.  The dispatch thread should be the network thread.  The handler
should not be allowed to do anything heavyweight directly.

http://codereview.chromium.org/4017002/diff/5001/6011
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/4017002/diff/5001/6011#newcode46
remoting/protocol/host_message_dispatcher.h:46: bool
Initialize(base::MessageLoopProxy* message_loop_proxy,
We seem to use Init() more than Initialize.  Shall we normalize on one of these?

Also, should init be asynchronous?

http://codereview.chromium.org/4017002/diff/5001/6011#newcode53
remoting/protocol/host_message_dispatcher.h:53: class EventMessageWrapper :
public base::RefCounted<EventMessageWrapper> {
I'm tempted to just suggest a templated class for this.

But I'm okay as is too since we only have 2 classes.

http://codereview.chromium.org/4017002/diff/5001/6011#newcode57
remoting/protocol/host_message_dispatcher.h:57: ClientEventMessage* message() {
return message_.get(); }
Make the method const?

http://codereview.chromium.org/4017002/diff/5001/6016
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4017002/diff/5001/6016#newcode25
remoting/protocol/message_decoder.h:25: class HostEventMessage;
On 2010/10/21 01:31:43, sergeyu wrote:
> I don't think we really need HostEventMessage. What will host send in the
event
> channel?

Yeah, let's remove it for now. I think we will actually probably need to add it
back in eventually (for instance, to handle numlock synchronization), but let's
add it when we need it.

http://codereview.chromium.org/4017002/diff/5001/6016#newcode80
remoting/protocol/message_decoder.h:80: // TODO(hclam): Remove this class.
Eventually this all just collapses into one function and the repeated code is
just removed, correct?

[Alpha Left Google, 2010-10-26 18:45:05]

updated this patch to use templates for MessageDecoder and MessageReader.

Also had to chnage MultiArrayInputStream because the code didn't handle
refcounting correctly.

[awong, 2010-10-26 22:05:57]

I think this looks much cleaner than the previous version. Thanks for taking the
time to work through all the template messes.

I'd still like some overview documentation explaining how the classes interact,
and how you expect people to use them.  I think I understand the design and
agree with it, but without the doc written up, I'm not 100% certain that we're
thinking the same thing...

http://codereview.chromium.org/4017002/diff/19001/20001
File remoting/base/multiple_array_input_stream.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20001#newcode29
remoting/base/multiple_array_input_stream.cc:29: // Reply with what is remaining
in the current buffer.
"Reply with the number of bytes remaining in the current buffer"

http://codereview.chromium.org/4017002/diff/19001/20002
File remoting/base/multiple_array_input_stream.h (right):

http://codereview.chromium.org/4017002/diff/19001/20002#newcode16
remoting/base/multiple_array_input_stream.h:16: class DrainableIOBuffer;
ordering.

http://codereview.chromium.org/4017002/diff/19001/20002#newcode22
remoting/base/multiple_array_input_stream.h:22: // backing arrays.
Move this to a file comment, and explain the subtle design decisions. 
Specifically, we should explain why this needs a DrainableIOBuffer, the expected
threading semantics, and the lifetime and object ownership issues.

http://codereview.chromium.org/4017002/diff/19001/20002#newcode34
remoting/base/multiple_array_input_stream.h:34: // This method
The second sentence "|buffer| is used as the base pointer of the data" doesn't
make much sense.  Isn't that implied by adding the buffer to the list?

For the last sentence, I would say "MultipleArrayInputStream retains |buffer|."

http://codereview.chromium.org/4017002/diff/19001/20020
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/4017002/diff/19001/20020#newcode46
remoting/protocol/host_message_dispatcher.h:46: // A ref counted wrapper of
protobuf message.
Please explain why this is necessary.

http://codereview.chromium.org/4017002/diff/19001/20020#newcode47
remoting/protocol/host_message_dispatcher.h:47: template <class T>
use typename instead of class.

http://codereview.chromium.org/4017002/diff/19001/20020#newcode67
remoting/protocol/host_message_dispatcher.h:67: template <class T>
Can this and the RefCountedMessage class go int he implementation file?

http://codereview.chromium.org/4017002/diff/19001/20020#newcode71
remoting/protocol/host_message_dispatcher.h:71: // that parses data on the
channel and then delegate the message to this
then delegate -> then delegates

http://codereview.chromium.org/4017002/diff/19001/20022
File remoting/protocol/jingle_chromoting_connection.h (right):

http://codereview.chromium.org/4017002/diff/19001/20022#newcode1
remoting/protocol/jingle_chromoting_connection.h:1: 
remove newline.

http://codereview.chromium.org/4017002/diff/19001/20024
File remoting/protocol/message_decoder.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20024#newcode25
remoting/protocol/message_decoder.cc:25: // Enqueue data received.
remove comment.  It just restates the function name.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode32
remoting/protocol/message_decoder.cc:32: // We have the value set to -1 for
checking later.
Why don't we just read directly into next_payload_?

Also remove the comment about setting to -1. Since you check it immedialy below,
it's pretty obvious.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode46
remoting/protocol/message_decoder.cc:46: // Create a MultipleArrayInputStream
for parsing.
Maybe explain the loop logic insead? Say somehting like add as many buffers as
necessary into the input stream until the next_payload_ number of bytes is
passed...except phrased better?

http://codereview.chromium.org/4017002/diff/19001/20024#newcode55
remoting/protocol/message_decoder.cc:55: // version of data pointer.
As we discussed yesterday, there should be a top-level file comemnt somewhere
explaining how all these classes work together, and that we're (ab)using
DrainableIOBuffer to handle the data pointer.   Then all these comments can
either be removed, or replaced with one comment that syas "read X to understand
why we do this".

http://codereview.chromium.org/4017002/diff/19001/20024#newcode64
remoting/protocol/message_decoder.cc:64: if (!buffer->BytesRemaining())
Since this isn't a true/false compare it to 0.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode73
remoting/protocol/message_decoder.cc:73: return header.length();
...this seems like a superfluous function doesn't it?

http://codereview.chromium.org/4017002/diff/19001/20024#newcode105
remoting/protocol/message_decoder.cc:105: if (GetHeaderSize(header) ==
kHeaderSize) {
Should this be a CHECK instead?

http://codereview.chromium.org/4017002/diff/19001/20025
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4017002/diff/19001/20025#newcode16
remoting/protocol/message_decoder.h:16: class IOBuffer;
ordering

http://codereview.chromium.org/4017002/diff/19001/20025#newcode18
remoting/protocol/message_decoder.h:18: }
closing namespace comment.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode30
remoting/protocol/message_decoder.h:30: // A protocol decoder is used to decode
data transmitted in the chromoting
This isn't quite a sufficient description of the class.

The goal of class documentation should be that someone should understand
basically how to code against it, and just as importantly, how it interacts with
other classes in the system.   The second point in particular usually doesn't
get expressed well in either implementation or implementation comments.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode39
remoting/protocol/message_decoder.h:39: void
ParseMessages(scoped_refptr<net::IOBuffer> data,
Comment explaining what this does.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode43
remoting/protocol/message_decoder.h:43: // Then try to parse one message until
we can't parse anymore.
"parse one message" -> "pares the next message"

http://codereview.chromium.org/4017002/diff/19001/20025#newcode53
remoting/protocol/message_decoder.h:53: typedef
std::deque<scoped_refptr<net::DrainableIOBuffer> > DataList;
BufferList seems more appropriate.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode62
remoting/protocol/message_decoder.h:62: // And finally it is parsing.
remove comment. Not useful.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode71
remoting/protocol/message_decoder.h:71: // Enqueue the IOBuffer received to the
internal queue and update internal
This comment basically repeats the function name.  remove?

http://codereview.chromium.org/4017002/diff/19001/20025#newcode73
remoting/protocol/message_decoder.h:73: void
EnqueueData(scoped_refptr<net::IOBuffer> data, int data_size);
Other places in the codebase, we use "Add" to mean "enqeuue".  Change to match?

Call it AddBuffer maybe?

http://codereview.chromium.org/4017002/diff/19001/20025#newcode75
remoting/protocol/message_decoder.h:75: // Create an input stream composed by
multiple arrays of data.
How about

"Create an input stream of the added data that can be used by the protobuf
library to parse out messages."

http://codereview.chromium.org/4017002/diff/19001/20025#newcode80
remoting/protocol/message_decoder.h:80: // data list. Return false if we don't
have enough data.
Explain how size is modified.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode85
remoting/protocol/message_decoder.h:85: // Count the number of bytes in
|data_list_| not read.
"Count the" -> "The"

http://codereview.chromium.org/4017002/diff/19001/20025#newcode88
remoting/protocol/message_decoder.h:88: // Stores the size of the next payload
if known.
How about combinng this comment with the one for next_payload_known_, and put it
above both variable declarations.

So something like

// |next_payload_| stores the side of the next payload if 
// |next_payload_known_| is true.  |next_payload_known_|
// is set to false after one payload is fully read.
int next_payload_;
bool next_payload_known_;

http://codereview.chromium.org/4017002/diff/19001/20026
File remoting/protocol/message_decoder_unittest.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20026#newcode65
remoting/protocol/message_decoder_unittest.cc:65: void
SimulateReadSequence(const int read_sequence[], int sequence_size) {
Careful...ASSERT_* won't end tests inside a function that's not a TEST or
TEST_F.

http://codereview.chromium.org/4017002/diff/19001/20026#newcode94
remoting/protocol/message_decoder_unittest.cc:94:
ASSERT_TRUE(message_list.size() > 0);
Did you want an EXPECT_TRUE?

http://codereview.chromium.org/4017002/diff/19001/20026#newcode124
remoting/protocol/message_decoder_unittest.cc:124: SimulateReadSequence(kReads,
arraysize(kReads));
FYI, I'm generally not a fan of having the various EXPECT_* statements sit
inside subfunctions in unittests. It messes up the error reporting cause
regardless of which test fails, the reported line number is where the function
is.

This will get especially confusing if you start adding multiple calls to the
function within one test case.

Is there a way to break this test down into smaller chunks?

I'll think about this more in a bit...need to leave soon.

http://codereview.chromium.org/4017002/diff/19001/20028
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/4017002/diff/19001/20028#newcode26
remoting/protocol/message_reader.h:26: // Internal namespace for a templated
class that decodes data received into
I don't think you need this comment. Nesting a namespace called "internal" is
pretty common.

[Sergey Ulanov, 2010-10-26 23:05:55]

Please merge with the changes I landed yesterday and today. They include some
things that you have here (e.g. control channel).

http://codereview.chromium.org/4017002/diff/19001/20003
File remoting/base/multiple_array_input_stream_unittest.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20003#newcode75
remoting/base/multiple_array_input_stream_unittest.cc:75: mstream->AddBuffer(new
net::StringIOBuffer(std::string(data, size)), size);
whoa, why std::string? new net::WrappedIOBuffer(data) seems to be more
appropriate here.

http://codereview.chromium.org/4017002/diff/19001/20020
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/4017002/diff/19001/20020#newcode73
remoting/protocol/host_message_dispatcher.h:73: scoped_ptr<MessageReader>
control_message_reader_;
I think it would be better if this was MessageReader<ClientControlMessage>.

http://codereview.chromium.org/4017002/diff/19001/20028
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/4017002/diff/19001/20028#newcode79
remoting/protocol/message_reader.h:79: template <class T>
Why not template the whole class instead of just this method?

[Alpha Left Google, 2010-10-27 00:27:36]

http://codereview.chromium.org/4017002/diff/19001/20001
File remoting/base/multiple_array_input_stream.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20001#newcode29
remoting/base/multiple_array_input_stream.cc:29: // Reply with what is remaining
in the current buffer.
On 2010/10/26 22:05:57, awong wrote:
> "Reply with the number of bytes remaining in the current buffer"

Done.

http://codereview.chromium.org/4017002/diff/19001/20002
File remoting/base/multiple_array_input_stream.h (right):

http://codereview.chromium.org/4017002/diff/19001/20002#newcode16
remoting/base/multiple_array_input_stream.h:16: class DrainableIOBuffer;
On 2010/10/26 22:05:57, awong wrote:
> ordering.

Done.

http://codereview.chromium.org/4017002/diff/19001/20002#newcode22
remoting/base/multiple_array_input_stream.h:22: // backing arrays.
On 2010/10/26 22:05:57, awong wrote:
> Move this to a file comment, and explain the subtle design decisions. 
> Specifically, we should explain why this needs a DrainableIOBuffer, the
expected
> threading semantics, and the lifetime and object ownership issues.

Done.

http://codereview.chromium.org/4017002/diff/19001/20002#newcode34
remoting/base/multiple_array_input_stream.h:34: // This method
On 2010/10/26 22:05:57, awong wrote:
> The second sentence "|buffer| is used as the base pointer of the data" doesn't
> make much sense.  Isn't that implied by adding the buffer to the list?
> 
> For the last sentence, I would say "MultipleArrayInputStream retains
|buffer|."

Done.

http://codereview.chromium.org/4017002/diff/19001/20003
File remoting/base/multiple_array_input_stream_unittest.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20003#newcode75
remoting/base/multiple_array_input_stream_unittest.cc:75: mstream->AddBuffer(new
net::StringIOBuffer(std::string(data, size)), size);
On 2010/10/26 23:05:55, sergeyu wrote:
> whoa, why std::string? new net::WrappedIOBuffer(data) seems to be more
> appropriate here.

io_buffer.h says that WrapperIOBuffer is the last resort. This just need an
extra copy in the test case.

http://codereview.chromium.org/4017002/diff/19001/20020
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/4017002/diff/19001/20020#newcode46
remoting/protocol/host_message_dispatcher.h:46: // A ref counted wrapper of
protobuf message.
On 2010/10/26 22:05:57, awong wrote:
> Please explain why this is necessary.

Done.

http://codereview.chromium.org/4017002/diff/19001/20020#newcode47
remoting/protocol/host_message_dispatcher.h:47: template <class T>
On 2010/10/26 22:05:57, awong wrote:
> use typename instead of class.

Done.

http://codereview.chromium.org/4017002/diff/19001/20020#newcode67
remoting/protocol/host_message_dispatcher.h:67: template <class T>
On 2010/10/26 22:05:57, awong wrote:
> Can this and the RefCountedMessage class go int he implementation file?

RefCountedMessage should be shared with client code. RefCountedMessage is
private, makes it hard to be a state method in .cc file. Can do it separately if
we share RefCountedMessage.

http://codereview.chromium.org/4017002/diff/19001/20020#newcode71
remoting/protocol/host_message_dispatcher.h:71: // that parses data on the
channel and then delegate the message to this
On 2010/10/26 22:05:57, awong wrote:
> then delegate -> then delegates

Done.

http://codereview.chromium.org/4017002/diff/19001/20020#newcode73
remoting/protocol/host_message_dispatcher.h:73: scoped_ptr<MessageReader>
control_message_reader_;
On 2010/10/26 23:05:55, sergeyu wrote:
> I think it would be better if this was MessageReader<ClientControlMessage>.

We'd have to restructure MessageReader again to 'templat'ize  the whole class.
We can do that separately.

http://codereview.chromium.org/4017002/diff/19001/20022
File remoting/protocol/jingle_chromoting_connection.h (right):

http://codereview.chromium.org/4017002/diff/19001/20022#newcode1
remoting/protocol/jingle_chromoting_connection.h:1: 
On 2010/10/26 22:05:57, awong wrote:
> remove newline.

Done.

http://codereview.chromium.org/4017002/diff/19001/20024
File remoting/protocol/message_decoder.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20024#newcode25
remoting/protocol/message_decoder.cc:25: // Enqueue data received.
On 2010/10/26 22:05:57, awong wrote:
> remove comment.  It just restates the function name.
> 

Done.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode32
remoting/protocol/message_decoder.cc:32: // We have the value set to -1 for
checking later.
On 2010/10/26 22:05:57, awong wrote:
> Why don't we just read directly into next_payload_?
> 
> Also remove the comment about setting to -1. Since you check it immedialy
below,
> it's pretty obvious.

see comments.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode46
remoting/protocol/message_decoder.cc:46: // Create a MultipleArrayInputStream
for parsing.
On 2010/10/26 22:05:57, awong wrote:
> Maybe explain the loop logic insead? Say somehting like add as many buffers as
> necessary into the input stream until the next_payload_ number of bytes is
> passed...except phrased better?

http://codereview.chromium.org/4017002/diff/19001/20024#newcode46
remoting/protocol/message_decoder.cc:46: // Create a MultipleArrayInputStream
for parsing.
On 2010/10/26 22:05:57, awong wrote:
> Maybe explain the loop logic insead? Say somehting like add as many buffers as
> necessary into the input stream until the next_payload_ number of bytes is
> passed...except phrased better?

Done.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode55
remoting/protocol/message_decoder.cc:55: // version of data pointer.
On 2010/10/26 22:05:57, awong wrote:
> As we discussed yesterday, there should be a top-level file comemnt somewhere
> explaining how all these classes work together, and that we're (ab)using
> DrainableIOBuffer to handle the data pointer.   Then all these comments can
> either be removed, or replaced with one comment that syas "read X to
understand
> why we do this".

Done.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode64
remoting/protocol/message_decoder.cc:64: if (!buffer->BytesRemaining())
On 2010/10/26 22:05:57, awong wrote:
> Since this isn't a true/false compare it to 0.

in many chromium code we treat 0 as false.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode73
remoting/protocol/message_decoder.cc:73: return header.length();
On 2010/10/26 22:05:57, awong wrote:
> ...this seems like a superfluous function doesn't it?

don't want to do static_cast<int>(header.length()); just to get around
converting size_t to int.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode105
remoting/protocol/message_decoder.cc:105: if (GetHeaderSize(header) ==
kHeaderSize) {
On 2010/10/26 22:05:57, awong wrote:
> Should this be a CHECK instead?

nope, we don't want critical failure if we can't parse.

http://codereview.chromium.org/4017002/diff/19001/20025
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4017002/diff/19001/20025#newcode16
remoting/protocol/message_decoder.h:16: class IOBuffer;
On 2010/10/26 22:05:57, awong wrote:
> ordering

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode18
remoting/protocol/message_decoder.h:18: }
On 2010/10/26 22:05:57, awong wrote:
> closing namespace comment.

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode30
remoting/protocol/message_decoder.h:30: // A protocol decoder is used to decode
data transmitted in the chromoting
On 2010/10/26 22:05:57, awong wrote:
> This isn't quite a sufficient description of the class.
> 
> The goal of class documentation should be that someone should understand
> basically how to code against it, and just as importantly, how it interacts
with
> other classes in the system.   The second point in particular usually doesn't
> get expressed well in either implementation or implementation comments.

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode39
remoting/protocol/message_decoder.h:39: void
ParseMessages(scoped_refptr<net::IOBuffer> data,
On 2010/10/26 22:05:57, awong wrote:
> Comment explaining what this does.

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode43
remoting/protocol/message_decoder.h:43: // Then try to parse one message until
we can't parse anymore.
On 2010/10/26 22:05:57, awong wrote:
> "parse one message" -> "pares the next message"

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode53
remoting/protocol/message_decoder.h:53: typedef
std::deque<scoped_refptr<net::DrainableIOBuffer> > DataList;
On 2010/10/26 22:05:57, awong wrote:
> BufferList seems more appropriate.

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode62
remoting/protocol/message_decoder.h:62: // And finally it is parsing.
On 2010/10/26 22:05:57, awong wrote:
> remove comment. Not useful.

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode71
remoting/protocol/message_decoder.h:71: // Enqueue the IOBuffer received to the
internal queue and update internal
On 2010/10/26 22:05:57, awong wrote:
> This comment basically repeats the function name.  remove?

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode73
remoting/protocol/message_decoder.h:73: void
EnqueueData(scoped_refptr<net::IOBuffer> data, int data_size);
On 2010/10/26 22:05:57, awong wrote:
> Other places in the codebase, we use "Add" to mean "enqeuue".  Change to
match?
> 
> Call it AddBuffer maybe?

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode75
remoting/protocol/message_decoder.h:75: // Create an input stream composed by
multiple arrays of data.
On 2010/10/26 22:05:57, awong wrote:
> How about
> 
> "Create an input stream of the added data that can be used by the protobuf
> library to parse out messages." 

removed.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode80
remoting/protocol/message_decoder.h:80: // data list. Return false if we don't
have enough data.
On 2010/10/26 22:05:57, awong wrote:
> Explain how size is modified.

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode85
remoting/protocol/message_decoder.h:85: // Count the number of bytes in
|data_list_| not read.
On 2010/10/26 22:05:57, awong wrote:
> "Count the" -> "The"

Done.

http://codereview.chromium.org/4017002/diff/19001/20025#newcode88
remoting/protocol/message_decoder.h:88: // Stores the size of the next payload
if known.
On 2010/10/26 22:05:57, awong wrote:
> How about combinng this comment with the one for next_payload_known_, and put
it
> above both variable declarations.
> 
> So something like
> 
> // |next_payload_| stores the side of the next payload if 
> // |next_payload_known_| is true.  |next_payload_known_|
> // is set to false after one payload is fully read.
> int next_payload_;
> bool next_payload_known_;

Done.

http://codereview.chromium.org/4017002/diff/19001/20026
File remoting/protocol/message_decoder_unittest.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20026#newcode65
remoting/protocol/message_decoder_unittest.cc:65: void
SimulateReadSequence(const int read_sequence[], int sequence_size) {
On 2010/10/26 22:05:57, awong wrote:
> Careful...ASSERT_* won't end tests inside a function that's not a TEST or
> TEST_F.

Done.

http://codereview.chromium.org/4017002/diff/19001/20026#newcode94
remoting/protocol/message_decoder_unittest.cc:94:
ASSERT_TRUE(message_list.size() > 0);
On 2010/10/26 22:05:57, awong wrote:
> Did you want an EXPECT_TRUE?

Done.

http://codereview.chromium.org/4017002/diff/19001/20026#newcode124
remoting/protocol/message_decoder_unittest.cc:124: SimulateReadSequence(kReads,
arraysize(kReads));
On 2010/10/26 22:05:57, awong wrote:
> FYI, I'm generally not a fan of having the various EXPECT_* statements sit
> inside subfunctions in unittests. It messes up the error reporting cause
> regardless of which test fails, the reported line number is where the function
> is.
> 
> This will get especially confusing if you start adding multiple calls to the
> function within one test case.
> 
> Is there a way to break this test down into smaller chunks?
> 
> I'll think about this more in a bit...need to leave soon.

You can have the simulate method return a value and then expect in TEST().
However I don't think it's important to fix in this patch. These added test
cases are for extra protection of the changes to message decoder.

http://codereview.chromium.org/4017002/diff/19001/20028
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/4017002/diff/19001/20028#newcode26
remoting/protocol/message_reader.h:26: // Internal namespace for a templated
class that decodes data received into
On 2010/10/26 22:05:57, awong wrote:
> I don't think you need this comment. Nesting a namespace called "internal" is
> pretty common.

Done.

http://codereview.chromium.org/4017002/diff/19001/20028#newcode79
remoting/protocol/message_reader.h:79: template <class T>
On 2010/10/26 23:05:55, sergeyu wrote:
> Why not template the whole class instead of just this method?

It require more careful 'templat'ization of this class. I don't think it matters
that much to what this patch is supposed to do.

[awong, 2010-10-27 02:09:35]

Mostly nits around comments, but 2 real coding issues.

http://codereview.chromium.org/4017002/diff/19001/20020
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/4017002/diff/19001/20020#newcode67
remoting/protocol/host_message_dispatcher.h:67: template <class T>
On 2010/10/27 00:27:36, Alpha wrote:
> On 2010/10/26 22:05:57, awong wrote:
> > Can this and the RefCountedMessage class go int he implementation file?
> 
> RefCountedMessage should be shared with client code. RefCountedMessage is
> private, makes it hard to be a state method in .cc file. Can do it separately
if
> we share RefCountedMessage.

I'm not sure I understand...

You just use both of these to create a callback i the impelmentation file.
Clients just see a Task.  Why does this need to be in the header?

http://codereview.chromium.org/4017002/diff/19001/20024
File remoting/protocol/message_decoder.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20024#newcode73
remoting/protocol/message_decoder.cc:73: return header.length();
On 2010/10/27 00:27:36, Alpha wrote:
> On 2010/10/26 22:05:57, awong wrote:
> > ...this seems like a superfluous function doesn't it?
> 
> don't want to do static_cast<int>(header.length()); just to get around
> converting size_t to int.

Ah.  Add a comment saying that.

http://codereview.chromium.org/4017002/diff/19001/20024#newcode105
remoting/protocol/message_decoder.cc:105: if (GetHeaderSize(header) ==
kHeaderSize) {
On 2010/10/27 00:27:36, Alpha wrote:
> On 2010/10/26 22:05:57, awong wrote:
> > Should this be a CHECK instead?
> 
> nope, we don't want critical failure if we can't parse.

But you hit a NOTREACHED?  That should be reserved for programmatic invariant
violations.

Also, we don't have a well-defined recovery semantic.  How would you know where
to start parsing again? 

Since we're reacting to user input (via a network stream), we should define how
to handle this behavior.  I think at the very least we need to be able to signal
an error upwards out of this API and allow higher level logic decide what the
best way to recover is (most likely by just terminating the connection...)

http://codereview.chromium.org/4017002/diff/19001/20026
File remoting/protocol/message_decoder_unittest.cc (right):

http://codereview.chromium.org/4017002/diff/19001/20026#newcode94
remoting/protocol/message_decoder_unittest.cc:94:
ASSERT_TRUE(message_list.size() > 0);
On 2010/10/27 00:27:36, Alpha wrote:
> On 2010/10/26 22:05:57, awong wrote:
> > Did you want an EXPECT_TRUE?
> 
> Done.

...Now the test will just crash on the next line.

http://codereview.chromium.org/4017002/diff/19001/20026#newcode124
remoting/protocol/message_decoder_unittest.cc:124: SimulateReadSequence(kReads,
arraysize(kReads));
On 2010/10/27 00:27:36, Alpha wrote:
> On 2010/10/26 22:05:57, awong wrote:
> > FYI, I'm generally not a fan of having the various EXPECT_* statements sit
> > inside subfunctions in unittests. It messes up the error reporting cause
> > regardless of which test fails, the reported line number is where the
function
> > is.
> > 
> > This will get especially confusing if you start adding multiple calls to the
> > function within one test case.
> > 
> > Is there a way to break this test down into smaller chunks?
> > 
> > I'll think about this more in a bit...need to leave soon.
> 
> You can have the simulate method return a value and then expect in TEST().
> However I don't think it's important to fix in this patch. These added test
> cases are for extra protection of the changes to message decoder.

I agree that this can be done in another CL.  However, I've seen this pattern
show up 3 times in the last week.  We should TODO it, and decide what coding
convention we want to use in our tests this week.

http://codereview.chromium.org/4017002/diff/19001/20028
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/4017002/diff/19001/20028#newcode79
remoting/protocol/message_reader.h:79: template <class T>
On 2010/10/27 00:27:36, Alpha wrote:
> On 2010/10/26 23:05:55, sergeyu wrote:
> > Why not template the whole class instead of just this method?
> 
> It require more careful 'templat'ization of this class. I don't think it
matters
> that much to what this patch is supposed to do. 

A more likely candidate for doing this is MessageDecoder.  It's almost as if
there should be a IOBuffer->MAIS adapter class.  That a templated MessageDecoder
can contain.  The main reason this would be nice is because as is, the API
allows one to accidentally call ParseMessage multiple times with different types
and this will break things.

Sergey, Alpha, do you guys think this is important enough to try and fix?  If
yes...we should do it in a second CL.  This one has churned enough.

http://codereview.chromium.org/4017002/diff/34001/35002
File remoting/base/multiple_array_input_stream.h (right):

http://codereview.chromium.org/4017002/diff/34001/35002#newcode4
remoting/base/multiple_array_input_stream.h:4: //
remove // on line 4.  Just have  a pure newline separating the copyright from
the real comment.

http://codereview.chromium.org/4017002/diff/34001/35002#newcode8
remoting/base/multiple_array_input_stream.h:8: // This input stream is made of
multiple IOBuffer(s) received from the network.
Just say IOBuffers

http://codereview.chromium.org/4017002/diff/34001/35002#newcode10
remoting/base/multiple_array_input_stream.h:10: // within these IOBuffer(s).
IOBuffers.

"This object takes owenrship of these objects" is a bit confusing, and slightly
incorrect.  It doesn't take ownership cause the ownership is shared; it just
retains the object.

Maybe "This object retains the IOBuffers added to it"

http://codereview.chromium.org/4017002/diff/34001/35002#newcode12
remoting/base/multiple_array_input_stream.h:12: // In order to keep tracking of
information like how much data is read from an
tracking -> track

http://codereview.chromium.org/4017002/diff/34001/35002#newcode13
remoting/base/multiple_array_input_stream.h:13: // IOBuffer, a DrainableIOBuffer
is created for each IOBuffer added to the
This paragraph is confusing when read in tandem with the above paragraph.  Both
the MAIS and the DrainableIOBuffer are taking ownership of the IOBuffer?  If you
mention that MAIS is taking ownership, that's probably good enough.

Then you can say something like:

---
Internally, we wrap each added IOBuffer in a DrainableIOBuffer. This allows us
to track how much data has been consumed from each IOBuffer.
---

It's a bit of an implementation detail, but doesn't seem harmful to have here.

http://codereview.chromium.org/4017002/diff/34001/35002#newcode38
remoting/base/multiple_array_input_stream.h:38: // Add a buffer to the list.
Ownership of |buffer| is retained by this
Just say "The |buffer| is retained by this object."

Ownership is weird when there is no single owner.

http://codereview.chromium.org/4017002/diff/34001/35020
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/4017002/diff/34001/35020#newcode48
remoting/protocol/host_message_dispatcher.h:48: // provided to multiple message
handlers.
How about

"A single protobuf can contain multiple messages that will be handled in by
different message handlers.  We use this wrapper to ensure that the protobuf is
only deleted after all the handlers have finished executing."

As a side note, usually people just say "protobuf" when they refer to a protocol
buffer message.

http://codereview.chromium.org/4017002/diff/34001/35024
File remoting/protocol/message_decoder.cc (right):

http://codereview.chromium.org/4017002/diff/34001/35024#newcode46
remoting/protocol/message_decoder.cc:46: // The following loop gather enough
buffers in |buffer_list_| to make up of
grammar...I actually can't quite parse the comment.

http://codereview.chromium.org/4017002/diff/34001/35025
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4017002/diff/34001/35025#newcode34
remoting/protocol/message_decoder.h:34: // It provides ParseMessages() which
accepts an object of IOBuffer. If bytes
It provides ParseMessage() which accepts an IOBuffer.  If enough bytes are
collected to produce a protobuf, then the bytes will be consumed into a protobuf
of the requested type and added into the output list.

http://codereview.chromium.org/4017002/diff/34001/35025#newcode39
remoting/protocol/message_decoder.h:39: // It retains ownership of IOBuffer
given to this object and keep it alive
keep it -> keeps it

http://codereview.chromium.org/4017002/diff/34001/35025#newcode47
remoting/protocol/message_decoder.h:47: // MessageType. The ownership of |data|
is passed to this object. If there
Put the ownership information at the end.  Also, could use a little more
information about how the function should be sued...Maybe something like

Parses the bytes in |data| into a protobuf of type MessageType.  The bytes in
|data| are conceptually a stream of bytes to be parsed into a series of
protobufs. Each parsed protouf is appended into the |messages|. All calls to
ParseMessages should use same MessageType for any single instace of
MessageDecoder.

This function retains |data| until all its bytes are consued.
Ownership of the produced protobufs is passed to the caller via the |messages|
list.

http://codereview.chromium.org/4017002/diff/34001/35025#newcode88
remoting/protocol/message_decoder.h:88: // size. |size| is the output parameter
for payload size of the next message.
A more concise phrasing would be.

Retrieves the read payload size of the current protocol buffer via |size|. 
Returns false and leaves |size| unmodified, if we do not have enough data to
retrieve the current size.

http://codereview.chromium.org/4017002/diff/34001/35025#newcode93
remoting/protocol/message_decoder.h:93: // The number of bytes in |buffer_list_|
not read.
read -> consumed

keeping terminology consistent...

[Alpha Left Google, 2010-10-27 20:45:24]

fixed comments. I can't find comments on code issues, can you put it in again?

http://codereview.chromium.org/4017002/diff/34001/35002
File remoting/base/multiple_array_input_stream.h (right):

http://codereview.chromium.org/4017002/diff/34001/35002#newcode4
remoting/base/multiple_array_input_stream.h:4: //
On 2010/10/27 02:09:36, awong wrote:
> remove // on line 4.  Just have  a pure newline separating the copyright from
> the real comment.

Done.

http://codereview.chromium.org/4017002/diff/34001/35002#newcode8
remoting/base/multiple_array_input_stream.h:8: // This input stream is made of
multiple IOBuffer(s) received from the network.
On 2010/10/27 02:09:36, awong wrote:
> Just say IOBuffers

Done.

http://codereview.chromium.org/4017002/diff/34001/35002#newcode10
remoting/base/multiple_array_input_stream.h:10: // within these IOBuffer(s).
On 2010/10/27 02:09:36, awong wrote:
> IOBuffers.
> 
> "This object takes owenrship of these objects" is a bit confusing, and
slightly
> incorrect.  It doesn't take ownership cause the ownership is shared; it just
> retains the object.
> 
> Maybe "This object retains the IOBuffers added to it"

Done.

http://codereview.chromium.org/4017002/diff/34001/35002#newcode12
remoting/base/multiple_array_input_stream.h:12: // In order to keep tracking of
information like how much data is read from an
On 2010/10/27 02:09:36, awong wrote:
> tracking -> track

Done.

http://codereview.chromium.org/4017002/diff/34001/35002#newcode13
remoting/base/multiple_array_input_stream.h:13: // IOBuffer, a DrainableIOBuffer
is created for each IOBuffer added to the
On 2010/10/27 02:09:36, awong wrote:
> This paragraph is confusing when read in tandem with the above paragraph. 
Both
> the MAIS and the DrainableIOBuffer are taking ownership of the IOBuffer?  If
you
> mention that MAIS is taking ownership, that's probably good enough.
> 
> Then you can say something like:
> 
> ---
> Internally, we wrap each added IOBuffer in a DrainableIOBuffer. This allows us
> to track how much data has been consumed from each IOBuffer.
> ---
> 
> It's a bit of an implementation detail, but doesn't seem harmful to have here.

Done.

http://codereview.chromium.org/4017002/diff/34001/35002#newcode38
remoting/base/multiple_array_input_stream.h:38: // Add a buffer to the list.
Ownership of |buffer| is retained by this
On 2010/10/27 02:09:36, awong wrote:
> Just say "The |buffer| is retained by this object."
> 
> Ownership is weird when there is no single owner.

Done.

http://codereview.chromium.org/4017002/diff/34001/35020
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/4017002/diff/34001/35020#newcode48
remoting/protocol/host_message_dispatcher.h:48: // provided to multiple message
handlers.
On 2010/10/27 02:09:36, awong wrote:
> How about
> 
> "A single protobuf can contain multiple messages that will be handled in by
> different message handlers.  We use this wrapper to ensure that the protobuf
is
> only deleted after all the handlers have finished executing."
> 
> As a side note, usually people just say "protobuf" when they refer to a
protocol
> buffer message.

Done.

http://codereview.chromium.org/4017002/diff/34001/35024
File remoting/protocol/message_decoder.cc (right):

http://codereview.chromium.org/4017002/diff/34001/35024#newcode46
remoting/protocol/message_decoder.cc:46: // The following loop gather enough
buffers in |buffer_list_| to make up of
On 2010/10/27 02:09:36, awong wrote:
> grammar...I actually can't quite parse the comment.

Done.

http://codereview.chromium.org/4017002/diff/34001/35025
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4017002/diff/34001/35025#newcode34
remoting/protocol/message_decoder.h:34: // It provides ParseMessages() which
accepts an object of IOBuffer. If bytes
On 2010/10/27 02:09:36, awong wrote:
> It provides ParseMessage() which accepts an IOBuffer.  If enough bytes are
> collected to produce a protobuf, then the bytes will be consumed into a
protobuf
> of the requested type and added into the output list.

Done.

http://codereview.chromium.org/4017002/diff/34001/35025#newcode39
remoting/protocol/message_decoder.h:39: // It retains ownership of IOBuffer
given to this object and keep it alive
On 2010/10/27 02:09:36, awong wrote:
> keep it -> keeps it

Done.

http://codereview.chromium.org/4017002/diff/34001/35025#newcode47
remoting/protocol/message_decoder.h:47: // MessageType. The ownership of |data|
is passed to this object. If there
On 2010/10/27 02:09:36, awong wrote:
> Put the ownership information at the end.  Also, could use a little more
> information about how the function should be sued...Maybe something like
> 
> Parses the bytes in |data| into a protobuf of type MessageType.  The bytes in
> |data| are conceptually a stream of bytes to be parsed into a series of
> protobufs. Each parsed protouf is appended into the |messages|. All calls to
> ParseMessages should use same MessageType for any single instace of
> MessageDecoder.
> 
> This function retains |data| until all its bytes are consued.
> Ownership of the produced protobufs is passed to the caller via the |messages|
> list.  
> 

Done.

http://codereview.chromium.org/4017002/diff/34001/35025#newcode88
remoting/protocol/message_decoder.h:88: // size. |size| is the output parameter
for payload size of the next message.
On 2010/10/27 02:09:36, awong wrote:
> A more concise phrasing would be.
> 
> Retrieves the read payload size of the current protocol buffer via |size|. 
> Returns false and leaves |size| unmodified, if we do not have enough data to
> retrieve the current size. 

Done.

http://codereview.chromium.org/4017002/diff/34001/35025#newcode93
remoting/protocol/message_decoder.h:93: // The number of bytes in |buffer_list_|
not read.
On 2010/10/27 02:09:36, awong wrote:
> read -> consumed
> 
> keeping terminology consistent...

Done.

[awong, 2010-10-27 20:53:07]

one nit.

http://codereview.chromium.org/4017002/diff/81001/49026
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4017002/diff/81001/49026#newcode52
remoting/protocol/message_decoder.h:52: // This function retains |data| until
all its bytes are consued.
consued -> consumed

[Alpha Left Google, 2010-10-27 22:59:30]

fixed and merged.

[awong, 2010-10-27 23:03:36]

LGTM

I'm just going to assume you merged that correctly.