Fix WebRTC transport to use single offer-answer exchange instead of two.

Fix WebRTC transport to use single offer-answer exchange instead of two.

Previously when using WebRTC protocol 2 offer-answer roundtrips were
necessary. First one without media stream, and second one with media
stream. That was happening because VideoStream was created only after
data channels are connected. Now VideoStream is created as soon as the
connection is authenticated.

BUG=547158
Committed: https://crrev.com/9102cff28634777eecc5554be98d33307c206bb8
Cr-Commit-Position: refs/heads/master@{#385830}

[Sergey Ulanov, 2016-04-02 00:49:18]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-04-02 00:49:36]

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

[kelvinp, 2016-04-02 06:22:18]

On 2016/04/02 00:49:36, Sergey Ulanov wrote:

Do I need to make any corresponding change on the client-side?

[Sergey Ulanov, 2016-04-04 18:10:20]

On 2016/04/02 06:22:18, kelvinp wrote:
> On 2016/04/02 00:49:36, Sergey Ulanov wrote:
> 
> Do I need to make any corresponding change on the client-side?

No changes should be required on the client side. This change doesn't include
anything for the client, but the tests still pass.

[Jamie, 2016-04-04 18:21:47]

https://codereview.chromium.org/1852033002/diff/20001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/1852033002/diff/20001/remoting/host/client_se...
remoting/host/client_session.h:234: // client. Until then it's stored in
|pending_video_layout_message_|.
Maybe expand on this a bit to explain why the message can't be sent until then
(since it's not conditional on what the capabilities are). If you think that's
too much detail for the header, maybe move it to the implementation.

Is it a client-side limitation, and if so can it be fixed there?

https://codereview.chromium.org/1852033002/diff/20001/remoting/protocol/conne...
File remoting/protocol/connection_to_client.h (right):

https://codereview.chromium.org/1852033002/diff/20001/remoting/protocol/conne...
remoting/protocol/connection_to_client.h:45: // after
OnConnectionChannelsConnected().
If this is a method to request that the implementation does something, rather
than a notification that something has happened, then I don't think calling it
On* is the right convention. Perhaps just CreateVideoStreams()?

[Sergey Ulanov, 2016-04-05 21:21:54]

https://codereview.chromium.org/1852033002/diff/20001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/1852033002/diff/20001/remoting/host/client_se...
remoting/host/client_session.h:234: // client. Until then it's stored in
|pending_video_layout_message_|.
On 2016/04/04 18:21:47, Jamie wrote:
> Maybe expand on this a bit to explain why the message can't be sent until then
> (since it's not conditional on what the capabilities are). If you think that's
> too much detail for the header, maybe move it to the implementation.

My thinking was that in the future we may need to change the format of that
messages depending on the capabilities. But looking at this today, I don't think
we really need this right now, so I removed this requirement. In any case the
message can be sent only after control channel is connected, so we still need
this variable.

> 
> Is it a client-side limitation, and if so can it be fixed there?

No, it's not client-side limitation.

https://codereview.chromium.org/1852033002/diff/20001/remoting/protocol/conne...
File remoting/protocol/connection_to_client.h (right):

https://codereview.chromium.org/1852033002/diff/20001/remoting/protocol/conne...
remoting/protocol/connection_to_client.h:45: // after
OnConnectionChannelsConnected().
On 2016/04/04 18:21:47, Jamie wrote:
> If this is a method to request that the implementation does something, rather
> than a notification that something has happened, then I don't think calling it
> On* is the right convention. Perhaps just CreateVideoStreams()?

Done.

[Jamie, 2016-04-05 21:46:36]

LGTM unless you take my suggesting of making the queuing more generic (and even
then it's probably better done as a separate CL).

https://codereview.chromium.org/1852033002/diff/40001/remoting/host/client_se...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/1852033002/diff/40001/remoting/host/client_se...
remoting/host/client_session.cc:462: // is necessary for the case in the future.
I don't understand this second sentence. Are you saying that a future update
will make use of the VideoLayout message or that a future update will restrict
it to be sent only when the control channel is connected? Either way, I don't
think you need the second sentence, but feel free to clarify if you meant
something else.

https://codereview.chromium.org/1852033002/diff/40001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/1852033002/diff/40001/remoting/host/client_se...
remoting/host/client_session.h:238: scoped_ptr<protocol::VideoLayout>
pending_video_layout_message_;
Optional: Would it be better to queue this in the client stub instead of here?
The fact that this is sent over the control channel seems like an implementation
detail that this class shouldn't have to care about. In fact, it seems like
there might be any number of messages that may need to be queued pending
connection establishment, and they could be queued in a generic way at an
appropriate layer.

[Sergey Ulanov, 2016-04-06 21:23:50]

https://codereview.chromium.org/1852033002/diff/40001/remoting/host/client_se...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/1852033002/diff/40001/remoting/host/client_se...
remoting/host/client_session.cc:462: // is necessary for the case in the future.
On 2016/04/05 21:46:35, Jamie wrote:
> I don't understand this second sentence. Are you saying that a future update
> will make use of the VideoLayout message or that a future update will restrict
> it to be sent only when the control channel is connected? Either way, I don't
> think you need the second sentence, but feel free to clarify if you meant
> something else.

I don't understand it either :-/. Removed.

https://codereview.chromium.org/1852033002/diff/40001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/1852033002/diff/40001/remoting/host/client_se...
remoting/host/client_session.h:238: scoped_ptr<protocol::VideoLayout>
pending_video_layout_message_;
On 2016/04/05 21:46:36, Jamie wrote:
> Optional: Would it be better to queue this in the client stub instead of here?
> The fact that this is sent over the control channel seems like an
implementation
> detail that this class shouldn't have to care about. In fact, it seems like
> there might be any number of messages that may need to be queued pending
> connection establishment, and they could be queued in a generic way at an
> appropriate layer.

That's a great suggestions. Queuing actually works for all stubs when using
IceConnection objects (which use BufferedSocketWriter internally), but not for
WebRTC connections. I've added TODO.

[Sergey Ulanov, 2016-04-06 21:24:26]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-04-06 21:24:27]

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/1852033002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-04-06 21:24:59]

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

[commit-bot: I haz the power, 2016-04-06 23:05:14]

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

[commit-bot: I haz the power, 2016-04-06 23:05:15]

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

[Sergey Ulanov, 2016-04-07 00:57:24]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-04-07 00:57:49]

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

