Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage.

webrtc/api/quicdatatransport.cc

Index: webrtc/api/quicdatatransport.cc
diff --git a/webrtc/api/quicdatatransport.cc b/webrtc/api/quicdatatransport.cc
index c1caf54067ebbfeb6e084b23e69ca679aa5a7cba..e9ceec1d25f5250bd04a89a90613fb8183e8390a 100644
--- a/webrtc/api/quicdatatransport.cc
+++ b/webrtc/api/quicdatatransport.cc
@@ -10,24 +10,40 @@
 
 #include "webrtc/api/quicdatatransport.h"
 
+#include "webrtc/base/bind.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/p2p/quic/quictransportchannel.h"
 #include "webrtc/p2p/quic/reliablequicstream.h"
 
 namespace webrtc {
 
-QuicDataTransport::QuicDataTransport(rtc::Thread* signaling_thread,
-                                     rtc::Thread* worker_thread,
-                                     rtc::Thread* network_thread)
+QuicDataTransport::QuicDataTransport(
+    rtc::Thread* signaling_thread,
+    rtc::Thread* worker_thread,
+    rtc::Thread* network_thread,
+    cricket::TransportController* transport_controller)
     : signaling_thread_(signaling_thread),
       worker_thread_(worker_thread),
-      network_thread_(network_thread) {
+      network_thread_(network_thread),
+      transport_controller_(transport_controller) {
   RTC_DCHECK(signaling_thread_);
   RTC_DCHECK(worker_thread_);
   RTC_DCHECK(network_thread_);
 }
 
-QuicDataTransport::~QuicDataTransport() {}
+QuicDataTransport::~QuicDataTransport() {
+  network_thread_->Invoke<void>(
+      RTC_FROM_HERE,
+      rtc::Bind(&QuicDataTransport::DestroyTransportChannels_n, this));
+  LOG(LS_INFO) << "Destroyed the QUIC data transport.";
+}
+
+void QuicDataTransport::DestroyTransportChannels_n() {

[Taylor Brandstetter, 2016/07/21 23:39:57]

nit: I think the location of the method in the .cc file should match where it is
in the .h file.

[Zhi Huang, 2016/07/25 23:40:36]

Done.

+  if (quic_transport_channel_) {
+    transport_controller_->DestroyTransportChannel_n(
+        transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP);

[pthatcher1, 2016/07/22 17:57:57]

Can you add a new component constant, perhaps ICE_CANDIDATE_COMPONENT_DEFAULT
(with the same value), so we don't need "RTP" in here?

[Zhi Huang, 2016/07/25 23:40:36]

We already have "const int ICE_CANDIDATE_COMPONENT_DEFAULT = 1;"
in p2pconstants.cc. I will use that one.

+  }
+}
 
 bool QuicDataTransport::SetTransportChannel(
     cricket::QuicTransportChannel* channel) {
@@ -35,16 +51,19 @@ bool QuicDataTransport::SetTransportChannel(
     LOG(LS_ERROR) << "|channel| is NULL. Cannot set transport channel.";
     return false;
   }
-  if (quic_transport_channel_) {
-    if (channel == quic_transport_channel_) {
-      LOG(LS_WARNING) << "Ignoring duplicate transport channel.";
-      return true;
-    }
-    LOG(LS_ERROR) << "|channel| does not match existing transport channel.";
-    return false;
+
+  if (channel == quic_transport_channel_) {
+    LOG(LS_WARNING) << "Ignoring duplicate transport channel.";
+    return true;
   }
 
   LOG(LS_INFO) << "Setting QuicTransportChannel for QuicDataTransport";
+
+  if (quic_transport_channel_) {
+    quic_transport_channel_->SignalIncomingStream.disconnect(this);
+    transport_controller_->DestroyTransportChannel_n(
+        transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP);

[pthatcher1, 2016/07/22 17:57:57]

Can you put this in a common method, perhaps DestroyQuicTransportChannel() and
call it from the two placs?

[Zhi Huang, 2016/07/25 23:40:36]

Since we are not supporting switching the transportchannel in QuicDataChannel
for now, I will disable the support of switching the transport channel for
QuicDataTransport as well.

+  }
   quic_transport_channel_ = channel;
   quic_transport_channel_->SignalIncomingStream.connect(
       this, &QuicDataTransport::OnIncomingStream);
@@ -62,6 +81,32 @@ bool QuicDataTransport::SetTransportChannel(
   return success;
 }
 
+bool QuicDataTransport::SetTransport(const std::string& transport_name) {
+  return network_thread_->Invoke<bool>(
+      RTC_FROM_HERE,
+      rtc::Bind(&QuicDataTransport::SetTransport_n, this, transport_name));
+}
+
+bool QuicDataTransport::SetTransport_n(const std::string& transport_name) {

[Taylor Brandstetter, 2016/07/21 23:39:57]

I think there needs to be a way to un-set the transport (for example, when
PeerConnection::Close is called, or a description is set with "m=application
0"). This can be done by defining a new method or using an empty string to
signify "no transport" in this method.

[pthatcher1, 2016/07/22 10:46:03]

I don't think we need to unset the transport.  Just close the transport
underneath and close the data channels.  Why do you need more?

[Taylor Brandstetter, 2016/07/22 19:04:56]

That's what unsetting the transport would do. Are you suggesting WebRtcSession
does this directly, as opposed to going through QuicDataTransport? I don't like
that. It means the ownership of the transport is fuzzy, and we'd still need a
way to tell QuicDataTransport "your transport is closed, stop using it".

[Zhi Huang, 2016/07/25 23:40:36]

I think what Peter suggested is that we can directly delete the
QuicDataTransport instead of unsetting the transport for it. The transport and
channels will be destroyed in the destructor of QuicDataTransport.

+  RTC_DCHECK(network_thread_->IsCurrent());
+
+  if (transport_name_ == transport_name) {
+    // Nothing to do if transport name isn't changing
+    return true;
+  }
+
+  SetTransportChannel(static_cast<cricket::QuicTransportChannel*>(
+      transport_controller_->CreateTransportChannel_n(
+          transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP)));

[pthatcher1, 2016/07/22 17:57:57]

Can you put this in a CreateQuicTransportChannel method next to the
DestroyQuicTransportChannel method and use it here?

[Zhi Huang, 2016/07/25 23:40:36]

Done.

+
+  if (!quic_transport_channel_) {
+    return false;
+  }
+
+  transport_name_ = transport_name;
+  return true;
+}
+
 rtc::scoped_refptr<DataChannelInterface> QuicDataTransport::CreateDataChannel(
     const std::string& label,
     const DataChannelInit* config) {