Add support for dynamic channels in WebrtcTransport.

Add support for dynamic channels in WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only if the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accept that channel. Also channels now can be
closed dynamically and the transport doesn't terminate connection when one
of the channels is closed.

BUG=621691
Committed: https://crrev.com/d059c46f227d0a491f0ce2d4b8a302beda5533b3
Cr-Commit-Position: refs/heads/master@{#406639}

[Sergey Ulanov, 2016-07-18 22:46:03]

Description was changed from

==========
Remove incoming_channel_factory() from WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only the the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accpt that channel. Another side effect is that
unsupported channels are closed instead of being left open.
==========

to

==========
Add support for dynamic channels in WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only the the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accept that channel. Also channels now can be 
closed dynamically and the transport doesn't terminate connection when one
of the channels is closed.

BUG=621691
==========

[Sergey Ulanov, 2016-07-18 22:46:13]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-07-18 23:14:57]

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

[Jamie, 2016-07-18 23:58:06]

Description was changed from

==========
Add support for dynamic channels in WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only the the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accept that channel. Also channels now can be 
closed dynamically and the transport doesn't terminate connection when one
of the channels is closed.

BUG=621691
==========

to

==========
Add support for dynamic channels in WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only if the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accept that channel. Also channels now can be 
closed dynamically and the transport doesn't terminate connection when one
of the channels is closed.

BUG=621691
==========

