Add TransportSession interface to prepare for WebRTC-based transport.

Add TransportSession interface to prepare for WebRTC-based transport.

Previosly JingleSession was responsible to establishing connection
with the peer and managing list of channels. Now the new
IceTransportSession class (that implements TransportSession interface)
is responsible for managing list of channels and so it will be possible
to add support for WebRTC-based transport by adding another
TransportSession implementation.

There are no functional changes in this CL.

BUG=547158
Committed: https://crrev.com/7a5351f4c6e407e456978f6599d475616a69fc0b
Cr-Commit-Position: refs/heads/master@{#356588}

Committed: https://crrev.com/0b6c1c8d449971599b6b9700defd96c74b60a9f8
Cr-Commit-Position: refs/heads/master@{#356638}

[Sergey Ulanov, 2015-10-23 19:31:54]

Description was changed from

==========
Add TransportSession interface to prepare for WebRTC-based transport.

Previosly JingleSession was responsible to establishing connection
with the peer and managing list of channels. Now the new
IceTransportSession class (that implements TransportSession interface)
is responsible for managing list of channels and so it will be possible
to add support for WebRTC-based transport by adding another
TransportSession implementation.

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

to

==========
Add TransportSession interface to prepare for WebRTC-based transport.

Previosly JingleSession was responsible to establishing connection
with the peer and managing list of channels. Now the new
IceTransportSession class (that implements TransportSession interface)
is responsible for managing list of channels and so it will be possible
to add support for WebRTC-based transport by adding another
TransportSession implementation.

There are no functional changes in this CL.

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

[Sergey Ulanov, 2015-10-23 19:31:54]

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

[Sergey Ulanov, 2015-10-23 19:35:21]

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/jingl...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/jingl...
remoting/protocol/jingle_session.h:46: StreamChannelFactory*
GetQuicChannelFactory() override;
Note that this can potentially be moved to TransportSession too, but it would
significatnly complicate this CL because, so I decided to keep it here for now
in case we decide to get rid of QIUC completely.

[Jamie, 2015-10-27 21:32:27]

Is there any part of this CL you'd like me to pay particular attention to? It
sounds like most of it is copy/paste, in which case I trust you (and the
compiler) to catch any errors, but if there's anything more subtle, please point
it out and I'll look more closely.

LGTM if there's nothing of that sort.

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/clien...
File remoting/protocol/client_video_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/clien...
remoting/protocol/client_video_dispatcher_unittest.cc:76:
session_.GetTransportSession()->GetStreamChannelFactory()->GetFakeChannel(
Break before GetFakeChannel, for consistency with previous instances.

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_session.cc (right):

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_session.cc:27: NOTIMPLEMENTED();
Why NOTIMPLEMENTED for this method vs. NOTREACHED for the others?

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/ice_t...
File remoting/protocol/ice_transport_session.cc (right):

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/ice_t...
remoting/protocol/ice_transport_session.cc:48: for
(std::list<IceTransportInfo::IceCredentials>::const_iterator it =
Can you use auto here?

[Sergey Ulanov, 2015-10-27 23:03:52]

On 2015/10/27 21:32:27, Jamie wrote:
> Is there any part of this CL you'd like me to pay particular attention to? It
> sounds like most of it is copy/paste, in which case I trust you (and the
> compiler) to catch any errors, but if there's anything more subtle, please
point
> it out and I'll look more closely.

Yes. Mostly this CL is about moving some code from JingleSession to
IceTransportSession. Other than that the only interesting portion of this CL is
the new TransportSession interface in transport.h (which I plan to rename to
Transport in a separate CL).

[Sergey Ulanov, 2015-10-27 23:03:58]

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/clien...
File remoting/protocol/client_video_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/clien...
remoting/protocol/client_video_dispatcher_unittest.cc:76:
session_.GetTransportSession()->GetStreamChannelFactory()->GetFakeChannel(
On 2015/10/27 21:32:27, Jamie wrote:
> Break before GetFakeChannel, for consistency with previous instances.

This is the way clang-format formats this code.

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_session.cc (right):

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_session.cc:27: NOTIMPLEMENTED();
On 2015/10/27 21:32:27, Jamie wrote:
> Why NOTIMPLEMENTED for this method vs. NOTREACHED for the others?

Potentially we could implement GetDatagramChannelFactory() if some tests needed
it.
Start() and ProcessTransportInfo() are intended to be called by the session
signaling layer, i.e. FakeSession, but FakeSession doesn't call these methods,
so it wouldn't make sense to implement Start() and ProcessTransportInfo() as
long as FakeTransportSession is created only by FakeSession.

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/ice_t...
File remoting/protocol/ice_transport_session.cc (right):

https://codereview.chromium.org/1420273002/diff/20001/remoting/protocol/ice_t...
remoting/protocol/ice_transport_session.cc:48: for
(std::list<IceTransportInfo::IceCredentials>::const_iterator it =
On 2015/10/27 21:32:27, Jamie wrote:
> Can you use auto here?

Done.

[Jamie, 2015-10-27 23:46:00]

Thanks for narrowing it down for me :)

https://codereview.chromium.org/1420273002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport.h (right):

https://codereview.chromium.org/1420273002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport.h:146: // |transport_info| is in invalid format.
Returning true on error seems like an odd choice. Typically, I'd expect a method
called Process* to return true if it has successfully processed the input, which
it hasn't in the case of error. Perhaps it makes sense if false is reserved for
some sort of "pending" state, but then I think you need more documentation about
when it should return true vs. false.

[Sergey Ulanov, 2015-10-28 17:23:40]

https://codereview.chromium.org/1420273002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport.h (right):

https://codereview.chromium.org/1420273002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport.h:146: // |transport_info| is in invalid format.
On 2015/10/27 23:46:00, Jamie wrote:
> Returning true on error seems like an odd choice. Typically, I'd expect a
method
> called Process* to return true if it has successfully processed the input,
which
> it hasn't in the case of error. Perhaps it makes sense if false is reserved
for
> some sort of "pending" state, but then I think you need more documentation
about
> when it should return true vs. false.

The comment was incorrect. The implementation actually returns false when the
message is invalid. Fixed the comment.

[Jamie, 2015-10-28 17:26:56]

lgtm

[Sergey Ulanov, 2015-10-28 17:47:31]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-10-28 17:47:40]

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

[commit-bot: I haz the power, 2015-10-28 18:24:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-28 18:26:39]

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

[Georges Khalil, 2015-10-28 18:46:25]

On 2015/10/28 18:26:39, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/7a5351f4c6e407e456978f6599d475616a69fc0b
> Cr-Commit-Position: refs/heads/master@{#356588}

Looks like this broke the android build.

Reverting for now.

[Georges Khalil, 2015-10-28 18:47:03]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1413813006/ by georgesak@chromium.org.

The reason for reverting is: Broke android build..

[Sergey Ulanov, 2015-10-28 19:08:46]

Description was changed from

==========
Add TransportSession interface to prepare for WebRTC-based transport.

Previosly JingleSession was responsible to establishing connection
with the peer and managing list of channels. Now the new
IceTransportSession class (that implements TransportSession interface)
is responsible for managing list of channels and so it will be possible
to add support for WebRTC-based transport by adding another
TransportSession implementation.

There are no functional changes in this CL.

BUG=547158

Committed: https://crrev.com/7a5351f4c6e407e456978f6599d475616a69fc0b
Cr-Commit-Position: refs/heads/master@{#356588}
==========

to

==========
Add TransportSession interface to prepare for WebRTC-based transport.

Previosly JingleSession was responsible to establishing connection
with the peer and managing list of channels. Now the new
IceTransportSession class (that implements TransportSession interface)
is responsible for managing list of channels and so it will be possible
to add support for WebRTC-based transport by adding another
TransportSession implementation.

There are no functional changes in this CL.

BUG=547158

Committed: https://crrev.com/7a5351f4c6e407e456978f6599d475616a69fc0b
Cr-Commit-Position: refs/heads/master@{#356588}
==========

[Sergey Ulanov, 2015-10-28 20:07:33]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-10-28 20:07:34]

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

[commit-bot: I haz the power, 2015-10-28 20:08:24]

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

[commit-bot: I haz the power, 2015-10-28 20:48:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-28 20:50:17]

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