Refactor channel dispatchers on the host side.

Refactor channel dispatchers on the host side.

The new HostControlDispatcher manages reading and writing to and from control 
channel on the host side. Similarly HostEventDispatcher is responsible for
reading and, in future, writing to and from the event channel.


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

[Sergey Ulanov, 2011-11-15 22:26:18]

In the next CL I'll do similar refactoring for the client side.

[Wez, 2011-11-17 02:03:40]

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
File remoting/protocol/host_control_dispatcher.cc (right):

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.cc:7: #include
"remoting/protocol/buffered_socket_writer.h"
This belongs after the remoting/proto/ includes?

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.cc:42:
writer_->Write(SerializeAndFrameMessage(message), base::Closure());
Is it really the dispatcher's responsibility to do that?  It feels like it'd be
more appropriate for the calling code to do it, leaving this a pure helper
class.  Or the fact that Init() sends this message should be mentioned in the
header.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.cc:47: LOG(WARNING) << "Invalid
control message received.";
nit: Not "invalid", just "unknown"!

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
File remoting/protocol/host_control_dispatcher.h (right):

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.h:30: explicit
HostControlDispatcher(base::MessageLoopProxy* message_loop);
What is message_loop used for?

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.h:33: void Init(Session* session,
HostStub* host_stub);
How are session & host_stub used, and who owns them?

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.h:37: void Close();
Calling code doesn't seem to use this - it just deletes the object?

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
File remoting/protocol/host_input_dispatcher.cc (right):

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
remoting/protocol/host_input_dispatcher.cc:46: return;
nit: The return strictly belongs in the scope of if (has_key_event()), I think.

I'd recommend either using:

 if (...) { ...; return; }
 if (...) { ...; return; }
 ...;

or

 if (...) { ...; }
 else if (...) { ...; }
 else {...}

to make it clearer how to handle adding new message types.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
File remoting/protocol/host_input_dispatcher.h (right):

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
remoting/protocol/host_input_dispatcher.h:29: // message handlers.
How are they used, and who owns them, etc?

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
remoting/protocol/host_input_dispatcher.h:32: const SequenceNumberCallback&
seq_num_callback);
Style guide says no abbreviated names. :(

[Sergey Ulanov, 2011-11-17 18:38:13]

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
File remoting/protocol/host_control_dispatcher.cc (right):

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.cc:7: #include
"remoting/protocol/buffered_socket_writer.h"
On 2011/11/17 02:03:40, Wez wrote:
> This belongs after the remoting/proto/ includes?

Done.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.cc:42:
writer_->Write(SerializeAndFrameMessage(message), base::Closure());
On 2011/11/17 02:03:40, Wez wrote:
> Is it really the dispatcher's responsibility to do that?  It feels like it'd
be
> more appropriate for the calling code to do it, leaving this a pure helper
> class.  Or the fact that Init() sends this message should be mentioned in the
> header.

I'd like to hide version-dependent details inside dispatcher. I.e. the interface
of the dispatcher is always the same no matter what version of the protocol is
used. This message is here for backward compatibility and upper layers shouldn't
care about it.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.cc:47: LOG(WARNING) << "Invalid
control message received.";
On 2011/11/17 02:03:40, Wez wrote:
> nit: Not "invalid", just "unknown"!

Done.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
File remoting/protocol/host_control_dispatcher.h (right):

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.h:30: explicit
HostControlDispatcher(base::MessageLoopProxy* message_loop);
On 2011/11/17 02:03:40, Wez wrote:
> What is message_loop used for?
Removed it - we don't really need it because it always corresponds to the
current thread.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.h:33: void Init(Session* session,
HostStub* host_stub);
On 2011/11/17 02:03:40, Wez wrote:
> How are session & host_stub used, and who owns them?

Done.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.h:37: void Close();
On 2011/11/17 02:03:40, Wez wrote:
> Calling code doesn't seem to use this - it just deletes the object?

Done. Also in HostInputDispatcher.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
File remoting/protocol/host_input_dispatcher.cc (right):

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
remoting/protocol/host_input_dispatcher.cc:46: return;
On 2011/11/17 02:03:40, Wez wrote:
> nit: The return strictly belongs in the scope of if (has_key_event()), I
think.
> 
> I'd recommend either using:
> 
>  if (...) { ...; return; }
>  if (...) { ...; return; }
>  ...;
> 
> or
> 
>  if (...) { ...; }
>  else if (...) { ...; }
>  else {...}
> 
> to make it clearer how to handle adding new message types.

Done.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
File remoting/protocol/host_input_dispatcher.h (right):

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
remoting/protocol/host_input_dispatcher.h:29: // message handlers.
On 2011/11/17 02:03:40, Wez wrote:
> How are they used, and who owns them, etc?

Done.

http://codereview.chromium.org/8468022/diff/2001/remoting/protocol/host_input...
remoting/protocol/host_input_dispatcher.h:32: const SequenceNumberCallback&
seq_num_callback);
On 2011/11/17 02:03:40, Wez wrote:
> Style guide says no abbreviated names. :(

Done.

[Wez, 2011-11-17 22:25:15]

LGTM with a few remaining nits. :)

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
File remoting/protocol/host_control_dispatcher.cc (right):

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
remoting/protocol/host_control_dispatcher.cc:35: // Write legacy BeginSession
message.
We should remove this once clients can reasonably be expected to have been
upgraded to not need this; create a bug with the relevant milestone, and link to
it?

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
File remoting/protocol/host_control_dispatcher.h (right):

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
remoting/protocol/host_control_dispatcher.h:34: // |session|. Doesn't take
ownership of |session|.
nit: Doesn't take... -> Caller retains ...

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
remoting/protocol/host_control_dispatcher.h:38: // message. Doesn't take
ownershipt of |host_stub|. It must outlive
typo: ownershipt

