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

webrtc/api/rtcstatscollector.h

Index: webrtc/api/rtcstatscollector.h
diff --git a/webrtc/api/rtcstatscollector.h b/webrtc/api/rtcstatscollector.h
index 2c160ee414025ccfb0e9e936a1b7c88c3e4d5e04..bca258ae8c3051725322592d413dcfe9b7d15c90 100644
--- a/webrtc/api/rtcstatscollector.h
+++ b/webrtc/api/rtcstatscollector.h
@@ -13,14 +13,17 @@
 
 #include <map>
 #include <memory>
+#include <set>
 #include <vector>
 
+#include "webrtc/api/datachannel.h"
 #include "webrtc/api/datachannelinterface.h"
 #include "webrtc/api/stats/rtcstats_objects.h"
 #include "webrtc/api/stats/rtcstatsreport.h"
 #include "webrtc/base/asyncinvoker.h"
 #include "webrtc/base/refcount.h"
 #include "webrtc/base/scoped_ref_ptr.h"
+#include "webrtc/base/sigslot.h"
 #include "webrtc/base/sslidentity.h"
 #include "webrtc/base/timeutils.h"
 
@@ -49,7 +52,8 @@ class RTCStatsCollectorCallback : public virtual rtc::RefCountInterface {
 // Stats are gathered on the signaling, worker and network threads
 // asynchronously. The callback is invoked on the signaling thread. Resulting
 // reports are cached for |cache_lifetime_| ms.
-class RTCStatsCollector : public virtual rtc::RefCountInterface {
+class RTCStatsCollector : public virtual rtc::RefCountInterface,
+                          public sigslot::has_slots<> {
  public:
   static rtc::scoped_refptr<RTCStatsCollector> Create(
       PeerConnection* pc,
@@ -64,6 +68,10 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface {
   // calling |GetStatsReport| guarantees fresh stats.
   void ClearCachedStatsReport();
 
+  // Exposed for testing only.
+  void OnDataChannelOpenedForTesting(DataChannel* channel);
+  void OnDataChannelClosedForTesting(DataChannel* channel);
+
  protected:
   RTCStatsCollector(PeerConnection* pc, int64_t cache_lifetime_us);
 
@@ -78,6 +86,9 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface {
       const rtc::scoped_refptr<RTCStatsReport>& partial_report);
 
  private:
+  // To allow |pc_| to wire up RTCStatsCollector::OnBlahBlah signal slots.
+  friend class PeerConnection;
+
   struct CertificateStatsPair {
     std::unique_ptr<rtc::SSLCertificateStats> local;
     std::unique_ptr<rtc::SSLCertificateStats> remote;
@@ -115,6 +126,10 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface {
   std::map<std::string, CertificateStatsPair>
   PrepareTransportCertificateStats_s(const SessionStats& session_stats) const;
 
+  // Slots for signals (sigslot).
+  void OnDataChannelOpened(DataChannel* channel);
+  void OnDataChannelClosed(DataChannel* channel);
+
   PeerConnection* const pc_;
   rtc::Thread* const signaling_thread_;
   rtc::Thread* const worker_thread_;
@@ -133,6 +148,16 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface {
   int64_t cache_timestamp_us_;
   int64_t cache_lifetime_us_;
   rtc::scoped_refptr<const RTCStatsReport> cached_report_;
+
+  // The opened count goes up when a channel is fully opened and the closed
+  // count goes up if a previously opened channel has fully closed. The opened
+  // count does not go down when a channel closes, meaning (opened - closed) is
+  // the number of channels currently opened. A channel that is closed before
+  // reaching the open state does not affect these counters.
+  uint32_t data_channels_opened_;
+  uint32_t data_channels_closed_;

[hta-webrtc, 2016/11/03 13:49:58]

This is actually the beginning of a new path in stats collecting, where you
allow the StatsCollector object to hold references to data between calls and
depend on them being updated correctly from the start of time to the end of
time.

You might want to flag that with a comment above; somewhere in the future, we
might consider factoring out all that kind of stuff into a (private?) class that
is separate from the statscollector itself.

But for these 2 counters, just making a note should be OK.

[hbos, 2016/11/03 15:02:02]

Created struct InternalRecord and added a comment.

+  // Channels that have been opened, they remain in the set until fully closed.
+  std::set<DataChannel*> opened_data_channels_;

[hta-webrtc, 2016/11/03 13:49:58]

Do we actually need this? I'd suggest not retaining pointers if we don't have a
clear use case for them - they're a temptation to sin.
If they're used as identifiers (and I think they are), make a clear comment that
that's what they are - they must NOT be followed.

It's tempting to cast them to int in order to limit the temptation.

[hbos, 2016/11/03 15:02:02]

Changed to std::set<uintptr_t>

 };
 
 const char* CandidateTypeToRTCIceCandidateTypeForTesting(