Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

webrtc/api/peerconnection.cc

Index: webrtc/api/peerconnection.cc
diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc
index 830c301a6d9cf0090870e3a06bca4ce23835ec0c..f865cbe79df6e34d061fb8906822e4b00f728930 100644
--- a/webrtc/api/peerconnection.cc
+++ b/webrtc/api/peerconnection.cc
@@ -2079,6 +2079,10 @@ rtc::scoped_refptr<DataChannel> PeerConnection::InternalCreateDataChannel(
     sid_allocator_.ReleaseSid(new_config.id);
     return nullptr;
   }
+  channel->SignalOpened.connect(stats_collector_.get(),
+                                &RTCStatsCollector::OnDataChannelOpened);

[Taylor Brandstetter, 2016/11/09 00:32:48]

This is confusing to me since the slot is connected in a different file than
where it's defined, and it requires a friend relationship. I think it begins a
worrisome coupling between RTCStatsCollector and PeerConnection.

It would be better if:

- PeerConnection just kept track of the history itself and provided accessors.

- PeerConnection called a method on RTCStatsCollector called
"MonitorDataChannel" (or equivalent) which connected the signals.

- A step further: PeerConnection fires a "DataChannelCreated" signal, -which
RTCStatsCollector connects to. This in turn causes it to start monitoring the
new data channel.

I'd be happy with any of these, but I'd prefer to not have the friend
relationship and "ForTesting" methods. Since this is a pattern we'll likely need
for other things, it would be nice to start with a good precedent.

[hbos, 2016/11/11 09:52:02]

Done.

I added a SignalDataChannelCreated to PeerConnection and wired it up.
I wanted to test the signal in peerconnection_unittest.cc, but I don't get how
you're supposed to test PeerConnection-specific things, the tests are handling
scoped_refptr<PeerConnectionInterface> and I don't want to introduce casting to
PeerConnection. (Why, there's already a peerconnectioninterface_unittest.cc?) I
also don't want to update the PeerConnectionInterface willy-nilly.

So right now only mocks are tested, ensuring RTCStatsCollector is working as
intended. I will soon start work on an integration test for getStats, testing
without mocks.

[Taylor Brandstetter, 2016/11/11 20:54:53]

RTCStatsCollector already uses sctp_data_channels(), which is
PeerConnection-specific, so I don't think it's a new problem. We do need to
improve our PeerConnection test infrastructure so we can add these sorts of
tests more easily, but I think that's outside the scope of this CL.

+  channel->SignalOpened.connect(stats_collector_.get(),
+                                &RTCStatsCollector::OnDataChannelClosed);
 
   if (channel->data_channel_type() == cricket::DCT_RTP) {
     if (rtp_data_channels_.find(channel->label()) != rtp_data_channels_.end()) {