[commit-bot: I haz the power, 2016-04-07 01:07:34]

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

[commit-bot: I haz the power, 2016-04-07 01:07:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2016-04-07 18:24:04]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-04-07 18:24:05]

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/1852033002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-04-07 18:24:52]

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

[commit-bot: I haz the power, 2016-04-07 19:14:14]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-04-07 19:16:50]

Description was changed from

==========
Fix WebRTC transport to use single offer-answer exchange instead of two.

Previously when using WebRTC protocol 2 offer-answer roundtrips were
necessary. First one without media stream, and second one with media
stream. That was happening because VideoStream was created only after
data channels are connected. Now VideoStream is created as soon as the
connection is authenticated.

BUG=547158
==========

to

==========
Fix WebRTC transport to use single offer-answer exchange instead of two.

Previously when using WebRTC protocol 2 offer-answer roundtrips were
necessary. First one without media stream, and second one with media
stream. That was happening because VideoStream was created only after
data channels are connected. Now VideoStream is created as soon as the
connection is authenticated.

BUG=547158

Committed: https://crrev.com/9102cff28634777eecc5554be98d33307c206bb8
Cr-Commit-Position: refs/heads/master@{#385830}
==========

[commit-bot: I haz the power, 2016-04-07 19:16:51]

Patchset 4 (id:??) landed as
https://crrev.com/9102cff28634777eecc5554be98d33307c206bb8
Cr-Commit-Position: refs/heads/master@{#385830}