[Jamie, 2016-07-19 18:24:48]

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/chann...
File remoting/protocol/channel_dispatcher_base.h (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/chann...
remoting/protocol/channel_dispatcher_base.h:53: // Initialized the channel using
|message_pipe| that's already connected.
s/Initialized/Initializes/

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/messa...
File remoting/protocol/message_pipe.h (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/messa...
remoting/protocol/message_pipe.h:41: // Starts the channel. |event_handle| will
be called to notify when a message
s/handle/handler/

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/messa...
remoting/protocol/message_pipe.h:42: // is received of the pipe is closed.
s/of/or/

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.cc:158:
control_dispatcher_->Init(transport_->outgoing_channel_factory(), this);
Could this be initialized in OnWebrtcTransportIncomingDataChannel so that the
treatment of the control and event channels is symmetric? If not, please add a
comment explaining why the two channels are treated differently.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_data_stream_adapter.cc (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_data_stream_adapter.cc:100: return;
Should we log an error here? Silently dropping the packet doesn't seem right.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_data_stream_adapter.cc:124: // Currently channels are
not expected to be closed.
Log an error in this case?

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_data_stream_adapter.cc:151: case State::CONNECTING:
You're not setting the state to CLOSED in this branch; is that intentional?

[Sergey Ulanov, 2016-07-19 23:38:44]

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/chann...
File remoting/protocol/channel_dispatcher_base.h (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/chann...
remoting/protocol/channel_dispatcher_base.h:53: // Initialized the channel using
|message_pipe| that's already connected.
On 2016/07/19 18:24:47, Jamie wrote:
> s/Initialized/Initializes/

Done.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/messa...
File remoting/protocol/message_pipe.h (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/messa...
remoting/protocol/message_pipe.h:41: // Starts the channel. |event_handle| will
be called to notify when a message
On 2016/07/19 18:24:47, Jamie wrote:
> s/handle/handler/

Done.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/messa...
remoting/protocol/message_pipe.h:42: // is received of the pipe is closed.
On 2016/07/19 18:24:47, Jamie wrote:
> s/of/or/

Done.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.cc:158:
control_dispatcher_->Init(transport_->outgoing_channel_factory(), this);
On 2016/07/19 18:24:47, Jamie wrote:
> Could this be initialized in OnWebrtcTransportIncomingDataChannel so that the
> treatment of the control and event channels is symmetric? If not, please add a
> comment explaining why the two channels are treated differently.

Control channel is created by the host, while event channel is created by the
client.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_data_stream_adapter.cc (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_data_stream_adapter.cc:100: return;
On 2016/07/19 18:24:47, Jamie wrote:
> Should we log an error here? Silently dropping the packet doesn't seem right.

Replaced this with a DCHECK().
Also changed OnMessagePipeClosed() notification to synchronous. This means that
EventHandler is notified as soon as state changes and so it knows it cannot call
Send() anymore.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_data_stream_adapter.cc:124: // Currently channels are
not expected to be closed.
On 2016/07/19 18:24:47, Jamie wrote:
> Log an error in this case?

Removed the comment. This class now does support closing channels dynamically.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_data_stream_adapter.cc:151: case State::CONNECTING:
On 2016/07/19 18:24:47, Jamie wrote:
> You're not setting the state to CLOSED in this branch; is that intentional?

No. Fixed now.

[Jamie, 2016-07-20 00:31:39]

LGTM once the stray DCHECK is removed.

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.cc:158:
control_dispatcher_->Init(transport_->outgoing_channel_factory(), this);
On 2016/07/19 23:38:43, Sergey Ulanov wrote:
> On 2016/07/19 18:24:47, Jamie wrote:
> > Could this be initialized in OnWebrtcTransportIncomingDataChannel so that
the
> > treatment of the control and event channels is symmetric? If not, please add
a
> > comment explaining why the two channels are treated differently.
> 
> Control channel is created by the host, while event channel is created by the
> client.

It's beyond the scope of this CL, but would it be possible to have all channels
created by the same endpoint?

https://codereview.chromium.org/2146213002/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/2146213002/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.cc:206: DCHECK(false);
Left-over debugging?

[Sergey Ulanov, 2016-07-20 01:18:22]

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/2146213002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.cc:158:
control_dispatcher_->Init(transport_->outgoing_channel_factory(), this);
On 2016/07/20 00:31:39, Jamie wrote:
> On 2016/07/19 23:38:43, Sergey Ulanov wrote:
> > On 2016/07/19 18:24:47, Jamie wrote:
> > > Could this be initialized in OnWebrtcTransportIncomingDataChannel so that
> the
> > > treatment of the control and event channels is symmetric? If not, please
add
> a
> > > comment explaining why the two channels are treated differently.
> > 
> > Control channel is created by the host, while event channel is created by
the
> > client.
> 
> It's beyond the scope of this CL, but would it be possible to have all
channels
> created by the same endpoint?

I think we want to support creation of channels in both directions, so that
either end can create channels it needs. For example the host will create a
separate video_stats channels (adding in a separate CL) for each video stream
because the host controls the set of video streams. Similarly the client may
create channels for any new feature we add in the future. Pontially control
channel could be created by the client as well, but I wanted to create on the
host to make sure we support channel creation in both directions.

https://codereview.chromium.org/2146213002/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/2146213002/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.cc:206: DCHECK(false);
On 2016/07/20 00:31:39, Jamie wrote:
> Left-over debugging?

Done.

[Sergey Ulanov, 2016-07-20 01:18:28]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-07-20 01:18:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2146213002/#ps60001
(title: "No DCHECK")

[commit-bot: I haz the power, 2016-07-20 01:19:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 01:47:23]

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

[commit-bot: I haz the power, 2016-07-20 01:47:24]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sergey Ulanov, 2016-07-20 17:49:17]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-07-20 17:49:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2146213002/#ps80001
(title: "fix windows")

[commit-bot: I haz the power, 2016-07-20 17:49:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 18:22:39]

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

[commit-bot: I haz the power, 2016-07-20 18:22:41]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sergey Ulanov, 2016-07-20 18:39:46]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-07-20 18:39:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2146213002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2016-07-20 18:40:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 19:34:24]

Description was changed from

==========
Add support for dynamic channels in WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only if the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accept that channel. Also channels now can be 
closed dynamically and the transport doesn't terminate connection when one
of the channels is closed.

BUG=621691
==========

to

==========
Add support for dynamic channels in WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only if the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accept that channel. Also channels now can be 
closed dynamically and the transport doesn't terminate connection when one
of the channels is closed.

BUG=621691
==========

[commit-bot: I haz the power, 2016-07-20 19:34:26]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-07-20 19:34:30]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 19:35:35]

Description was changed from

==========
Add support for dynamic channels in WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only if the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accept that channel. Also channels now can be 
closed dynamically and the transport doesn't terminate connection when one
of the channels is closed.

BUG=621691
==========

to

==========
Add support for dynamic channels in WebrtcTransport.

Previously WebrtcTransport clients had to use incoming_channel_factory() to
accept incoming data channels. That worked only if the receiver knows in
advance names of all channels it can receive. Now the transport calls
EventHandler for incoming data channels, which allows the receiver to decide
dynamically if it wants to accept that channel. Also channels now can be
closed dynamically and the transport doesn't terminate connection when one
of the channels is closed.

BUG=621691

Committed: https://crrev.com/d059c46f227d0a491f0ce2d4b8a302beda5533b3
Cr-Commit-Position: refs/heads/master@{#406639}
==========

[commit-bot: I haz the power, 2016-07-20 19:35:37]

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