Add support for multiplexed channels in remoting::protocol::Session interface.

Add support for multiplexed channels in remoting::protocol::Session interface.

Now the Session interface has two methods that return channel factories - one for regular channels and one for multiplexed channels.
Also refactored AudioReader and AudioWriter to inherit from 
ChannelDispatcherBase.

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

[Wez, 2012-08-15 00:52:23]

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/channel_mult...
File remoting/protocol/channel_multiplexer.cc (right):

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/channel_mult...
remoting/protocol/channel_multiplexer.cc:396: // If this is the first
multiplexed channel then start creation on the base
nit: ... then create the base channel ...

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#ne...
remoting/protocol/session.h:103: // multiplexed channels work over a single
regular channel.
The Session knows the configuration that is in-use, so I don't think there's any
need for callers to choose the ChannelFactory explicitly via the API; the
Session can implement ChannelFactory and call on to the
multiplexed/non-multiplexed factory based on the configuration data for the
named channel?

That would also mean you could replace the Session parameter to the various
channel readers & writers with ChannelFactory.

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#ne...
remoting/protocol/session.h:104: virtual ChannelFactory* GetMainChannelFactory()
= 0;
nit: GetSeparateChannelFactory()/GetIndependentChannelFactory()?

[Sergey Ulanov, 2012-08-15 01:35:42]

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/channel_mult...
File remoting/protocol/channel_multiplexer.cc (right):

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/channel_mult...
remoting/protocol/channel_multiplexer.cc:396: // If this is the first
multiplexed channel then start creation on the base
On 2012/08/15 00:52:23, Wez wrote:
> nit: ... then create the base channel ...

Done.

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#ne...
remoting/protocol/session.h:103: // multiplexed channels work over a single
regular channel.
On 2012/08/15 00:52:23, Wez wrote:
> The Session knows the configuration that is in-use, so I don't think there's
any
> need for callers to choose the ChannelFactory explicitly via the API; the
> Session can implement ChannelFactory and call on to the
> multiplexed/non-multiplexed factory based on the configuration data for the
> named channel?
> 
> That would also mean you could replace the Session parameter to the various
> channel readers & writers with ChannelFactory.

I don't think that would be the right approach.
 (1) channels may not always map to configuration. E.g. when we had RTP
reader/writer they were creating two separate channels - one for RTP and one for
RTCP.
 (2) Channel configuration is normally processed by the upper layer. E.g. if
event reader/writer supported datagram-based channel they would have to decide
to create a stream a datagram channel instead of a stream. We should try not to
spread that logic through several layers. The session layer just provides a way
to establish a session and connect channels. Yes, it also takes care of
negotiating session config, but that doesn't mean it has to change it's behavior
based on that configuration

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#ne...
remoting/protocol/session.h:104: virtual ChannelFactory* GetMainChannelFactory()
= 0;
On 2012/08/15 00:52:23, Wez wrote:
> nit: GetSeparateChannelFactory()/GetIndependentChannelFactory()?

What do you think about GetBaseChannelFactory?

[Wez, 2012-08-16 00:45:00]

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#ne...
remoting/protocol/session.h:103: // multiplexed channels work over a single
regular channel.
On 2012/08/15 01:35:42, sergeyu wrote:
> On 2012/08/15 00:52:23, Wez wrote:
> > The Session knows the configuration that is in-use, so I don't think there's
> any
> > need for callers to choose the ChannelFactory explicitly via the API; the
> > Session can implement ChannelFactory and call on to the
> > multiplexed/non-multiplexed factory based on the configuration data for the
> > named channel?
> > 
> > That would also mean you could replace the Session parameter to the various
> > channel readers & writers with ChannelFactory.
> 
> I don't think that would be the right approach.
>  (1) channels may not always map to configuration. E.g. when we had RTP
> reader/writer they were creating two separate channels - one for RTP and one
for
> RTCP.
>  (2) Channel configuration is normally processed by the upper layer. E.g. if
> event reader/writer supported datagram-based channel they would have to decide
> to create a stream a datagram channel instead of a stream. We should try not
to
> spread that logic through several layers. The session layer just provides a
way
> to establish a session and connect channels. Yes, it also takes care of
> negotiating session config, but that doesn't mean it has to change it's
behavior
> based on that configuration

