Allow TransportController to create a QuicTransportChannel

Allow TransportController to create a QuicTransportChannel

A QuicTransport is implemented that subclasses Transport
and takes ownership of the QuicTransportChannel/P2PTransportChannel.

Split from CL https://codereview.webrtc.org/1844803002/.

BUG=

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

[mikescarlett, 2016-04-04 23:31:56]

Description was changed from

==========
Allow TransportController to create a QuicTransportChannel

BUG=
==========

to

==========
Allow TransportController to create a QuicTransportChannel

A QuicTransport is implemented that subclasses Transport
and takes ownership of the QuicTransportChannel/P2PTransportChannel.

Split from CL https://codereview.webrtc.org/1844803002/.

BUG=
==========

[mikescarlett, 2016-04-04 23:31:56]

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

[Taylor Brandstetter, 2016-04-05 22:13:49]

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransport.cc:138: return
Transport::NegotiateTransportDescription(local_role, error_desc);
There's a lot of duplicated code here. Would it maybe make sense to have
DtlsTransport and QuicTransport inherit from a common base class, like
"SecureTransport"? Or alternatively, define an SSL negotiation helper class
that's used by both classes (in order to use composition instead of
inheritance).

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransport.h (right):

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransport.h:18: #include
"webrtc/p2p/base/p2ptransportchannel.h"
You should be able to forward declare P2PTransportChannel and only include this
in the .cc file.

[mikescarlett, 2016-04-06 18:43:44]

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransport.cc:138: return
Transport::NegotiateTransportDescription(local_role, error_desc);
On 2016/04/05 22:13:49, Taylor Brandstetter wrote:
> There's a lot of duplicated code here. Would it maybe make sense to have
> DtlsTransport and QuicTransport inherit from a common base class, like
> "SecureTransport"? Or alternatively, define an SSL negotiation helper class
> that's used by both classes (in order to use composition instead of
> inheritance).

To avoid code duplication, another solution is that lines 77-135 and the
equivalent code from DtlsTransport could be replaced with a utility function
like:

bool NegotiateSSLRole(ContentAction local_role, const TransportDescription&
local_description, const TransportDescription& remote_description, SSLRole*
ssl_role, std::string* error_desc);

I think moving this code out of the Transport class is a good solution - better
than inheritance if our goal is to eliminate dependence on it. A helper class or
interface makes sense if SSL negotiation depends on a state external to the
negotiated session description, but as far as I can tell a utility function
would be effective.

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransport.h (right):

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransport.h:18: #include
"webrtc/p2p/base/p2ptransportchannel.h"
On 2016/04/05 22:13:49, Taylor Brandstetter wrote:
> You should be able to forward declare P2PTransportChannel and only include
this
> in the .cc file.

Done. Also I realize that transport.h should be included instead.

[Taylor Brandstetter, 2016-04-06 19:27:41]

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransport.cc:138: return
Transport::NegotiateTransportDescription(local_role, error_desc);
On 2016/04/06 18:43:44, mikescarlett wrote:
> On 2016/04/05 22:13:49, Taylor Brandstetter wrote:
> > There's a lot of duplicated code here. Would it maybe make sense to have
> > DtlsTransport and QuicTransport inherit from a common base class, like
> > "SecureTransport"? Or alternatively, define an SSL negotiation helper class
> > that's used by both classes (in order to use composition instead of
> > inheritance).
> 
> To avoid code duplication, another solution is that lines 77-135 and the
> equivalent code from DtlsTransport could be replaced with a utility function
> like:
> 
> bool NegotiateSSLRole(ContentAction local_role, const TransportDescription&
> local_description, const TransportDescription& remote_description, SSLRole*
> ssl_role, std::string* error_desc);
> 
> I think moving this code out of the Transport class is a good solution -
better
> than inheritance if our goal is to eliminate dependence on it. A helper class
or
> interface makes sense if SSL negotiation depends on a state external to the
> negotiated session description, but as far as I can tell a utility function
> would be effective.

