Refactor client channel dispatchers.

Refactor client channel dispatchers.

The new ClientControlDispatcher and ClientEventDispatcher manage control 
and event channels.


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

[Wez, 2011-11-17 01:51:56]

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
File remoting/protocol/client_control_dispatcher.cc (right):

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.cc:38:
BufferedSocketWriter::WriteFailedCallback());
Seems strange to ignore write errors?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.cc:41:
&ClientControlDispatcher::OnMessageReceived, base::Unretained(this)));
Can either of these Init() calls fail?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.cc:51: LOG(WARNING) << "Invalid
control message received.";
nit: The message isn't necessarily "invalid", just "unknown".

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.cc:57: writer_->Close();
Fold this in to the destructor?  Do we even need to call Close() on the writer,
rather than just dropping our reference to it?

Are we guaranteed that when the reader is torn down, we'll get no more callbacks
from it?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
File remoting/protocol/client_control_dispatcher.h (right):

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:25: // ClientControlDispatcher
manages control channel on the client
typo: manages _the_ control channel

Although I think this comment could just become:

ClientControlDispatcher dispatches control messages from the Host to the
ClientStub, and implements the HostStub interface to write messages to the
control channel.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:27: // ClientStub, and also
implements HostStub for outgoing messages.
I think perhaps ClientStub and HostStub need renaming, probably in a follow-up
CL.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:30: // Construct a message
dispatcher.
What is the |message_loop| parameter used for?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:31:
ClientControlDispatcher(base::MessageLoopProxy* message_loop);
Why is the |message_loop| supplied to the ctor, rather than to Init()?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:31:
ClientControlDispatcher(base::MessageLoopProxy* message_loop);
Missing explicit here.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:35: // message handlers.
Who owns them?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:38: void Close();
What does this do?  Who uses it?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
File remoting/protocol/client_input_dispatcher.cc (right):

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.cc:29: // TODO(garykac) Set write
failed callback.
This TODO applies equally to the ClientControlDispatcher. ;)

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.cc:49: writer_->Close();
Fold this into dtor?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
File remoting/protocol/client_input_dispatcher.h (right):

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:5: #ifndef
REMOTING_PROTOCOL_CLINET_INPUT_DISPATCHER_H_
typo: CLINET

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:23: // implements InputStub for
outgoing input messages.
This comment should probably more closely mirror what ClientControlDispatcher
says.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:24: class ClientInputDispatcher :
public InputStub {
This actually manages the whole event channel, not just for input (although
currently that's all we use it for), so we should either rename these to
ClientEventDispatcher and EventStub, or rename the channel to "input"...

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:26: explicit
ClientInputDispatcher(base::MessageLoopProxy* message_loop);
How is MessageLoopProxy used?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:29: void Init(Session* session);
How is session used?  Who owns it?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:31: void Close();
What is this used for?

[Sergey Ulanov, 2011-11-17 19:29:06]

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
File remoting/protocol/client_control_dispatcher.cc (right):

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.cc:38:
BufferedSocketWriter::WriteFailedCallback());
On 2011/11/17 01:51:56, Wez wrote:
> Seems strange to ignore write errors?

Write may fail only when the channel is disconnected, and this should trigger
error from the session layer anyway. In either case I agree that it's not
correct to ignore this error, this should be fixed, but in a separate CL.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.cc:41:
&ClientControlDispatcher::OnMessageReceived, base::Unretained(this)));
On 2011/11/17 01:51:56, Wez wrote:
> Can either of these Init() calls fail?
No.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.cc:51: LOG(WARNING) << "Invalid
control message received.";
On 2011/11/17 01:51:56, Wez wrote:
> nit: The message isn't necessarily "invalid", just "unknown".

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.cc:57: writer_->Close();
On 2011/11/17 01:51:56, Wez wrote:
> Fold this in to the destructor? 

Done.

> Do we even need to call Close() on the writer,
> rather than just dropping our reference to it?

writer is currently ref-counted so we need to explicitly close it so that it
stops using the socket. Actually now there is no need to the writer to be
ref-counted because we use it only from one thread, so I'll make it
non-ref-counted.


> 
> Are we guaranteed that when the reader is torn down, we'll get no more
callbacks
> from it?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
File remoting/protocol/client_control_dispatcher.h (right):

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:25: // ClientControlDispatcher
manages control channel on the client
On 2011/11/17 01:51:56, Wez wrote:
> typo: manages _the_ control channel
> 
> Although I think this comment could just become:
> 
> ClientControlDispatcher dispatches control messages from the Host to the
> ClientStub, and implements the HostStub interface to write messages to the
> control channel.

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:27: // ClientStub, and also
implements HostStub for outgoing messages.
On 2011/11/17 01:51:56, Wez wrote:
> I think perhaps ClientStub and HostStub need renaming, probably in a follow-up
> CL.