Fair points. :(

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#ne...
remoting/protocol/session.h:104: virtual ChannelFactory* GetMainChannelFactory()
= 0;
On 2012/08/15 01:35:42, sergeyu wrote:
> On 2012/08/15 00:52:23, Wez wrote:
> > nit: GetSeparateChannelFactory()/GetIndependentChannelFactory()?
> 
> What do you think about GetBaseChannelFactory?

"Base" doesn't really convey "not-multiplexed", though. :-/

GetDedicatedChannelFactory(), perhaps? Or GetTransportChannelFactory()?

[Sergey Ulanov, 2012-08-16 00:59:48]

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#ne...
remoting/protocol/session.h:104: virtual ChannelFactory* GetMainChannelFactory()
= 0;
On 2012/08/16 00:45:00, Wez wrote:
> On 2012/08/15 01:35:42, sergeyu wrote:
> > On 2012/08/15 00:52:23, Wez wrote:
> > > nit: GetSeparateChannelFactory()/GetIndependentChannelFactory()?
> > 
> > What do you think about GetBaseChannelFactory?
> 
> "Base" doesn't really convey "not-multiplexed", though. :-/
> 
> GetDedicatedChannelFactory(), perhaps? Or GetTransportChannelFactory()?

Ok, changed it to GetTransportChannelFactory()

[Wez, 2012-08-18 00:30:35]

LGTM w/ nits

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_rea...
File remoting/protocol/audio_reader.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:29: typedef base::Callback<void(bool)>
InitializedCallback;
Remove this.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:35: static scoped_ptr<AudioReader> Create(const
SessionConfig& config);
nit: While you're improving this API, shouldn't Create() appear before the dtor
declaration, and have a single-line comment explaining how to configure
newly-created AudioReaders (e.g. it's no longer obvious how it learns about the
Session)

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.cc (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.cc:26: // TODO(sergeyu): Provide
WriteFailedCallback for the buffered writer.
nit: You mean provide an non-NULL callback?

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:35: typedef base::Callback<void(bool)>
InitializedCallback;
Remove this.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:37: static scoped_ptr<AudioWriter> Create(const
SessionConfig& config);
nit: Comment to clarify how to configure the AudioWriter with a Session to use.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/fake_sess...
File remoting/protocol/fake_session.cc (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/fake_sess...
remoting/protocol/fake_session.cc:333: return NULL;
nit: Return this here, so that tests can test code that tries to create
multiplexed channels?

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.cc:200: ChannelFactory*
JingleSession::GetTransportChannelFactory() {
nit: CalledOnValidThread?

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.cc:204: ChannelFactory*
JingleSession::GetMultiplexedChannelFactory() {
nit: CalledOnValidThread?

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/protobuf_...
File remoting/protocol/protobuf_video_reader.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/protobuf_...
remoting/protocol/protobuf_video_reader.h:39: ChannelFactory* channel_factory_;
nit: Place this near |channel_|?

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/protobuf_...
File remoting/protocol/protobuf_video_writer.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/protobuf_...
remoting/protocol/protobuf_video_writer.h:45: ChannelFactory* channel_factory_;
nit: Place near |channel_|?

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/session.h...
remoting/protocol/session.h:103: // multiplexed channels work over a single
regular channel.
Suggest reword:
"GetTransportChannelFactory returns a factory that creates a new transport
channel for each logical channel. GetMultiplexedChannelFactory channels share a
single underlying transport channel."

[Sergey Ulanov, 2012-08-18 01:53:56]

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_rea...
File remoting/protocol/audio_reader.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:29: typedef base::Callback<void(bool)>
InitializedCallback;
On 2012/08/18 00:30:36, Wez wrote:
> Remove this.

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:35: static scoped_ptr<AudioReader> Create(const
SessionConfig& config);
On 2012/08/18 00:30:36, Wez wrote:
> nit: While you're improving this API, shouldn't Create() appear before the
dtor
> declaration, and have a single-line comment explaining how to configure
> newly-created AudioReaders (e.g. it's no longer obvious how it learns about
the
> Session)

Strictly speaking the style guide says that constructor/destructor always go
first. But we can look at this method as a named constructor.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.cc (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.cc:26: // TODO(sergeyu): Provide
WriteFailedCallback for the buffered writer.
On 2012/08/18 00:30:36, Wez wrote:
> nit: You mean provide an non-NULL callback?

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:35: typedef base::Callback<void(bool)>
InitializedCallback;
On 2012/08/18 00:30:36, Wez wrote:
> Remove this.

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:37: static scoped_ptr<AudioWriter> Create(const
SessionConfig& config);
On 2012/08/18 00:30:36, Wez wrote:
> nit: Comment to clarify how to configure the AudioWriter with a Session to
use.

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/fake_sess...
File remoting/protocol/fake_session.cc (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/fake_sess...
remoting/protocol/fake_session.cc:333: return NULL;
On 2012/08/18 00:30:36, Wez wrote:
> nit: Return this here, so that tests can test code that tries to create
> multiplexed channels?

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.cc:200: ChannelFactory*
JingleSession::GetTransportChannelFactory() {
On 2012/08/18 00:30:36, Wez wrote:
> nit: CalledOnValidThread?

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.cc:204: ChannelFactory*
JingleSession::GetMultiplexedChannelFactory() {
On 2012/08/18 00:30:36, Wez wrote:
> nit: CalledOnValidThread?

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/protobuf_...
File remoting/protocol/protobuf_video_reader.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/protobuf_...
remoting/protocol/protobuf_video_reader.h:39: ChannelFactory* channel_factory_;
On 2012/08/18 00:30:36, Wez wrote:
> nit: Place this near |channel_|?

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/protobuf_...
File remoting/protocol/protobuf_video_writer.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/protobuf_...
remoting/protocol/protobuf_video_writer.h:45: ChannelFactory* channel_factory_;
On 2012/08/18 00:30:36, Wez wrote:
> nit: Place near |channel_|?

Done.

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/session.h...
remoting/protocol/session.h:103: // multiplexed channels work over a single
regular channel.
On 2012/08/18 00:30:36, Wez wrote:
> Suggest reword:
> "GetTransportChannelFactory returns a factory that creates a new transport
> channel for each logical channel. GetMultiplexedChannelFactory channels share
a
> single underlying transport channel."

Done.

[commit-bot: I haz the power, 2012-08-18 01:54:17]

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

[commit-bot: I haz the power, 2012-08-18 04:17:26]

Change committed as 152240

[Wez, 2012-08-18 21:44:05]

https://chromiumcodereview.appspot.com/10823323/diff/2002/remoting/protocol/a...
File remoting/protocol/audio_reader.h (right):

https://chromiumcodereview.appspot.com/10823323/diff/2002/remoting/protocol/a...
remoting/protocol/audio_reader.h:35: static scoped_ptr<AudioReader> Create(const
SessionConfig& config);
On 2012/08/18 01:53:56, sergeyu wrote:
> On 2012/08/18 00:30:36, Wez wrote:
> > nit: While you're improving this API, shouldn't Create() appear before the
> dtor
> > declaration, and have a single-line comment explaining how to configure
> > newly-created AudioReaders (e.g. it's no longer obvious how it learns about
> the
> > Session)
> 
> Strictly speaking the style guide says that constructor/destructor always go
> first. But we can look at this method as a named constructor.

Yes, I think that makes sense.