That sounds like a good idea. Though the advantage of using inheritance or a
helper class is that several more of these methods wouldn't need to be
duplicated (ApplyLocalTD, ApplyNegotiatedTD, etc.). However, if you think
QuicTransport's implementation of these methods may eventually diverge from 
DtlsTransport's, it makes sense to duplicate them for now; it's up to you

[mikescarlett, 2016-04-06 21:18:14]

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransport.cc:138: return
Transport::NegotiateTransportDescription(local_role, error_desc);
On 2016/04/06 19:27:41, Taylor Brandstetter wrote:
> On 2016/04/06 18:43:44, mikescarlett wrote:
> > On 2016/04/05 22:13:49, Taylor Brandstetter wrote:
> > > There's a lot of duplicated code here. Would it maybe make sense to have
> > > DtlsTransport and QuicTransport inherit from a common base class, like
> > > "SecureTransport"? Or alternatively, define an SSL negotiation helper
class
> > > that's used by both classes (in order to use composition instead of
> > > inheritance).
> > 
> > To avoid code duplication, another solution is that lines 77-135 and the
> > equivalent code from DtlsTransport could be replaced with a utility function
> > like:
> > 
> > bool NegotiateSSLRole(ContentAction local_role, const TransportDescription&
> > local_description, const TransportDescription& remote_description, SSLRole*
> > ssl_role, std::string* error_desc);
> > 
> > I think moving this code out of the Transport class is a good solution -
> better
> > than inheritance if our goal is to eliminate dependence on it. A helper
class
> or
> > interface makes sense if SSL negotiation depends on a state external to the
> > negotiated session description, but as far as I can tell a utility function
> > would be effective.
> 
> That sounds like a good idea. Though the advantage of using inheritance or a
> helper class is that several more of these methods wouldn't need to be
> duplicated (ApplyLocalTD, ApplyNegotiatedTD, etc.). However, if you think
> QuicTransport's implementation of these methods may eventually diverge from 
> DtlsTransport's, it makes sense to duplicate them for now; it's up to you

Good point. My main concern with inheritance is that the class hierarchy would
become:

Transport -> P2PTransport -> SecureTransport -> DtlsTransport
                                             -> QuicTransport

instead of:

Transport -> P2PTransport -> DtlsTransport
          -> QuicTransport

even though the main purpose of subclassing Transport appears to be
creating/destroying transport channels and performing negotiation logic not done
by the Transport base class.

Ideally DtlsTransport would have a TransportChannelFactory as an input
parameter, which creates the ICE and crypto channels, then DtlsTransport would
be renamed to SecureTransport to make the hierarchy:

Transport -> SecureTransport

[Taylor Brandstetter, 2016-04-06 21:56:45]

