Remove video_channel() from Session interface

Remove video_channel() from Session interface

BUG=None
TEST=Unittests.

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

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

[Wez, 2011-08-04 23:49:52]

The copyright years need updating, and I have a few comments (below).  Otherwise
LGTM!

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/jingle_sessi...
File remoting/protocol/jingle_session_unittest.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/jingle_sessi...
remoting/protocol/jingle_session_unittest.cc:573: socket_1_ = socket;
nit: Simpler to use an array of two sockets, here and above?

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/protobuf_vid...
File remoting/protocol/protobuf_video_writer.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/protobuf_vid...
remoting/protocol/protobuf_video_writer.cc:27: LOG(ERROR) << " creating
channel?";
nit: Errant debug logging?

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/protobuf_vid...
remoting/protocol/protobuf_video_writer.cc:57: LOG(ERROR) << "Writing!";
nit: More errant logging?

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_re...
File remoting/protocol/rtp_video_reader.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_re...
remoting/protocol/rtp_video_reader.cc:75: initialized_ = true;
Need to set |initialize_| before invoking callback, in case callback tears us
down?

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
File remoting/protocol/rtp_video_writer.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
remoting/protocol/rtp_video_writer.cc:47: initialized_ = true;
Set before callback?

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
remoting/protocol/rtp_video_writer.cc:65: initialized_ = true;
Set before callback?

Do we really need the |initialized_| flag?

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
File remoting/protocol/rtp_video_writer.h (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
remoting/protocol/rtp_video_writer.h:35: bool initialized_;
Should be |is_initialized_| here and in reader.

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/video_reader.cc
File remoting/protocol/video_reader.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/video_reader...
remoting/protocol/video_reader.cc:19: LOG(ERROR) << "Creating " <<
video_config.codec;
nit: Errant logging.

[Sergey Ulanov, 2011-08-09 19:41:10]

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/jingle_sessi...
File remoting/protocol/jingle_session_unittest.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/jingle_sessi...
remoting/protocol/jingle_session_unittest.cc:573: socket_1_ = socket;
On 2011/08/04 23:49:52, Wez wrote:
> nit: Simpler to use an array of two sockets, here and above?

Done.

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/protobuf_vid...
File remoting/protocol/protobuf_video_writer.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/protobuf_vid...
remoting/protocol/protobuf_video_writer.cc:27: LOG(ERROR) << " creating
channel?";
On 2011/08/04 23:49:52, Wez wrote:
> nit: Errant debug logging?

Done.

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/protobuf_vid...
remoting/protocol/protobuf_video_writer.cc:57: LOG(ERROR) << "Writing!";
On 2011/08/04 23:49:52, Wez wrote:
> nit: More errant logging?

Done.

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_re...
File remoting/protocol/rtp_video_reader.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_re...
remoting/protocol/rtp_video_reader.cc:75: initialized_ = true;
On 2011/08/04 23:49:52, Wez wrote:
> Need to set |initialize_| before invoking callback, in case callback tears us
> down?

Done.

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
File remoting/protocol/rtp_video_writer.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
remoting/protocol/rtp_video_writer.cc:47: initialized_ = true;
On 2011/08/04 23:49:52, Wez wrote:
> Set before callback?

Done.

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
remoting/protocol/rtp_video_writer.cc:65: initialized_ = true;
On 2011/08/04 23:49:52, Wez wrote:
> Set before callback?
> 
> Do we really need the |initialized_| flag?
Yes, we do. It's for the case when initialization of the first channel fails,
but then the second channel is connected successfully. We should not call the
callback again in that case.

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
File remoting/protocol/rtp_video_writer.h (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/rtp_video_wr...
remoting/protocol/rtp_video_writer.h:35: bool initialized_;
On 2011/08/04 23:49:52, Wez wrote:
> Should be |is_initialized_| here and in reader.
I think |initialized_| is much more common in the chromium code.

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/video_reader.cc
File remoting/protocol/video_reader.cc (right):

http://codereview.chromium.org/7508044/diff/29/remoting/protocol/video_reader...
remoting/protocol/video_reader.cc:19: LOG(ERROR) << "Creating " <<
video_config.codec;
On 2011/08/04 23:49:52, Wez wrote:
> nit: Errant logging.

Done.

[Wez, 2011-08-09 19:47:40]

LGTM

[Sergey Ulanov, 2011-08-10 18:42:14]

PTAL: I've added new session state: CONNECTED_CHANNELS, so now it is possible to
create all channels at the same time.

[Wez, 2011-08-10 23:36:52]

LGTM

http://codereview.chromium.org/7508044/diff/15001/remoting/protocol/connectio...
File remoting/protocol/connection_to_host.cc (right):

http://codereview.chromium.org/7508044/diff/15001/remoting/protocol/connectio...
remoting/protocol/connection_to_host.cc:227:
event_callback_->OnConnectionOpened(this);
Move this into a separate NotifyIfChannelsReady() or somthing like that, which
checks for all three being connected?

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

http://codereview.chromium.org/7508044/diff/15001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.cc:438: SetState(CONNECTED);
From the caller's point of view, this isn't "connected", since there are no
channels; it could be "preparing_channels" or "channel_setup" or something like
that?

[Sergey Ulanov, 2011-08-11 00:33:47]

http://codereview.chromium.org/7508044/diff/15001/remoting/protocol/connectio...
File remoting/protocol/connection_to_host.cc (right):

http://codereview.chromium.org/7508044/diff/15001/remoting/protocol/connectio...
remoting/protocol/connection_to_host.cc:227:
event_callback_->OnConnectionOpened(this);
On 2011/08/10 23:36:52, Wez wrote:
> Move this into a separate NotifyIfChannelsReady() or somthing like that, which
> checks for all three being connected?

Done.

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

http://codereview.chromium.org/7508044/diff/15001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.cc:438: SetState(CONNECTED);
On 2011/08/10 23:36:52, Wez wrote:
> From the caller's point of view, this isn't "connected", since there are no
> channels; it could be "preparing_channels" or "channel_setup" or something
like
> that?

I will remove CONNECTED_CHANNELS state later when I remove control/event
channels from Session interface. After that the CONNECTED state will make sense.