Chromoting protocol layers to receive and send login messages

Chromoting protocol layers to receive and send login messages

BUG=None
TEST=None

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

[awong, 2010-12-23 01:15:09]

looks mostly good.  Got a few nits below.

Also, is there a unittest that should be updated?

http://codereview.chromium.org/6030007/diff/4001/remoting/client/chromoting_c...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/6030007/diff/4001/remoting/client/chromoting_c...
remoting/client/chromoting_client.cc:228: // TODO(hclam): Implement.
Use a NOTIMPLEMENTED()

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.cc:22: class RefCountedMessage :
public base::RefCounted<RefCountedMessage<T> > {
Is this cut/paste code?  Can we factor it out?

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
File remoting/protocol/client_message_dispatcher.h (right):

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:20: class MessageReader;
sort

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:32: // Object of this class is
owned by ConnectionToHost to dispatch messages
Drop the "to dispatch messages to itself."

This dispatcher is actually dispatching messages to the client stub.

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:46: // This method is called by
|control_channel_reader_| when a control
This comment says exactly what the function name says.  Drop it?

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:51: // that parses data on the
channel and then delegates the message to this
"then delegates the message to this class" -> "then calls the corresponding
handler in this class."

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:57: ClientStub* client_stub_;
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/jingle_con...
File remoting/protocol/jingle_connection_to_host.cc (right):

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/jingle_con...
remoting/protocol/jingle_connection_to_host.cc:179:
dispatcher_->Initialize(session_.get(), client_stub_);
If Initialize() reutrns a bool, shouldn't we check it here?

[Alpha Left Google, 2010-12-23 02:17:58]

We already have unit tests for testing the pieces we are using, e.g.
MessageDecoder, JingleSession, etc. However we don't have mocks currently to
send protobuf messages through this protocol layer.

http://codereview.chromium.org/6030007/diff/4001/remoting/client/chromoting_c...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/6030007/diff/4001/remoting/client/chromoting_c...
remoting/client/chromoting_client.cc:228: // TODO(hclam): Implement.
On 2010/12/23 01:15:10, awong wrote:
> Use a NOTIMPLEMENTED()

Done.

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.cc:22: class RefCountedMessage :
public base::RefCounted<RefCountedMessage<T> > {
On 2010/12/23 01:15:10, awong wrote:
> Is this cut/paste code?  Can we factor it out?

Done.

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
File remoting/protocol/client_message_dispatcher.h (right):

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:20: class MessageReader;
On 2010/12/23 01:15:10, awong wrote:
> sort

Done.

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:32: // Object of this class is
owned by ConnectionToHost to dispatch messages
On 2010/12/23 01:15:10, awong wrote:
> Drop the "to dispatch messages to itself."
> 
> This dispatcher is actually dispatching messages to the client stub.

Done.

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:46: // This method is called by
|control_channel_reader_| when a control
On 2010/12/23 01:15:10, awong wrote:
> This comment says exactly what the function name says.  Drop it?

Done.

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:51: // that parses data on the
channel and then delegates the message to this
On 2010/12/23 01:15:10, awong wrote:
> "then delegates the message to this class" -> "then calls the corresponding
> handler in this class."

Done.

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/client_mes...
remoting/protocol/client_message_dispatcher.h:57: ClientStub* client_stub_;
On 2010/12/23 01:15:10, awong wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/jingle_con...
File remoting/protocol/jingle_connection_to_host.cc (right):

http://codereview.chromium.org/6030007/diff/4001/remoting/protocol/jingle_con...
remoting/protocol/jingle_connection_to_host.cc:179:
dispatcher_->Initialize(session_.get(), client_stub_);
On 2010/12/23 01:15:10, awong wrote:
> If Initialize() reutrns a bool, shouldn't we check it here?

Done.

[awong, 2010-12-23 02:30:31]

http://codereview.chromium.org/6030007/diff/11001/remoting/client/chromoting_...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/6030007/diff/11001/remoting/client/chromoting_...
remoting/client/chromoting_client.cc:221: // TODO(garykac): Implement this.
Can you add NOTIMPLEMENTED() here too?

http://codereview.chromium.org/6030007/diff/11001/remoting/protocol/jingle_co...
File remoting/protocol/jingle_connection_to_host.cc (right):

http://codereview.chromium.org/6030007/diff/11001/remoting/protocol/jingle_co...
remoting/protocol/jingle_connection_to_host.cc:171: LOG(WARNING) << "Failed to
initialize ClientMessageDispatcher";
...is this safe?

Is there any sort of sane recovery if dispatcher_ fails?  should we CHECK?

The more I look at it, the more I think we should just remove the bool from the
initialize API.

[Alpha Left Google, 2010-12-23 02:58:37]

Addressed your comments.

[awong, 2010-12-23 02:59:23]

LGTM