On 2016/04/06 21:18:14, mikescarlett wrote:
>
https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
> File webrtc/p2p/quic/quictransport.cc (right):
> 
>
https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
> webrtc/p2p/quic/quictransport.cc:138: return
> Transport::NegotiateTransportDescription(local_role, error_desc);
> On 2016/04/06 19:27:41, Taylor Brandstetter wrote:
> > On 2016/04/06 18:43:44, mikescarlett wrote:
> > > On 2016/04/05 22:13:49, Taylor Brandstetter wrote:
> > > > There's a lot of duplicated code here. Would it maybe make sense to have
> > > > DtlsTransport and QuicTransport inherit from a common base class, like
> > > > "SecureTransport"? Or alternatively, define an SSL negotiation helper
> class
> > > > that's used by both classes (in order to use composition instead of
> > > > inheritance).
> > > 
> > > To avoid code duplication, another solution is that lines 77-135 and the
> > > equivalent code from DtlsTransport could be replaced with a utility
function
> > > like:
> > > 
> > > bool NegotiateSSLRole(ContentAction local_role, const
TransportDescription&
> > > local_description, const TransportDescription& remote_description,
SSLRole*
> > > ssl_role, std::string* error_desc);
> > > 
> > > I think moving this code out of the Transport class is a good solution -
> > better
> > > than inheritance if our goal is to eliminate dependence on it. A helper
> class
> > or
> > > interface makes sense if SSL negotiation depends on a state external to
the
> > > negotiated session description, but as far as I can tell a utility
function
> > > would be effective.
> > 
> > That sounds like a good idea. Though the advantage of using inheritance or a
> > helper class is that several more of these methods wouldn't need to be
> > duplicated (ApplyLocalTD, ApplyNegotiatedTD, etc.). However, if you think
> > QuicTransport's implementation of these methods may eventually diverge from 
> > DtlsTransport's, it makes sense to duplicate them for now; it's up to you
> 
> Good point. My main concern with inheritance is that the class hierarchy would
> become:
> 
> Transport -> P2PTransport -> SecureTransport -> DtlsTransport
>                                              -> QuicTransport
> 
> instead of:
> 
> Transport -> P2PTransport -> DtlsTransport
>           -> QuicTransport
> 
> even though the main purpose of subclassing Transport appears to be
> creating/destroying transport channels and performing negotiation logic not
done
> by the Transport base class.
> 
> Ideally DtlsTransport would have a TransportChannelFactory as an input
> parameter, which creates the ICE and crypto channels, then DtlsTransport would
> be renamed to SecureTransport to make the hierarchy:
> 
> Transport -> SecureTransport

I had that idea too. But then, what will happen once Transport and
TransportChannel are merged? There would no longer be a Transport that uses a
factory to create a TransportChannel; the TransportChannel would be created
directly. Then the choice again becomes "use inheritance (inherit from
SecureTransportChannel) or use composition (use an SSL negotiation helper
class)."

Maybe Peter would like to weigh in. I lean slightly towards making a helper
class but I could be persuaded.

[mikescarlett, 2016-04-12 00:06:38]

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/60001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransport.cc:138: return
Transport::NegotiateTransportDescription(local_role, error_desc);
On 2016/04/06 21:18:14, mikescarlett wrote:
> On 2016/04/06 19:27:41, Taylor Brandstetter wrote:
> > On 2016/04/06 18:43:44, mikescarlett wrote:
> > > On 2016/04/05 22:13:49, Taylor Brandstetter wrote:
> > > > There's a lot of duplicated code here. Would it maybe make sense to have
> > > > DtlsTransport and QuicTransport inherit from a common base class, like
> > > > "SecureTransport"? Or alternatively, define an SSL negotiation helper
> class
> > > > that's used by both classes (in order to use composition instead of
> > > > inheritance).
> > > 
> > > To avoid code duplication, another solution is that lines 77-135 and the
> > > equivalent code from DtlsTransport could be replaced with a utility
function
> > > like:
> > > 
> > > bool NegotiateSSLRole(ContentAction local_role, const
TransportDescription&
> > > local_description, const TransportDescription& remote_description,
SSLRole*
> > > ssl_role, std::string* error_desc);
> > > 
> > > I think moving this code out of the Transport class is a good solution -
> > better
> > > than inheritance if our goal is to eliminate dependence on it. A helper
> class
> > or
> > > interface makes sense if SSL negotiation depends on a state external to
the
> > > negotiated session description, but as far as I can tell a utility
function
> > > would be effective.
> > 
> > That sounds like a good idea. Though the advantage of using inheritance or a
> > helper class is that several more of these methods wouldn't need to be
> > duplicated (ApplyLocalTD, ApplyNegotiatedTD, etc.). However, if you think
> > QuicTransport's implementation of these methods may eventually diverge from 
> > DtlsTransport's, it makes sense to duplicate them for now; it's up to you
> 
> Good point. My main concern with inheritance is that the class hierarchy would
> become:
> 
> Transport -> P2PTransport -> SecureTransport -> DtlsTransport
>                                              -> QuicTransport
> 
> instead of:
> 
> Transport -> P2PTransport -> DtlsTransport
>           -> QuicTransport
> 
> even though the main purpose of subclassing Transport appears to be
> creating/destroying transport channels and performing negotiation logic not
done
> by the Transport base class.
> 
> Ideally DtlsTransport would have a TransportChannelFactory as an input
> parameter, which creates the ICE and crypto channels, then DtlsTransport would
> be renamed to SecureTransport to make the hierarchy:
> 
> Transport -> SecureTransport

