Simplify VideoReader and VideoWriter

Simplify VideoReader and VideoWriter

Previously VideoReader/Writer were abstract interfaces with single
implementation. Now both the reader and writer are concrete
implementations reusing ChannelDispatcherBase, which significantly
simplified both.

BUG=402993
Committed: https://crrev.com/2385cffed5c8a258b29d76160434635b66ea9968
Cr-Commit-Position: refs/heads/master@{#295760}

[Sergey Ulanov, 2014-09-15 20:49:02]

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

[Wez, 2014-09-16 23:10:07]

Is it worth adding some unit-tests for these, as well, e.g. for cases like the
BufferedWriter trying to invoke the write-failure callback?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:138:
video_writer_->Init(session_.get(),
nit: Does Init() need to be separate from construction, or could we combine the
two?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
File remoting/protocol/video_reader.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.cc:18: :
ChannelDispatcherBase(kVideoChannelName),
nit: Should the channel name be a parameter to the VideoReader?

If not then isn't VideoReader a too generic name - perhaps VideoChannel or
VideoChannelPump might be better?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
File remoting/protocol/video_reader.h (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:29: void set_video_stub(VideoStub* video_stub)
{ video_stub_ = video_stub; }
Why does this need a setter rather than being a constructor parameter?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:37: const base::Closure& done_task);
OnVideoPacket?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:39: StreamChannelFactory* channel_factory_;
This doesn't seem to be used?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:40: ProtobufMessageReader<VideoPacket> reader_;
It ends up looking a little strange to have VideoReader then contain a class
that does all the actual reading; this outer class is really just a pump between
a StreamSocket and a VideoStub.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:42: // The stub that processes all received
packets.
Or "Interface to which VideoPackets are passed for processing."?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
File remoting/protocol/video_writer.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:8: #include "net/socket/stream_socket.h"
Is StreamSocket actually used here, or is the forward-decl sufficient?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:12: #include
"remoting/protocol/stream_channel_factory.h"
Not used?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:18: : ChannelDispatcherBase(kVideoChannelName)
{
Similar comment applies re channel name in VideoWriter; should this be
VideoChannelWriter or VideoChannelImpl or VideoChannel, for example?

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:25: // TODO(sergeyu): Provide
WriteFailedCallback for the buffered writer.
What will happen with it NULL like this? Crash?

[Sergey Ulanov, 2014-09-17 21:52:07]

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:138:
video_writer_->Init(session_.get(),
On 2014/09/16 23:10:06, Wez wrote:
> nit: Does Init() need to be separate from construction, or could we combine
the
> two?

Yes. It's implemented in the ChannelDispatcherBase. These parameters could be
passed to the constructor, but then they would have to be duplicated for each
class based on ChannelDispatcherBase

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
File remoting/protocol/video_reader.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.cc:18: :
ChannelDispatcherBase(kVideoChannelName),
On 2014/09/16 23:10:06, Wez wrote:
> nit: Should the channel name be a parameter to the VideoReader?

I don't think it needs to be - it's consistent with other channel dispatcher
classes.

> 
> If not then isn't VideoReader a too generic name - perhaps VideoChannel or
> VideoChannelPump might be better?

Renamed them to (Client|Host)VideoDispatcher for consistency with other
dispatchers

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
File remoting/protocol/video_reader.h (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:29: void set_video_stub(VideoStub* video_stub)
{ video_stub_ = video_stub; }
On 2014/09/16 23:10:07, Wez wrote:
> Why does this need a setter rather than being a constructor parameter?

This is consistent with other channel dispatchers.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:37: const base::Closure& done_task);
On 2014/09/16 23:10:06, Wez wrote:
> OnVideoPacket?

Done.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:39: StreamChannelFactory* channel_factory_;
On 2014/09/16 23:10:06, Wez wrote:
> This doesn't seem to be used?

Done.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:40: ProtobufMessageReader<VideoPacket> reader_;
On 2014/09/16 23:10:06, Wez wrote:
> It ends up looking a little strange to have VideoReader then contain a class
> that does all the actual reading; this outer class is really just a pump
between
> a StreamSocket and a VideoStub.

Renamed this class

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:42: // The stub that processes all received
packets.
On 2014/09/16 23:10:06, Wez wrote:
> Or "Interface to which VideoPackets are passed for processing."?

Done.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
File remoting/protocol/video_writer.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:8: #include "net/socket/stream_socket.h"
On 2014/09/16 23:10:07, Wez wrote:
> Is StreamSocket actually used here, or is the forward-decl sufficient?

It's needed to upcast net::StreamSocket to net::Socket when passing it to
BufferedWriter::Init()

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:12: #include
"remoting/protocol/stream_channel_factory.h"
On 2014/09/16 23:10:07, Wez wrote:
> Not used?

Done.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:18: : ChannelDispatcherBase(kVideoChannelName)
{
On 2014/09/16 23:10:07, Wez wrote:
> Similar comment applies re channel name in VideoWriter; should this be
> VideoChannelWriter or VideoChannelImpl or VideoChannel, for example?

Renamed

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:25: // TODO(sergeyu): Provide
WriteFailedCallback for the buffered writer.
On 2014/09/16 23:10:07, Wez wrote:
> What will happen with it NULL like this? Crash?

BufferedSocketWriter doesn't call it if it's NULL.

[Wez, 2014-09-18 00:23:55]

LGTM w/ a few remaining nits!

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:138:
video_writer_->Init(session_.get(),
On 2014/09/17 21:52:06, Sergey Ulanov wrote:
> On 2014/09/16 23:10:06, Wez wrote:
> > nit: Does Init() need to be separate from construction, or could we combine
> the
> > two?
> 
> Yes. It's implemented in the ChannelDispatcherBase. These parameters could be
> passed to the constructor, but then they would have to be duplicated for each
> class based on ChannelDispatcherBase

Acknowledged.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
File remoting/protocol/video_reader.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.cc:18: :
ChannelDispatcherBase(kVideoChannelName),
On 2014/09/17 21:52:06, Sergey Ulanov wrote:
> On 2014/09/16 23:10:06, Wez wrote:
> > nit: Should the channel name be a parameter to the VideoReader?
> 
> I don't think it needs to be - it's consistent with other channel dispatcher
> classes.
> 
> > 
> > If not then isn't VideoReader a too generic name - perhaps VideoChannel or
> > VideoChannelPump might be better?
> 
> Renamed them to (Client|Host)VideoDispatcher for consistency with other
> dispatchers

Acknowledged.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
File remoting/protocol/video_reader.h (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_read...
remoting/protocol/video_reader.h:29: void set_video_stub(VideoStub* video_stub)
{ video_stub_ = video_stub; }
On 2014/09/17 21:52:06, Sergey Ulanov wrote:
> On 2014/09/16 23:10:07, Wez wrote:
> > Why does this need a setter rather than being a constructor parameter?
> 
> This is consistent with other channel dispatchers.

Acknowledged.

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
File remoting/protocol/video_writer.cc (right):

https://codereview.chromium.org/577473002/diff/1/remoting/protocol/video_writ...
remoting/protocol/video_writer.cc:8: #include "net/socket/stream_socket.h"
On 2014/09/17 21:52:06, Sergey Ulanov wrote:
> On 2014/09/16 23:10:07, Wez wrote:
> > Is StreamSocket actually used here, or is the forward-decl sufficient?
> 
> It's needed to upcast net::StreamSocket to net::Socket when passing it to
> BufferedWriter::Init()

Acknowledged.

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/client...
File remoting/protocol/client_video_dispatcher.cc (right):

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/client...
remoting/protocol/client_video_dispatcher.cc:31:
video_stub_->ProcessVideoPacket(packet.Pass(), done_task);
nit to consider for a future CL: Since ProtobufVideoReader is also part of
remoting/protocol, perhaps we should just be Init()ing it with the VideoStub
directly, and cut out this middle-man?

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
File remoting/protocol/host_video_dispatcher.cc (right):

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.cc:30: const base::Closure& done) {
nit: Indentation

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
File remoting/protocol/host_video_dispatcher.h (right):

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.h:28: void OnInitialized();
Should this be protected, as in the client impl, rather than private?

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.h:28: void OnInitialized();
// ChannelDispatcherBase overrides.
virtual void OnInitialized() OVERRIDE;

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.h:30: 
nit: Errant blank line!

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.h:31: BufferedSocketWriter
buffered_writer_;
nit: In the ClientVideoDispatcher, the ProtobufVideoReader is just called
reader_, so consider calling this writer_?

[Sergey Ulanov, 2014-09-18 22:34:52]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2014-09-18 22:35:00]

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/client...
File remoting/protocol/client_video_dispatcher.cc (right):

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/client...
remoting/protocol/client_video_dispatcher.cc:31:
video_stub_->ProcessVideoPacket(packet.Pass(), done_task);
On 2014/09/18 00:23:55, Wez wrote:
> nit to consider for a future CL: Since ProtobufVideoReader is also part of
> remoting/protocol, perhaps we should just be Init()ing it with the VideoStub
> directly, and cut out this middle-man?

Done.

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
File remoting/protocol/host_video_dispatcher.cc (right):

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.cc:30: const base::Closure& done) {
On 2014/09/18 00:23:55, Wez wrote:
> nit: Indentation

Done.

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
File remoting/protocol/host_video_dispatcher.h (right):

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.h:28: void OnInitialized();
On 2014/09/18 00:23:55, Wez wrote:
> // ChannelDispatcherBase overrides.
> virtual void OnInitialized() OVERRIDE;

Done.

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.h:28: void OnInitialized();
On 2014/09/18 00:23:55, Wez wrote:
> Should this be protected, as in the client impl, rather than private?

Done.

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.h:30: 
On 2014/09/18 00:23:55, Wez wrote:
> nit: Errant blank line!

Done.

https://codereview.chromium.org/577473002/diff/20001/remoting/protocol/host_v...
remoting/protocol/host_video_dispatcher.h:31: BufferedSocketWriter
buffered_writer_;
On 2014/09/18 00:23:55, Wez wrote:
> nit: In the ClientVideoDispatcher, the ProtobufVideoReader is just called
> reader_, so consider calling this writer_?

Done.

[commit-bot: I haz the power, 2014-09-18 22:36:00]

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

[commit-bot: I haz the power, 2014-09-18 22:53:12]

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

[commit-bot: I haz the power, 2014-09-18 22:53:12]

Try jobs failed on following builders:
  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_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Sergey Ulanov, 2014-09-18 23:41:34]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-09-18 23:42:34]

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

[commit-bot: I haz the power, 2014-09-18 23:57:14]

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

[commit-bot: I haz the power, 2014-09-18 23:57:16]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2014-09-19 18:43:34]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-09-19 18:45:09]

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

[commit-bot: I haz the power, 2014-09-19 19:23:48]

Committed patchset #5 (id:80001) as 8c55a0543486c9a1ede6c8ac10a6bc78cf90dc62

[commit-bot: I haz the power, 2014-09-19 19:24:29]

Patchset 5 (id:??) landed as
https://crrev.com/2385cffed5c8a258b29d76160434635b66ea9968
Cr-Commit-Position: refs/heads/master@{#295760}