Suggest:
Caller retains ownership of |host_stub|, which must outlive the dispatcher.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
File remoting/protocol/host_event_dispatcher.cc (right):

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.cc:39:
input_stub_->InjectKeyEvent(event);
Is there a risk that InjectKeyEvent() triggers a teardown that invalidates the
|done_task|?

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.cc:45: return;
nit: Lose the return?

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
File remoting/protocol/host_event_dispatcher.h (right):

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.h:28: // |session|. Doesn't take
ownership of |session|.
nit: Caller retains ...

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.h:33: // the dispatcher.
typo, and nit: Caller retains ...

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.h:36: // Set callback that will be
called with sequence number received in
nit: Set a callback to notify of each message's sequence number.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.h:37: // each message.
Clarify that this callback is not permitted to tear anything down?

[Wez, 2011-11-17 22:26:16]

Oh, and the CL description needs updating to refer to HostEventDispatcher, not
HostInputDispatcher.

[Sergey Ulanov, 2011-11-17 22:41:39]

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
File remoting/protocol/host_control_dispatcher.cc (right):

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
remoting/protocol/host_control_dispatcher.cc:35: // Write legacy BeginSession
message.
On 2011/11/17 22:25:16, Wez wrote:
> We should remove this once clients can reasonably be expected to have been
> upgraded to not need this; create a bug with the relevant milestone, and link
to
> it?

Done.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
File remoting/protocol/host_control_dispatcher.h (right):

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
remoting/protocol/host_control_dispatcher.h:34: // |session|. Doesn't take
ownership of |session|.
On 2011/11/17 22:25:16, Wez wrote:
> nit: Doesn't take... -> Caller retains ...
I moved this method to ChannelDispatcherBase in another CL I published today.
I'll update the comment there.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_cont...
remoting/protocol/host_control_dispatcher.h:38: // message. Doesn't take
ownershipt of |host_stub|. It must outlive
On 2011/11/17 22:25:16, Wez wrote:
> typo: ownershipt
> 
> Suggest:
> Caller retains ownership of |host_stub|, which must outlive the dispatcher.

Caller doesn't own |host_stub|

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
File remoting/protocol/host_event_dispatcher.cc (right):

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.cc:39:
input_stub_->InjectKeyEvent(event);
On 2011/11/17 22:25:16, Wez wrote:
> Is there a risk that InjectKeyEvent() triggers a teardown that invalidates the
> |done_task|?
I don't think so.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.cc:45: return;
On 2011/11/17 22:25:16, Wez wrote:
> nit: Lose the return?

Done.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
File remoting/protocol/host_event_dispatcher.h (right):

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.h:28: // |session|. Doesn't take
ownership of |session|.
On 2011/11/17 22:25:16, Wez wrote:
> nit: Caller retains ...

Done.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.h:33: // the dispatcher.
On 2011/11/17 22:25:16, Wez wrote:
> typo, and nit: Caller retains ...

Caller doesn't own input_stub, so i think "caller retains" would be incorrect.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.h:36: // Set callback that will be
called with sequence number received in
On 2011/11/17 22:25:16, Wez wrote:
> nit: Set a callback to notify of each message's sequence number.

Done.

http://codereview.chromium.org/8468022/diff/10001/remoting/protocol/host_even...
remoting/protocol/host_event_dispatcher.h:37: // each message.
On 2011/11/17 22:25:16, Wez wrote:
> Clarify that this callback is not permitted to tear anything down?

Done.

[Sergey Ulanov, 2011-11-17 22:41:50]

On 2011/11/17 22:26:16, Wez wrote:
> Oh, and the CL description needs updating to refer to HostEventDispatcher, not
> HostInputDispatcher.

Done

[commit-bot: I haz the power, 2011-11-17 22:42:02]

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

[commit-bot: I haz the power, 2011-11-17 23:57:58]

Commit queue failed due to new patchset.

[commit-bot: I haz the power, 2011-11-17 23:58:11]

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

[commit-bot: I haz the power, 2011-11-18 01:31:45]

Change committed as 110625