Peter and I discussed this offline. He says that duplicating the code here is
okay as a temporary measure, since the negotiation logic should eventually be
moved to PeerConnection out of Transport.

Since moving the negotiation logic right now would make landing this CL complex,
my plan is to keep QuicTransport as a direct subclass of Transport with the
duplicated code.

[pthatcher1, 2016-04-12 00:13:12]

After having spoken to Taylor about this, I think we could avoid most of the
code duplication without any major refactorings or ugliness just by making two
helper functions, which I have outlined below.

Where should those go?   I'm not sure.  It would be a little hacky, but maybe in
transport.h/transport.cc.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:96: #endif  // HAVE_QUIC
These don't need to be behind HAVE_QUIC.  Here and below.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:23: secure_role_(rtc::SSL_CLIENT) {}
You can do this in the .h:

secure_role_ = rtc::SSL_CLIENT;

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:36: return false;
{}s please

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:68: }
A helper method of VerfiyCertificateFingerprint(certificate_, local_fp) would be
nice here.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:137: secure_role_ = is_remote_server ?
rtc::SSL_CLIENT : rtc::SSL_SERVER;
A helper method NegotiateRole(local_description(), remote_description()) would
be good here.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:153: RTC_DCHECK(kv !=
ice_channel_by_quic_channel_.end());
Why do we have to store this?  Why not just get the ice transport channel from
the quic transport channel (perhaps via
QuicTransportChannel::transport_channel())?

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransport.h (right):

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.h:59: ice_channel_by_quic_channel_;
Why is this needed?

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.h:60: rtc::scoped_refptr<rtc::RTCCertificate>
certificate_;
local_certificate_ might be a little more clear.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.h:61: rtc::SSLRole secure_role_;
And local_role_

[mikescarlett, 2016-04-12 19:28:18]

I added helper functions to the Transport class:

bool VerifyCertificateFingerprint(
       const rtc::RTCCertificate* certificate,
       const rtc::SSLFingerprint* local_fingerprint,
       std::string* error_desc) const;

and

bool NegotiateRole(
       ContentAction local_role,
       rtc::SSLRole* ssl_role,
       rtc::scoped_ptr<rtc::SSLFingerprint>* remote_fingerprint,
       std::string* error_desc) const;

and essentially reformatted the code from DtlsTransport.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:96: #endif  // HAVE_QUIC
On 2016/04/12 00:13:12, pthatcher1 wrote:
> These don't need to be behind HAVE_QUIC.  Here and below.

Done.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:23: secure_role_(rtc::SSL_CLIENT) {}
On 2016/04/12 00:13:12, pthatcher1 wrote:
> You can do this in the .h:
> 
> secure_role_ = rtc::SSL_CLIENT;

Done.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:36: return false;
On 2016/04/12 00:13:12, pthatcher1 wrote:
> {}s please

Done.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:68: }
On 2016/04/12 00:13:12, pthatcher1 wrote:
> A helper method of VerfiyCertificateFingerprint(certificate_, local_fp) would
be
> nice here.

Done.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:137: secure_role_ = is_remote_server ?
rtc::SSL_CLIENT : rtc::SSL_SERVER;
On 2016/04/12 00:13:12, pthatcher1 wrote:
> A helper method NegotiateRole(local_description(), remote_description()) would
> be good here.

