Refactoring on QUIC

Refactoring on QUIC related classes.

Merge with the latest webrtc native code.
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to QUIC related classes.

Committed: https://crrev.com/f2c2f8f20cdc2e77ca69b36b9daa00c4de220022
Cr-Commit-Position: refs/heads/master@{#13472}

[Zhi Huang, 2016-06-24 17:02:34]

zhihuang@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2016-06-28 00:33:38]

Description was changed from

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has ben modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/
==========

to

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has been modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/
==========

[pthatcher1, 2016-06-28 22:39:03]

lgtm, with nits

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:2091: // audio and video, and a QUIC data
channel.
Weird formatting.

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:556: else if (rtc_configuration.enable_quic &&
dtls_enabled_) {
I don't think we need to check for dtls_enabled_.

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:561: LOG(LS_INFO) << "Using QUIC instead of DTLS";
We could log this if dtls_enabled_.  But otherwise, I don't think we need to.

[Taylor Brandstetter, 2016-06-29 17:50:05]

Mostly just nits about tests names etc., but I have some concerns about the
interaction between QuicDataTransport, QuicTransportChannel, PeerConnection and
WebRtcSession (see below).

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:2120: TEST_F(P2PTestConductor,
CreateOfferWithQuicDataChannel) {
This isn't just testing the offer creation, it's testing the whole negotiation.
So it should be renamed to NegotiateQuicDataChannel or similar.

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:2133: // This test sets up a Jsep call 
using QUIC. The callee only receives video.
Remove extra space after "call", change Jsep to JSEP here and below.

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:2134: TEST_F(P2PTestConductor,
LocalP2PTestAnswerVideoQuic) {
I don't understand the naming "AnswerVideoQuic". Would something like
"LocalP2PTestVideoOnlyWithQuic" make more sense? Same with 2 tests below.

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionendtoend_unittest.cc (right):

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectionendtoend_unittest.cc:380: EXPECT_EQ_WAIT(message_2,
dc_2_observer->last_message(), kMaxWait);
Should we also test data sent in the reverse direction (callee to caller)?

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1842: #endif  // HAVE_QUIC
I think this needs to be rethought. This design means that when using QUIC,
WebRtcSession creates the transport channel instead of BaseChannel. But that
means WebRtcSession should be doing the same stuff that happens when using
BaseChannels, such as:

* If BUNDLE is negotiated, may need to switch which TransportChannel is used for
data
* If a media description is rejected, the TransportChannel should be destroyed.

It may be easier to just use BaseChannel, and signal when its TransportChannel
is changed and when it's created/destroyed. For example, when bundling is
negotiated, the PeerConnection's QuicDataTransport should start using the
QuicTransportChannel negotiated for bundling data.

Or, QuicDataTransport could just be a subclass of BaseChannel. Then there are
much less wires to hook up.

It's probably a good idea to have a whiteboard discussion when Peter is here.

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession_...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession_...
webrtc/api/webrtcsession_unittest.cc:4316: TEST_P(WebRtcSessionTest,
TestUseQuic) {
Maybe "TestNegotiateQuic" would be a better name, since this is mainly testing
that SetLocalDescription/SetRemoteDescription succeeds.

[pthatcher1, 2016-06-29 18:35:29]

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2089553002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1842: #endif  // HAVE_QUIC
On 2016/06/29 17:50:04, Taylor Brandstetter wrote:
> I think this needs to be rethought. This design means that when using QUIC,
> WebRtcSession creates the transport channel instead of BaseChannel. But that
> means WebRtcSession should be doing the same stuff that happens when using
> BaseChannels, such as:
> 
> * If BUNDLE is negotiated, may need to switch which TransportChannel is used
for
> data
> * If a media description is rejected, the TransportChannel should be
destroyed.
> 
> It may be easier to just use BaseChannel, and signal when its TransportChannel
> is changed and when it's created/destroyed. For example, when bundling is
> negotiated, the PeerConnection's QuicDataTransport should start using the
> QuicTransportChannel negotiated for bundling data.
> 
> Or, QuicDataTransport could just be a subclass of BaseChannel. Then there are
> much less wires to hook up.
> 
> It's probably a good idea to have a whiteboard discussion when Peter is here.

Yeah, we can discuss it in person.  This was the design Michael and I came up
with while he was here.  I was trying to avoid the complexity of BaseChannel by
requiring that QUIC only be used with max-bundle, at least in the early version
of support for QUIC.

Similarly, this CL doesn't do any negotiation.  It just assumes you will do QUIC
and won't reject QUIC (kind of a like a max-quic setting would be).  Doing
negotiation can come in a later CL (at which point, we may need to do something
with BaseChannel, but there are actually harder things to whiteboard and figure
out around offering either DTLS or QUIC and then reacting to whichever comes
back either on the signaling or media path).

[Zhi Huang, 2016-07-13 20:08:40]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2016-07-13 20:08:40]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2089553002/#ps60001 (title:
"Split the CL")

[commit-bot: I haz the power, 2016-07-13 20:08:44]

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

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

Description was changed from

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has been modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/
==========

to

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has been modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/
==========

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

Committed patchset #4 (id:60001)

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

CQ bit was unchecked.

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

Description was changed from

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has been modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/
==========

to

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has been modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/

Committed: https://crrev.com/36c8d69ce188102ae6fd48c371cf1518f08698fb
Cr-Commit-Position: refs/heads/master@{#13470}
==========

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

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

[Taylor Brandstetter, 2016-07-13 20:57:11]

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

The reason for reverting is: Reverting because description was inaccurate. Will
reland after updating description..

[Taylor Brandstetter, 2016-07-13 20:57:41]

Description was changed from

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has been modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/

Committed: https://crrev.com/36c8d69ce188102ae6fd48c371cf1518f08698fb
Cr-Commit-Position: refs/heads/master@{#13470}
==========

to

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has been modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/
==========

[Zhi Huang, 2016-07-13 21:09:21]

Description was changed from

==========
Modify PeerConnection for end-to-end QuicDataChannel usage

To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.

PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.

WebRtcSession has been modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.

Modification of previous in-flight CL:
https://codereview.chromium.org/1844803002/
==========

to

==========
Refactoring on QUIC.

Merge with lastest webrtc code. Remove deprecated function Connect() in
QuicTransportChannel. Fix the compiling issue and broken unit tests by adding
the network thread to QUIC related object.
==========

[Zhi Huang, 2016-07-13 21:10:06]

Description was changed from

==========
Refactoring on QUIC.

Merge with lastest webrtc code. Remove deprecated function Connect() in
QuicTransportChannel. Fix the compiling issue and broken unit tests by adding
the network thread to QUIC related object.
==========

to

==========
Refactoring on QUIC related classes.

Merge with lastest webrtc code. Remove deprecated function Connect() in
QuicTransportChannel. Fix the compiling issue and broken unit tests by adding
the network thread to QUIC related classes.
==========

[Zhi Huang, 2016-07-13 21:10:25]

Description was changed from

==========
Refactoring on QUIC related classes.

Merge with lastest webrtc code. Remove deprecated function Connect() in
QuicTransportChannel. Fix the compiling issue and broken unit tests by adding
the network thread to QUIC related classes.
==========

to

==========
Refactoring on QUIC related classes.

Merge with lastest webrtc code. 
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.
==========

[Zhi Huang, 2016-07-13 21:10:42]

Description was changed from

==========
Refactoring on QUIC related classes.

Merge with lastest webrtc code. 
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.
==========

to

==========
Refactoring on QUIC related classes.

Merge with latest webrtc code. 
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.
==========

[Zhi Huang, 2016-07-13 21:11:02]

Description was changed from

==========
Refactoring on QUIC related classes.

Merge with latest webrtc code. 
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.
==========

to

==========
Refactoring on QUIC related classes.

Merge with the latest webrtc native code. 
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.
==========

[Zhi Huang, 2016-07-13 21:12:11]

The CQ bit was checked by zhihuang@webrtc.org

[commit-bot: I haz the power, 2016-07-13 21:12:15]

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

[commit-bot: I haz the power, 2016-07-13 21:13:52]

Description was changed from

==========
Refactoring on QUIC related classes.

Merge with the latest webrtc native code. 
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.
==========

to

==========
Refactoring on QUIC related classes.

Merge with the latest webrtc native code. 
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.
==========

[commit-bot: I haz the power, 2016-07-13 21:13:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-13 21:13:59]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 21:14:00]

Description was changed from

==========
Refactoring on QUIC related classes.

Merge with the latest webrtc native code. 
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.
==========

to

==========
Refactoring on QUIC related classes.

Merge with the latest webrtc native code.
Remove deprecated function Connect() in QuicTransportChannel.
Fix the compiling issue and broken unit tests by adding the network thread to
QUIC related classes.

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

[commit-bot: I haz the power, 2016-07-13 21:14:02]

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