Remove remaining sigslots from DataChannelController

This includes:
* SignalDataChannelTransportWritable_s
* SignalDataChannelTransportReceivedData_s
* SignalDataChannelTransportChannelClosing_s
* Removing sigslot::has_slots<> inheritance from SctpDataChannel

Instead, we use the existing sctp_data_channels_ vector of channels
known to the DCC to deliver the callbacks.

Bug: webrtc:11943, webrtc:11547
Change-Id: I7935d7505856eedf04981b8ba665ef8419166c1d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297100
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39557}

pc/data_channel_controller.cc

@@ -41,32 +41,16 @@ bool DataChannelController::SendData(int sid,
 bool DataChannelController::ConnectDataChannel(
     SctpDataChannel* webrtc_data_channel) {
   RTC_DCHECK_RUN_ON(signaling_thread());
-  if (!data_channel_transport()) {
-    // Don't log an error here, because DataChannels are expected to call
-    // ConnectDataChannel in this state. It's the only way to initially tell
-    // whether or not the underlying transport is ready.
-    return false;
-  }
-  SignalDataChannelTransportWritable_s.connect(
-      webrtc_data_channel, &SctpDataChannel::OnTransportReady);
-  SignalDataChannelTransportReceivedData_s.connect(
-      webrtc_data_channel, &SctpDataChannel::OnDataReceived);
-  SignalDataChannelTransportChannelClosing_s.connect(
-      webrtc_data_channel, &SctpDataChannel::OnClosingProcedureStartedRemotely);
-  return true;
+  // TODO(bugs.webrtc.org/11547): This method can be removed once not
+  // needed by `SctpDataChannel`.
+  return data_channel_transport() ? true : false;
 }
 
 void DataChannelController::DisconnectDataChannel(
     SctpDataChannel* webrtc_data_channel) {
   RTC_DCHECK_RUN_ON(signaling_thread());
-  if (!data_channel_transport()) {
-    RTC_LOG(LS_ERROR)
-        << "DisconnectDataChannel called when sctp_transport_ is NULL.";
-    return;
-  }
-  SignalDataChannelTransportWritable_s.disconnect(webrtc_data_channel);
-  SignalDataChannelTransportReceivedData_s.disconnect(webrtc_data_channel);
-  SignalDataChannelTransportChannelClosing_s.disconnect(webrtc_data_channel);
+  // TODO(bugs.webrtc.org/11547): This method can be removed once not
+  // needed by `SctpDataChannel`.
 }
 
 void DataChannelController::AddSctpDataStream(int sid) {
@@ -120,10 +104,11 @@ void DataChannelController::OnDataReceived(
       SafeTask(signaling_safety_.flag(), [this, params, buffer] {
         RTC_DCHECK_RUN_ON(signaling_thread());
         // TODO(bugs.webrtc.org/11547): The data being received should be
-        // delivered on the network thread (change
-        // SignalDataChannelTransportReceivedData_s to
-        // SignalDataChannelTransportReceivedData_n).
-        SignalDataChannelTransportReceivedData_s(params, buffer);
+        // delivered on the network thread.
+        for (const auto& channel : sctp_data_channels_) {
+          if (channel->id() == params.sid)
+            channel->OnDataReceived(params, buffer);
+        }
       }));
 }
 
@@ -132,7 +117,11 @@ void DataChannelController::OnChannelClosing(int channel_id) {
   signaling_thread()->PostTask(
       SafeTask(signaling_safety_.flag(), [this, channel_id] {
         RTC_DCHECK_RUN_ON(signaling_thread());
-        SignalDataChannelTransportChannelClosing_s(channel_id);
+        // TODO(bugs.webrtc.org/11547): Should run on the network thread.
+        for (const auto& channel : sctp_data_channels_) {
+          if (channel->id() == channel_id)
+            channel->OnClosingProcedureStartedRemotely();
+        }
       }));
 }
 
@@ -151,8 +140,6 @@ void DataChannelController::OnChannelClosed(int channel_id) {
           // Note: this causes OnSctpDataChannelClosed() to not do anything
           // when called from within `OnClosingProcedureComplete`.
           sctp_data_channels_.erase(it);
-
-          DisconnectDataChannel(channel.get());
           sid_allocator_.ReleaseSid(channel->sid());
 
           channel->OnClosingProcedureComplete();
@@ -165,7 +152,8 @@ void DataChannelController::OnReadyToSend() {
   signaling_thread()->PostTask(SafeTask(signaling_safety_.flag(), [this] {
     RTC_DCHECK_RUN_ON(signaling_thread());
     data_channel_transport_ready_to_send_ = true;
-    SignalDataChannelTransportWritable_s(data_channel_transport_ready_to_send_);
+    for (const auto& channel : sctp_data_channels_)
+      channel->OnTransportReady(true);
   }));
 }
 

pc/data_channel_controller.h

@@ -26,7 +26,6 @@
 #include "rtc_base/checks.h"
 #include "rtc_base/copy_on_write_buffer.h"
 #include "rtc_base/ssl_stream_adapter.h"
-#include "rtc_base/third_party/sigslot/sigslot.h"
 #include "rtc_base/thread.h"
 #include "rtc_base/thread_annotations.h"
 #include "rtc_base/weak_ptr.h"
@@ -151,19 +150,6 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   std::vector<rtc::scoped_refptr<SctpDataChannel>> sctp_data_channels_
       RTC_GUARDED_BY(signaling_thread());
 
-  // Signals from `data_channel_transport_`.  These are invoked on the
-  // signaling thread.
-  // TODO(bugs.webrtc.org/11547): These '_s' signals likely all belong on the
-  // network thread.
-  sigslot::signal1<bool> SignalDataChannelTransportWritable_s
-      RTC_GUARDED_BY(signaling_thread());
-  sigslot::signal2<const cricket::ReceiveDataParams&,
-                   const rtc::CopyOnWriteBuffer&>
-      SignalDataChannelTransportReceivedData_s
-          RTC_GUARDED_BY(signaling_thread());
-  sigslot::signal1<int> SignalDataChannelTransportChannelClosing_s
-      RTC_GUARDED_BY(signaling_thread());
-
   // Owning PeerConnection.
   PeerConnectionInternal* const pc_;
   // The weak pointers must be dereferenced and invalidated on the signalling

pc/data_channel_unittest.cc

@@ -433,21 +433,6 @@ TEST_F(SctpDataChannelTest, SendDataId) {
   EXPECT_EQ(1, controller_->last_sid());
 }
 
-// Tests that the incoming messages with wrong ids are rejected.
-TEST_F(SctpDataChannelTest, ReceiveDataWithInvalidId) {
-  webrtc_data_channel_->SetSctpSid(StreamId(1));
-  SetChannelReady();
-
-  AddObserver();
-
-  cricket::ReceiveDataParams params;
-  params.sid = 0;
-  DataBuffer buffer("abcd");
-  webrtc_data_channel_->OnDataReceived(params, buffer.data);
-
-  EXPECT_EQ(0U, observer_->messages_received());
-}
-
 // Tests that the incoming messages with right ids are accepted.
 TEST_F(SctpDataChannelTest, ReceiveDataWithValidId) {
   webrtc_data_channel_->SetSctpSid(StreamId(1));

pc/sctp_data_channel.cc

@@ -369,9 +369,9 @@ void SctpDataChannel::SetSctpSid(const StreamId& sid) {
   controller_->AddSctpDataStream(sid.stream_id_int());
 }
 
-void SctpDataChannel::OnClosingProcedureStartedRemotely(int sid) {
+void SctpDataChannel::OnClosingProcedureStartedRemotely() {
   RTC_DCHECK_RUN_ON(signaling_thread_);
-  if (id_.stream_id_int() == sid && state_ != kClosing && state_ != kClosed) {
+  if (state_ != kClosing && state_ != kClosed) {
     // Don't bother sending queued data since the side that initiated the
     // closure wouldn't receive it anyway. See crbug.com/559394 for a lengthy
     // discussion about this.
@@ -429,9 +429,7 @@ DataChannelStats SctpDataChannel::GetStats() const {
 void SctpDataChannel::OnDataReceived(const cricket::ReceiveDataParams& params,
                                      const rtc::CopyOnWriteBuffer& payload) {
   RTC_DCHECK_RUN_ON(signaling_thread_);
-  if (id_.stream_id_int() != params.sid) {
-    return;
-  }
+  RTC_DCHECK_EQ(id_.stream_id_int(), params.sid);
 
   if (params.type == DataMessageType::kControl) {
     if (handshake_state_ != kHandshakeWaitingForAck) {

pc/sctp_data_channel.h

@@ -29,7 +29,6 @@
 #include "rtc_base/containers/flat_set.h"
 #include "rtc_base/copy_on_write_buffer.h"
 #include "rtc_base/ssl_stream_adapter.h"  // For SSLRole
-#include "rtc_base/third_party/sigslot/sigslot.h"
 #include "rtc_base/thread.h"
 #include "rtc_base/thread_annotations.h"
 #include "rtc_base/weak_ptr.h"
@@ -100,7 +99,7 @@ class SctpSidAllocator {
 
 // SctpDataChannel is an implementation of the DataChannelInterface based on
 // SctpTransport. It provides an implementation of unreliable or
-// reliabledata channels.
+// reliable data channels.
 
 // DataChannel states:
 // kConnecting: The channel has been created the transport might not yet be
@@ -122,8 +121,7 @@ class SctpSidAllocator {
 // 5. Bob sends outgoing stream reset.
 // 6. Alice receives incoming reset, Bob receives acknowledgement. Both receive
 //    OnClosingProcedureComplete callback and transition to kClosed.
-class SctpDataChannel : public DataChannelInterface,
-                        public sigslot::has_slots<> {
+class SctpDataChannel : public DataChannelInterface {
  public:
   static rtc::scoped_refptr<SctpDataChannel> Create(
       rtc::WeakPtr<SctpDataChannelControllerInterface> controller,
@@ -188,9 +186,10 @@ class SctpDataChannel : public DataChannelInterface,
   // Sets the SCTP sid and adds to transport layer if not set yet. Should only
   // be called once.
   void SetSctpSid(const StreamId& sid);
+
   // The remote side started the closing procedure by resetting its outgoing
   // stream (our incoming stream). Sets state to kClosing.
-  void OnClosingProcedureStartedRemotely(int sid);
+  void OnClosingProcedureStartedRemotely();
   // The closing procedure is complete; both incoming and outgoing stream
   // resets are done and the channel can transition to kClosed. Called
   // asynchronously after RemoveSctpDataStream.