Agree. Any suggestions for new names?

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:30: // Construct a message
dispatcher.
On 2011/11/17 01:51:56, Wez wrote:
> What is the |message_loop| parameter used for?
Removed this comment and |message_loop|

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:31:
ClientControlDispatcher(base::MessageLoopProxy* message_loop);
On 2011/11/17 01:51:56, Wez wrote:
> Why is the |message_loop| supplied to the ctor, rather than to Init()?

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:31:
ClientControlDispatcher(base::MessageLoopProxy* message_loop);
On 2011/11/17 01:51:56, Wez wrote:
> Missing explicit here.
not needed anymore

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:35: // message handlers.
On 2011/11/17 01:51:56, Wez wrote:
> Who owns them?

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_contro...
remoting/protocol/client_control_dispatcher.h:38: void Close();
On 2011/11/17 01:51:56, Wez wrote:
> What does this do?  Who uses it?

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
File remoting/protocol/client_input_dispatcher.cc (right):

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.cc:29: // TODO(garykac) Set write
failed callback.
On 2011/11/17 01:51:56, Wez wrote:
> This TODO applies equally to the ClientControlDispatcher. ;)

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.cc:49: writer_->Close();
On 2011/11/17 01:51:56, Wez wrote:
> Fold this into dtor?

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
File remoting/protocol/client_input_dispatcher.h (right):

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:5: #ifndef
REMOTING_PROTOCOL_CLINET_INPUT_DISPATCHER_H_
On 2011/11/17 01:51:56, Wez wrote:
> typo: CLINET

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:23: // implements InputStub for
outgoing input messages.
On 2011/11/17 01:51:56, Wez wrote:
> This comment should probably more closely mirror what ClientControlDispatcher
> says.

currently client doesn't receive any messages on the event channel, that's why
this comment is different from ClientControlDispatcher.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:24: class ClientInputDispatcher :
public InputStub {
On 2011/11/17 01:51:56, Wez wrote:
> This actually manages the whole event channel, not just for input (although
> currently that's all we use it for), so we should either rename these to
> ClientEventDispatcher and EventStub, or rename the channel to "input"...


Renamed it to ClientEventDispatcher.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:26: explicit
ClientInputDispatcher(base::MessageLoopProxy* message_loop);
On 2011/11/17 01:51:56, Wez wrote:
> How is MessageLoopProxy used?

removed it.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:29: void Init(Session* session);
On 2011/11/17 01:51:56, Wez wrote:
> How is session used?  Who owns it?

Done.

http://codereview.chromium.org/8574025/diff/1/remoting/protocol/client_input_...
remoting/protocol/client_input_dispatcher.h:31: void Close();
On 2011/11/17 01:51:56, Wez wrote:
> What is this used for?

Removed

[Wez, 2011-11-17 22:30:22]

lgtm

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
File remoting/protocol/client_control_dispatcher.cc (right):

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
remoting/protocol/client_control_dispatcher.cc:46: // older clients. Ignore it.
nit: Ignore it without warning.

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
remoting/protocol/client_control_dispatcher.cc:50: done_task.Run();
nit: Use a ScopedClosureRunner so that future changes to this function are less
likely to break things?

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
File remoting/protocol/client_control_dispatcher.h (right):

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
remoting/protocol/client_control_dispatcher.h:34: // |session|. Doesn't take
ownership of |session|.
nit: Doesn't take... -> Caller retains ...

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
remoting/protocol/client_control_dispatcher.h:39: // this dispatcher.
typo & nit: Caller retains...

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_eve...
File remoting/protocol/client_event_dispatcher.h (right):

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_eve...
remoting/protocol/client_event_dispatcher.h:30: // |session|.
nit: Clarify that |session| must outlive the dispatcher?

[Sergey Ulanov, 2011-11-17 22:48:08]

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
File remoting/protocol/client_control_dispatcher.cc (right):

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
remoting/protocol/client_control_dispatcher.cc:46: // older clients. Ignore it.
On 2011/11/17 22:30:22, Wez wrote:
> nit: Ignore it without warning.

Done.

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
remoting/protocol/client_control_dispatcher.cc:50: done_task.Run();
On 2011/11/17 22:30:22, Wez wrote:
> nit: Use a ScopedClosureRunner so that future changes to this function are
less
> likely to break things?

Done.

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
File remoting/protocol/client_control_dispatcher.h (right):

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
remoting/protocol/client_control_dispatcher.h:34: // |session|. Doesn't take
ownership of |session|.
On 2011/11/17 22:30:22, Wez wrote:
> nit: Doesn't take... -> Caller retains ...

Removed this method in the next CL.

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_con...
remoting/protocol/client_control_dispatcher.h:39: // this dispatcher.
On 2011/11/17 22:30:22, Wez wrote:
> typo & nit: Caller retains...
Fixed typo. Caller doesn't own |client_stub|.

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_eve...
File remoting/protocol/client_event_dispatcher.h (right):

http://codereview.chromium.org/8574025/diff/5003/remoting/protocol/client_eve...
remoting/protocol/client_event_dispatcher.h:30: // |session|.
On 2011/11/17 22:30:22, Wez wrote:
> nit: Clarify that |session| must outlive the dispatcher?
I moved this method to ChannelDispatcher in the next CL.

[commit-bot: I haz the power, 2011-11-18 01:35:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8574025/7016

[commit-bot: I haz the power, 2011-11-18 04:06:11]

Change committed as 110648