I modified the method signature since local_description() and
remote_description() are functions of the Transport class so it doesn't seem
necessary to pass those in as parameters, and since the ConentAction is also
needed.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:153: RTC_DCHECK(kv !=
ice_channel_by_quic_channel_.end());
On 2016/04/12 00:13:12, pthatcher1 wrote:
> Why do we have to store this?  Why not just get the ice transport channel from
> the quic transport channel (perhaps via
> QuicTransportChannel::transport_channel())?

That's fine. I implemented that but named the method channel() to be consistent
with DtlsTransportChannelWrapper.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransport.h (right):

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.h:59: ice_channel_by_quic_channel_;
On 2016/04/12 00:13:12, pthatcher1 wrote:
> Why is this needed?

Removed. Not needed for DestroyTransportChannel() since QuicTransportChannel has
a getter for |channel_|.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.h:60: rtc::scoped_refptr<rtc::RTCCertificate>
certificate_;
On 2016/04/12 00:13:12, pthatcher1 wrote:
> local_certificate_ might be a little more clear.

Done.

https://codereview.webrtc.org/1856943002/diff/100001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.h:61: rtc::SSLRole secure_role_;
On 2016/04/12 00:13:12, pthatcher1 wrote:
> And local_role_

Done.

[mikescarlett, 2016-04-12 19:43:11]

On second though I removed the fingerprint stuff from NegotiateRole and put them
into QuicTransport/DtlsTransport. Should VerifyCertificateFingerprint and
NegotiateRole be member functions of Transport?

