Cleanup channel dispatchers

Cleanup channel dispatchers

Now ChannelDispatcherBase handles initialization of reader and writer,
which makes all dispatcher classes simpler. ChannelDispatcherBase now
can handle writer errors. Also renamed
ProtobufMessageReader -> ProtobufMessageParser.

Committed: https://crrev.com/203832d43af2d2c0cb92ca2a6ad3e59193c6836d
Cr-Commit-Position: refs/heads/master@{#310993}

[Sergey Ulanov, 2015-01-08 01:22:04]

sergeyu@chromium.org changed reviewers:
+ rmsousa@chromium.org

[Wez, 2015-01-08 01:39:30]

nit: CL description has typo ChannelDisparcherBase vs ChannelDispatcherBase

On 7 January 2015 at 17:22, <sergeyu@chromium.org> wrote:

> Reviewers: rmsousa,
>
> Description:
> Cleanup channel dispatchers
>
> Now ChannelDisparcherBase handles initialization of reader and writer,
> which makes all dispatcher classes simpler. ChannelDisparcherBase now
> can handle writer errors. Also renamed
> ProtobufMessageReader -> ProtobufMessageParser.
>
> Please review this at https://codereview.chromium.org/841773005/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+244, -298 lines):
>   M remoting/protocol/audio_reader.h
>   M remoting/protocol/audio_reader.cc
>   M remoting/protocol/audio_writer.h
>   M remoting/protocol/audio_writer.cc
>   M remoting/protocol/channel_dispatcher_base.h
>   M remoting/protocol/channel_dispatcher_base.cc
>   M remoting/protocol/channel_multiplexer.h
>   M remoting/protocol/channel_multiplexer.cc
>   M remoting/protocol/client_control_dispatcher.h
>   M remoting/protocol/client_control_dispatcher.cc
>   M remoting/protocol/client_event_dispatcher.h
>   M remoting/protocol/client_event_dispatcher.cc
>   M remoting/protocol/client_video_dispatcher.h
>   M remoting/protocol/client_video_dispatcher.cc
>   M remoting/protocol/connection_to_client.h
>   M remoting/protocol/connection_to_client.cc
>   M remoting/protocol/connection_to_host.h
>   M remoting/protocol/connection_to_host.cc
>   M remoting/protocol/host_control_dispatcher.h
>   M remoting/protocol/host_control_dispatcher.cc
>   M remoting/protocol/host_event_dispatcher.h
>   M remoting/protocol/host_event_dispatcher.cc
>   M remoting/protocol/host_video_dispatcher.h
>   M remoting/protocol/host_video_dispatcher.cc
>   M remoting/protocol/message_reader.h
>   M remoting/protocol/message_reader.cc
>   M remoting/protocol/message_reader_unittest.cc
>   A remoting/protocol/protobuf_message_parser.h
>
>
>

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

[rmsousa, 2015-01-08 02:22:31]

lgtm

lgtm with a few paranoid questions

https://codereview.chromium.org/841773005/diff/20001/remoting/protocol/channe...
File remoting/protocol/channel_dispatcher_base.cc (right):

https://codereview.chromium.org/841773005/diff/20001/remoting/protocol/channe...
remoting/protocol/channel_dispatcher_base.cc:60: writer_.Init(channel_.get(),
base::Bind(&ChannelDispatcherBase::OnWriteFailed,
This means that every child of ChannelDispatcherBase will now start both a
reader (that discards everything because the callback is null) and a writer.

Questions:
* Is there such a thing as a read-only/write-only channel, and would this cause
a problem in that case?
* As far as I can tell, each ChannelDispatcherBase creates its own channel -
but, just to confirm, there's no case where we create separate "reader" and
"writer" ChannelDispatchers for hte same channel, right? (i.e. there's no chance
that a "writer" ChannelDispatcherBase would read and discard messages that
another "reader" ChannelDispatcherBase would be interested in)?
* Any other unintended consequences of running a reader in channels wehre we
didn't previously run one? (I assume running hte writer is fine as long as we
don't write anything).

https://codereview.chromium.org/841773005/diff/20001/remoting/protocol/messag...
File remoting/protocol/message_reader.h (right):

https://codereview.chromium.org/841773005/diff/20001/remoting/protocol/messag...
remoting/protocol/message_reader.h:39: explicit MessageReader();
nit: explicit with zero arguments?

[Wez, 2015-01-08 02:47:13]

The only impact I can see to reading when we wouldn't otherwise have done
would be that we're then pulling data from PseudoTcp even though we don't
want it. Without that a writer would eventually have to stop writing
because the TCP window would drop to zero, whereas now it won't; seems a
fairly minor issue, though.

On 7 January 2015 at 18:22, <rmsousa@chromium.org> wrote:

> lgtm
>
> lgtm with a few paranoid questions
>
>
> https://codereview.chromium.org/841773005/diff/20001/
> remoting/protocol/channel_dispatcher_base.cc
> File remoting/protocol/channel_dispatcher_base.cc (right):
>
> https://codereview.chromium.org/841773005/diff/20001/
> remoting/protocol/channel_dispatcher_base.cc#newcode60
> remoting/protocol/channel_dispatcher_base.cc:60:
> writer_.Init(channel_.get(),
> base::Bind(&ChannelDispatcherBase::OnWriteFailed,
> This means that every child of ChannelDispatcherBase will now start both
> a reader (that discards everything because the callback is null) and a
> writer.
>
> Questions:
> * Is there such a thing as a read-only/write-only channel, and would
> this cause a problem in that case?
> * As far as I can tell, each ChannelDispatcherBase creates its own
> channel - but, just to confirm, there's no case where we create separate
> "reader" and "writer" ChannelDispatchers for hte same channel, right?
> (i.e. there's no chance that a "writer" ChannelDispatcherBase would read
> and discard messages that another "reader" ChannelDispatcherBase would
> be interested in)?
> * Any other unintended consequences of running a reader in channels
> wehre we didn't previously run one? (I assume running hte writer is fine
> as long as we don't write anything).
>
> https://codereview.chromium.org/841773005/diff/20001/
> remoting/protocol/message_reader.h
> File remoting/protocol/message_reader.h (right):
>
> https://codereview.chromium.org/841773005/diff/20001/
> remoting/protocol/message_reader.h#newcode39
> remoting/protocol/message_reader.h:39: explicit MessageReader();
> nit: explicit with zero arguments?
>
> https://codereview.chromium.org/841773005/
>

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

[Sergey Ulanov, 2015-01-08 19:20:32]

https://codereview.chromium.org/841773005/diff/20001/remoting/protocol/channe...
File remoting/protocol/channel_dispatcher_base.cc (right):

https://codereview.chromium.org/841773005/diff/20001/remoting/protocol/channe...
remoting/protocol/channel_dispatcher_base.cc:60: writer_.Init(channel_.get(),
base::Bind(&ChannelDispatcherBase::OnWriteFailed,
On 2015/01/08 02:22:31, rmsousa wrote:
> This means that every child of ChannelDispatcherBase will now start both a
> reader (that discards everything because the callback is null) and a writer.
> 
> Questions:
> * Is there such a thing as a read-only/write-only channel, and would this
cause
> a problem in that case?

Video and Audio channels are currently used in only one direction

> * As far as I can tell, each ChannelDispatcherBase creates its own channel -
> but, just to confirm, there's no case where we create separate "reader" and
> "writer" ChannelDispatchers for hte same channel, right? (i.e. there's no
chance
> that a "writer" ChannelDispatcherBase would read and discard messages that
> another "reader" ChannelDispatcherBase would be interested in)?

Right. ChannelDispatcherBase owns the channel, and there is no way to access it
from somewhere else.

> * Any other unintended consequences of running a reader in channels wehre we
> didn't previously run one? (I assume running hte writer is fine as long as we
> don't write anything).

The main difference is that if for some reason a peer sends some data where it's
not expected the receiver will now read and discard the data instead of just
holding it the TCP buffers as it was before. This behavior actually makes
handling of unknown messages more consistent - dispatchers normally drop
messages they do not understand.

https://codereview.chromium.org/841773005/diff/20001/remoting/protocol/messag...
File remoting/protocol/message_reader.h (right):

https://codereview.chromium.org/841773005/diff/20001/remoting/protocol/messag...
remoting/protocol/message_reader.h:39: explicit MessageReader();
On 2015/01/08 02:22:31, rmsousa wrote:
> nit: explicit with zero arguments?

Done.

[rmsousa, 2015-01-08 23:08:43]

lgtm

[Sergey Ulanov, 2015-01-10 07:48:02]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-01-10 07:48:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/841773005/40001

[commit-bot: I haz the power, 2015-01-10 07:51:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-10 07:51:40]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sergey Ulanov, 2015-01-10 20:26:40]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-01-10 20:27:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/841773005/60001

[commit-bot: I haz the power, 2015-01-10 21:44:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-10 21:45:27]

Patchset 4 (id:??) landed as
https://crrev.com/203832d43af2d2c0cb92ca2a6ad3e59193c6836d
Cr-Commit-Position: refs/heads/master@{#310993}