Remove event_channel() and control_channel() from Session interface.

Remove event_channel() and control_channel() from Session interface.

Now all channels are created using CreateStreamChannel() as they should be!

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

[Wez, 2011-11-17 22:39:40]

Why not expose whether the channel is connected or not from
ChannelDispatcherBase, so that callers need neither separate callback methods
for each channel, or an "is connected" boolean for each?

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
File remoting/protocol/channel_dispatcher_base.cc (right):

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
remoting/protocol/channel_dispatcher_base.cc:44:
initialized_callback_.Run(true);
Should we take a copy of the callback, in case OnInitialized destroys us, or is
that not allowed?

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
File remoting/protocol/channel_dispatcher_base.h (right):

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
remoting/protocol/channel_dispatcher_base.h:23: class ChannelDispatcherBase {
What is this class used for?

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
remoting/protocol/channel_dispatcher_base.h:29: explicit
ChannelDispatcherBase(const char* name);
What does |name| refer to?

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
remoting/protocol/channel_dispatcher_base.h:32: // Initializes the channel the
|session|.
typo?

[Sergey Ulanov, 2011-11-17 23:10:38]

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
File remoting/protocol/channel_dispatcher_base.cc (right):

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
remoting/protocol/channel_dispatcher_base.cc:44:
initialized_callback_.Run(true);
On 2011/11/17 22:39:41, Wez wrote:
> Should we take a copy of the callback, in case OnInitialized destroys us, or
is
> that not allowed?
It's not allowed. Added comments about it.

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
File remoting/protocol/channel_dispatcher_base.h (right):

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
remoting/protocol/channel_dispatcher_base.h:23: class ChannelDispatcherBase {
On 2011/11/17 22:39:41, Wez wrote:
> What is this class used for?

Done.

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
remoting/protocol/channel_dispatcher_base.h:29: explicit
ChannelDispatcherBase(const char* name);
On 2011/11/17 22:39:41, Wez wrote:
> What does |name| refer to?

renamed channel_name_

http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
remoting/protocol/channel_dispatcher_base.h:32: // Initializes the channel the
|session|.
On 2011/11/17 22:39:41, Wez wrote:
> typo?

Done.

[Sergey Ulanov, 2011-11-17 23:11:24]

On 2011/11/17 22:39:40, Wez wrote:
> Why not expose whether the channel is connected or not from
> ChannelDispatcherBase, so that callers need neither separate callback methods
> for each channel, or an "is connected" boolean for each?

Done. Also added IsConnected to VideoReader and VideoWriter interfaces.

> 
>
http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
> File remoting/protocol/channel_dispatcher_base.cc (right):
> 
>
http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
> remoting/protocol/channel_dispatcher_base.cc:44:
> initialized_callback_.Run(true);
> Should we take a copy of the callback, in case OnInitialized destroys us, or
is
> that not allowed?
> 
>
http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
> File remoting/protocol/channel_dispatcher_base.h (right):
> 
>
http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
> remoting/protocol/channel_dispatcher_base.h:23: class ChannelDispatcherBase {
> What is this class used for?
> 
>
http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
> remoting/protocol/channel_dispatcher_base.h:29: explicit
> ChannelDispatcherBase(const char* name);
> What does |name| refer to?
> 
>
http://codereview.chromium.org/8587053/diff/1/remoting/protocol/channel_dispa...
> remoting/protocol/channel_dispatcher_base.h:32: // Initializes the channel the
> |session|.
> typo?

[Wez, 2011-11-21 02:39:04]

lgtm

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
File remoting/protocol/channel_dispatcher_base.h (right):

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:23: // Base class for channel
dispatchers. It is responsible for creation
nit: What kind of dispatchers?  I/O dispatchers, perhaps?  Or message
dispatchers?  Derived dispatchers also typically handle both incoming and
outgoing I/O?

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:24: // of channel with the name
specified in the constructor.
nit: It's responsible for creating the named channel, which derived dispatchers
then dispatcher I/O (or messages) on?

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:31: explicit
ChannelDispatcherBase(const char* channel_name);
Should this be protected?

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:34: // Creates and connects channel
in the specified |session|. Called
typo: Called -> caller.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:34: // Creates and connects channel
in the specified |session|. Called
nit: connects _the_ channel.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:34: // Creates and connects channel
in the specified |session|. Called
nit: |session| -> Session.  (|session| isn't the type, Session is).

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:39: bool is_connected() { return
channel_.get() != NULL; }
nit: This could be { return channel() != NULL; }

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:290: LOG(ERROR) << "Received unexpected
session-terminate message.";
Is that really an error?

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/video_wri...
File remoting/protocol/video_writer.h (right):

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/video_wri...
remoting/protocol/video_writer.h:42: virtual bool is_connected() = 0;
nit: It seems clearer to put all set/get methods after all the non-set/get ones
in the interface & implementations.

[Sergey Ulanov, 2011-11-21 22:14:22]

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
File remoting/protocol/channel_dispatcher_base.h (right):

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:23: // Base class for channel
dispatchers. It is responsible for creation
On 2011/11/21 02:39:04, Wez wrote:
> nit: What kind of dispatchers?  I/O dispatchers, perhaps?  Or message
> dispatchers?  Derived dispatchers also typically handle both incoming and
> outgoing I/O?

Done.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:24: // of channel with the name
specified in the constructor.
On 2011/11/21 02:39:04, Wez wrote:
> nit: It's responsible for creating the named channel, which derived
dispatchers
> then dispatcher I/O (or messages) on?

Done.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:31: explicit
ChannelDispatcherBase(const char* channel_name);
On 2011/11/21 02:39:04, Wez wrote:
> Should this be protected?

Done.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:34: // Creates and connects channel
in the specified |session|. Called
On 2011/11/21 02:39:04, Wez wrote:
> typo: Called -> caller.

Done.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:34: // Creates and connects channel
in the specified |session|. Called
On 2011/11/21 02:39:04, Wez wrote:
> nit: connects _the_ channel.

Done.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:34: // Creates and connects channel
in the specified |session|. Called
On 2011/11/21 02:39:04, Wez wrote:
> nit: |session| -> Session.  (|session| isn't the type, Session is).

Done.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:39: bool is_connected() { return
channel_.get() != NULL; }
On 2011/11/21 02:39:04, Wez wrote:
> nit: This could be { return channel() != NULL; }

Done.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:290: LOG(ERROR) << "Received unexpected
session-terminate message.";
On 2011/11/21 02:39:04, Wez wrote:
> Is that really an error?
Changed to WARNING.

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/video_wri...
File remoting/protocol/video_writer.h (right):

http://codereview.chromium.org/8587053/diff/14002/remoting/protocol/video_wri...
remoting/protocol/video_writer.h:42: virtual bool is_connected() = 0;
On 2011/11/21 02:39:04, Wez wrote:
> nit: It seems clearer to put all set/get methods after all the non-set/get
ones
> in the interface & implementations.

Done.

[commit-bot: I haz the power, 2011-11-21 22:16:30]

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

[commit-bot: I haz the power, 2011-11-21 22:16:34]

Can't process patch for file remoting/protocol/client_control_dispatcher.h.
File's status is None, patchset upload is incomplete.