[Taylor Brandstetter, 2016-04-12 22:33:11]

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransport.h (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:89:
Base::local_description()->identity_fingerprint.get();
What if local_description() is null here? If it's impossible, then the check in
Base::NegotiateRole should be removed, and maybe an RTC_CHECK should be put in
this method.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:97: } else {
What if "remote_fp && !local_fp"? Before, that wouldn't set the remote
fingerprint. Maybe that doesn't matter, but if it does, it may be easier to just
keep the same if/else if/else structure as before.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:58:
remote_description()->identity_fingerprint.get();
This should be local_description(), right? And again, it seems odd to
dereference local_description() when NegotiateRole hasn't yet checked if it's
set.

[pthatcher1, 2016-04-12 23:26:59]

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransport.h (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:89:
Base::local_description()->identity_fingerprint.get();
On 2016/04/12 22:33:11, Taylor Brandstetter wrote:
> What if local_description() is null here? If it's impossible, then the check
in
> Base::NegotiateRole should be removed, and maybe an RTC_CHECK should be put in
> this method.

I think we should keep the check that both local and remote description are
non-null, like it was before.  Whether we keep that in NegotiateRole or not I'm
not sure.  Perhaps pass the (guaranteed to be non-null) descriptions into
NegotiateRole instead of having those read from the member variables again.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:97: } else {
On 2016/04/12 22:33:11, Taylor Brandstetter wrote:
> What if "remote_fp && !local_fp"? Before, that wouldn't set the remote
> fingerprint. Maybe that doesn't matter, but if it does, it may be easier to
just
> keep the same if/else if/else structure as before.

Yeah, I agree that keeping the old "if (remote_fp && local_fp)" would be better.
 Than we're just factoring out the big case where both are.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:408: return BadTransportDescription("No Local
fingerprint.", error_desc);
Here to, perhaps just stay "no fingerprint" and "fingerprint provided, but no
..." and "fingerprint does not match identity".

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:319: const rtc::SSLFingerprint* local_fingerprint,
This verifies any certificate and fingerprint, right?  So you could change it to
"fingerprint" instead of "local_fingerprint".

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:322: // Performs the offer/answer exchange specified
by RFC 4145, section-4.1 to
It doesn't perform the offer and answer exchange.  It uses the offer and answer
as inputs to negotiate the role.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:58:
remote_description()->identity_fingerprint.get();
On 2016/04/12 22:33:11, Taylor Brandstetter wrote:
> This should be local_description(), right? And again, it seems odd to
> dereference local_description() when NegotiateRole hasn't yet checked if it's
> set.

Yeah, similar to what I said earlier.  We should do the check here and maybe
pass it into NegotiateRole.

[mikescarlett, 2016-04-13 00:58:24]

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransport.h (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:89:
Base::local_description()->identity_fingerprint.get();
On 2016/04/12 22:33:11, Taylor Brandstetter wrote:
> What if local_description() is null here? If it's impossible, then the check
in
> Base::NegotiateRole should be removed, and maybe an RTC_CHECK should be put in
> this method.

I added the check back. That shouldn't have been deleted.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:97: } else {
On 2016/04/12 23:26:59, pthatcher1 wrote:
> On 2016/04/12 22:33:11, Taylor Brandstetter wrote:
> > What if "remote_fp && !local_fp"? Before, that wouldn't set the remote
> > fingerprint. Maybe that doesn't matter, but if it does, it may be easier to
> just
> > keep the same if/else if/else structure as before.
> 
> Yeah, I agree that keeping the old "if (remote_fp && local_fp)" would be
better.
>  Than we're just factoring out the big case where both are.

Changed back to if (remote_fp && local_fp).

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:408: return BadTransportDescription("No Local
fingerprint.", error_desc);
On 2016/04/12 23:26:59, pthatcher1 wrote:
> Here to, perhaps just stay "no fingerprint" and "fingerprint provided, but no
> ..." and "fingerprint does not match identity".

Done.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:319: const rtc::SSLFingerprint* local_fingerprint,
On 2016/04/12 23:26:59, pthatcher1 wrote:
> This verifies any certificate and fingerprint, right?  So you could change it
to
> "fingerprint" instead of "local_fingerprint".

Done.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:322: // Performs the offer/answer exchange specified
by RFC 4145, section-4.1 to
On 2016/04/12 23:26:59, pthatcher1 wrote:
> It doesn't perform the offer and answer exchange.  It uses the offer and
answer
> as inputs to negotiate the role.

Okay I changed the comment.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransport.cc (right):

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:58:
remote_description()->identity_fingerprint.get();
On 2016/04/12 23:26:59, pthatcher1 wrote:
> On 2016/04/12 22:33:11, Taylor Brandstetter wrote:
> > This should be local_description(), right? And again, it seems odd to
> > dereference local_description() when NegotiateRole hasn't yet checked if
it's
> > set.
> 
> Yeah, similar to what I said earlier.  We should do the check here and maybe
> pass it into NegotiateRole.

Done.

https://codereview.webrtc.org/1856943002/diff/230002/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransport.cc:58:
remote_description()->identity_fingerprint.get();
On 2016/04/12 22:33:11, Taylor Brandstetter wrote:
> This should be local_description(), right? And again, it seems odd to
> dereference local_description() when NegotiateRole hasn't yet checked if it's
> set.

I added a check similar to DtlsTransport.

[mikescarlett, 2016-04-15 18:34:56]

mikescarlett@webrtc.org changed reviewers:
+ honghaiz@webrtc.org

[mikescarlett, 2016-04-18 17:25:40]

Patchset #1 (id:1) has been deleted

[mikescarlett, 2016-04-18 17:25:51]

Patchset #7 (id:140001) has been deleted

[mikescarlett, 2016-04-18 17:26:08]

Patchset #8 (id:180001) has been deleted

[mikescarlett, 2016-04-18 17:26:18]

Patchset #8 (id:200001) has been deleted

[mikescarlett, 2016-04-18 17:26:31]

Patchset #12 (id:280001) has been deleted

[mikescarlett, 2016-04-18 17:26:40]

Patchset #12 (id:300001) has been deleted

[mikescarlett, 2016-04-18 17:26:46]

Patchset #9 (id:230001) has been deleted

[mikescarlett, 2016-04-18 17:26:53]

Patchset #8 (id:210001) has been deleted

[mikescarlett, 2016-04-18 17:27:06]

Patchset #1 (id:20001) has been deleted

[mikescarlett, 2016-04-18 17:27:15]

Patchset #1 (id:40001) has been deleted

[mikescarlett, 2016-04-18 22:59:23]

Patchset #8 (id:320001) has been deleted

[mikescarlett, 2016-04-18 23:08:08]

Patchset #8 (id:340001) has been deleted

[pthatcher1, 2016-04-29 20:07:18]

lgtm

[honghaiz3, 2016-04-29 22:26:53]

https://codereview.webrtc.org/1856943002/diff/360001/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransportchannel.h (right):

https://codereview.webrtc.org/1856943002/diff/360001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransportchannel.h:216: TransportChannelImpl* channel()
const { return channel_; }
Seems that this method is only used to delete the p2ptransportchannel.
Can we just make the quictransport channel owns the p2ptransportchannel so that
when the quictransportchannel is deleted, it delete the p2ptransportchannel?
That way, we don't need this method.

[mikescarlett, 2016-04-29 23:17:01]

https://codereview.webrtc.org/1856943002/diff/360001/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransportchannel.h (right):

https://codereview.webrtc.org/1856943002/diff/360001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransportchannel.h:216: TransportChannelImpl* channel()
const { return channel_; }
On 2016/04/29 22:26:52, honghaiz3 wrote:
> Seems that this method is only used to delete the p2ptransportchannel.
> Can we just make the quictransport channel owns the p2ptransportchannel so
that
> when the quictransportchannel is deleted, it delete the p2ptransportchannel?
> That way, we don't need this method. 

I made the QuicTransportChannel own the P2PTransportChannel.

[honghaiz3, 2016-04-29 23:23:16]

lgtm

[mikescarlett, 2016-04-30 02:03:03]

The CQ bit was checked by mikescarlett@webrtc.org

[mikescarlett, 2016-04-30 02:03:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, honghaiz@webrtc.org
Link to the patchset: https://codereview.chromium.org/1856943002/#ps400001
(title: "Sync to upstream")

[commit-bot: I haz the power, 2016-04-30 02:03:12]

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

[commit-bot: I haz the power, 2016-04-30 02:11:03]

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

[commit-bot: I haz the power, 2016-04-30 02:11:04]

Try jobs failed on following builders:
  linux_msan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/9721)
  linux_ubsan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/1685)

[mikescarlett, 2016-04-30 02:34:50]

The CQ bit was checked by mikescarlett@webrtc.org

[commit-bot: I haz the power, 2016-04-30 02:34:55]

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

[commit-bot: I haz the power, 2016-04-30 03:20:59]

Description was changed from

==========
Allow TransportController to create a QuicTransportChannel

A QuicTransport is implemented that subclasses Transport
and takes ownership of the QuicTransportChannel/P2PTransportChannel.

Split from CL https://codereview.webrtc.org/1844803002/.

BUG=
==========

to

==========
Allow TransportController to create a QuicTransportChannel

A QuicTransport is implemented that subclasses Transport
and takes ownership of the QuicTransportChannel/P2PTransportChannel.

Split from CL https://codereview.webrtc.org/1844803002/.

BUG=
==========

[commit-bot: I haz the power, 2016-04-30 03:21:00]

Committed patchset #10 (id:400001)

[commit-bot: I haz the power, 2016-05-01 22:02:20]

Description was changed from

==========
Allow TransportController to create a QuicTransportChannel

A QuicTransport is implemented that subclasses Transport
and takes ownership of the QuicTransportChannel/P2PTransportChannel.

Split from CL https://codereview.webrtc.org/1844803002/.

BUG=
==========

to

==========
Allow TransportController to create a QuicTransportChannel

A QuicTransport is implemented that subclasses Transport
and takes ownership of the QuicTransportChannel/P2PTransportChannel.

Split from CL https://codereview.webrtc.org/1844803002/.

BUG=

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

[commit-bot: I haz the power, 2016-05-01 22:02:21]

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