A step towards changing StatsReport::Value::name to an enum.

The stats reporting code does a lot of unnecessary string copying.
This is a step in the direction of removing that and forcing use of only known constants.

This is a reland of an already reviewed cl that got reverted by mistake.

TBR=xians@google.com

Review URL: https://webrtc-codereview.appspot.com/12989004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6678 4adac7df-926f-26a2-2b94-8c16560cd09d

talk/app/webrtc/objc/RTCStatsReport.mm

@@ -57,7 +57,7 @@
         [NSMutableArray arrayWithCapacity:statsReport.values.size()];
     webrtc::StatsReport::Values::const_iterator it = statsReport.values.begin();
     for (; it != statsReport.values.end(); ++it) {
-      RTCPair* pair = [[RTCPair alloc] initWithKey:@(it->name.c_str())
+      RTCPair* pair = [[RTCPair alloc] initWithKey:@(it->display_name())
                                              value:@(it->value.c_str())];
       [values addObject:pair];
     }

talk/app/webrtc/statscollector.cc

@@ -193,19 +193,17 @@ const char StatsReport::kStatsReportTypeCertificate[] = "googCertificate";
 const char StatsReport::kStatsReportVideoBweId[] = "bweforvideo";
 
 // Implementations of functions in statstypes.h
-void StatsReport::AddValue(const std::string& name, const std::string& value) {
+void StatsReport::AddValue(StatsReport::StatsValueName name,
-  Value temp;
+                           const std::string& value) {
-  temp.name = name;
+  values.push_back(Value(name, value));
-  temp.value = value;
-  values.push_back(temp);
 }
 
-void StatsReport::AddValue(const std::string& name, int64 value) {
+void StatsReport::AddValue(StatsReport::StatsValueName name, int64 value) {
   AddValue(name, talk_base::ToString<int64>(value));
 }
 
 template <typename T>
-void StatsReport::AddValue(const std::string& name,
+void StatsReport::AddValue(StatsReport::StatsValueName name,
                            const std::vector<T>& value) {
   std::ostringstream oss;
   oss << "[";
@@ -218,11 +216,11 @@ void StatsReport::AddValue(const std::string& name,
   AddValue(name, oss.str());
 }
 
-void StatsReport::AddBoolean(const std::string& name, bool value) {
+void StatsReport::AddBoolean(StatsReport::StatsValueName name, bool value) {
   AddValue(name, value ? "true" : "false");
 }
 
-void StatsReport::ReplaceValue(const std::string& name,
+void StatsReport::ReplaceValue(StatsReport::StatsValueName name,
                                const std::string& value) {
   for (Values::iterator it = values.begin(); it != values.end(); ++it) {
     if ((*it).name == name) {
@@ -258,7 +256,7 @@ std::string StatsId(const std::string& type, const std::string& id,
 
 bool ExtractValueFromReport(
     const StatsReport& report,
-    const std::string& name,
+    StatsReport::StatsValueName name,
     std::string* value) {
   StatsReport::Values::const_iterator it = report.values.begin();
   for (; it != report.values.end(); ++it) {

talk/app/webrtc/statscollector_unittest.cc

@@ -149,7 +149,7 @@ class FakeAudioTrack
 };
 
 bool GetValue(const StatsReport* report,
-              const std::string& name,
+              StatsReport::StatsValueName name,
               std::string* value) {
   StatsReport::Values::const_iterator it = report->values.begin();
   for (; it != report->values.end(); ++it) {
@@ -163,7 +163,7 @@ bool GetValue(const StatsReport* report,
 
 std::string ExtractStatsValue(const std::string& type,
                               const StatsReports& reports,
-                              const std::string name) {
+                              StatsReport::StatsValueName name) {
   if (reports.empty()) {
     return kNoReports;
   }
@@ -204,13 +204,13 @@ const StatsReport* FindReportById(const StatsReports& reports,
 }
 
 std::string ExtractSsrcStatsValue(StatsReports reports,
-                                  const std::string& name) {
+                                  StatsReport::StatsValueName name) {
   return ExtractStatsValue(
       StatsReport::kStatsReportTypeSsrc, reports, name);
 }
 
 std::string ExtractBweStatsValue(StatsReports reports,
-                                  const std::string& name) {
+                                 StatsReport::StatsValueName name) {
   return ExtractStatsValue(
       StatsReport::kStatsReportTypeBwe, reports, name);
 }
@@ -446,7 +446,7 @@ class StatsCollectorTest : public testing::Test {
   // This creates a standard setup with a transport called "trspname"
   // having one transport channel
   // and the specified virtual connection name.
-  void InitSessionStats(const std::string vc_name) {
+  void InitSessionStats(const std::string& vc_name) {
     const std::string kTransportName("trspname");
     cricket::TransportStats transport_stats;
     cricket::TransportChannelStats channel_stats;
@@ -687,11 +687,12 @@ TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) {
   EXPECT_CALL(session_, video_channel()).WillRepeatedly(Return(&video_channel));
   EXPECT_CALL(session_, voice_channel()).WillRepeatedly(ReturnNull());
   EXPECT_CALL(*media_channel, GetStats(_, _))
-    .WillOnce(DoAll(SetArgPointee<1>(stats_read),
+      .WillOnce(DoAll(SetArgPointee<1>(stats_read),
-                    Return(true)));
+                      Return(true)));
   stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
   stats.GetStats(NULL, &reports);
-  std::string result = ExtractSsrcStatsValue(reports, "bytesSent");
+  std::string result = ExtractSsrcStatsValue(reports,
+      StatsReport::kStatsValueNameBytesSent);
   EXPECT_EQ(kBytesSentString, result);
 }
 
@@ -730,9 +731,11 @@ TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) {
 
   stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
   stats.GetStats(NULL, &reports);
-  std::string result = ExtractSsrcStatsValue(reports, "bytesSent");
+  std::string result = ExtractSsrcStatsValue(reports,
+      StatsReport::kStatsValueNameBytesSent);
   EXPECT_EQ(kBytesSentString, result);
-  result = ExtractBweStatsValue(reports, "googTargetEncBitrate");
+  result = ExtractBweStatsValue(reports,
+      StatsReport::kStatsValueNameTargetEncBitrate);
   EXPECT_EQ(kTargetEncBitrateString, result);
 }
 
@@ -850,6 +853,9 @@ TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) {
 // stats object, and that this transport stats object exists in stats.
 TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) {
   webrtc::StatsCollector stats(&session_);  // Implementation under test.
+  // Ignore unused callback (logspam).
+  EXPECT_CALL(session_, GetTransport(_))
+      .WillOnce(Return(static_cast<cricket::Transport*>(NULL)));
   MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
   // The content_name known by the video channel.
   const std::string kVcName("vcname");
@@ -919,6 +925,9 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsAbsent) {
 // an outgoing SSRC where stats are returned.
 TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) {
   webrtc::StatsCollector stats(&session_);  // Implementation under test.
+  // Ignore unused callback (logspam).
+  EXPECT_CALL(session_, GetTransport(_))
+      .WillOnce(Return(static_cast<cricket::Transport*>(NULL)));
   MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
   // The content_name known by the video channel.
   const std::string kVcName("vcname");
@@ -1190,13 +1199,15 @@ TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) {
   StatsReports reports;  // returned values.
   stats.GetStats(NULL, &reports);
   std::string result = ExtractBweStatsValue(
-      reports, "googReceivedPacketGroupPropagationDeltaSumDebug");
+      reports,
+      StatsReport::kStatsValueNameRecvPacketGroupPropagationDeltaSumDebug);
   EXPECT_EQ("10", result);
   result = ExtractBweStatsValue(
-      reports, "googReceivedPacketGroupPropagationDeltaDebug");
+      reports,
+      StatsReport::kStatsValueNameRecvPacketGroupPropagationDeltaDebug);
   EXPECT_EQ("[100, 200]", result);
   result = ExtractBweStatsValue(
-      reports, "googReceivedPacketGroupArrivalTimeDebug");
+      reports, StatsReport::kStatsValueNameRecvPacketGroupArrivalTimeDebug);
   EXPECT_EQ("[1000, 2000]", result);
 }
 
@@ -1204,6 +1215,10 @@ TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) {
 // AudioTrackInterface::GetStats() method.
 TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) {
   webrtc::StatsCollector stats(&session_);  // Implementation under test.
+  // Ignore unused callback (logspam).
+  EXPECT_CALL(session_, GetTransport(_))
+      .WillOnce(Return(static_cast<cricket::Transport*>(NULL)));
+
   MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
   // The content_name known by the voice channel.
   const std::string kVcName("vcname");
@@ -1233,6 +1248,9 @@ TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) {
 // correctly.
 TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) {
   webrtc::StatsCollector stats(&session_);  // Implementation under test.
+  // Ignore unused callback (logspam).
+  EXPECT_CALL(session_, GetTransport(_))
+      .WillOnce(Return(static_cast<cricket::Transport*>(NULL)));
   MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
   // The content_name known by the voice channel.
   const std::string kVcName("vcname");
@@ -1256,6 +1274,9 @@ TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) {
 // after a RemoveLocalAudioTrack() call.
 TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) {
   webrtc::StatsCollector stats(&session_);  // Implementation under test.
+  // Ignore unused callback (logspam).
+  EXPECT_CALL(session_, GetTransport(_))
+      .WillOnce(Return(static_cast<cricket::Transport*>(NULL)));
   MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
   // The content_name known by the voice channel.
   const std::string kVcName("vcname");
@@ -1310,6 +1331,9 @@ TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) {
 // the same ssrc, they populate stats reports correctly.
 TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) {
   webrtc::StatsCollector stats(&session_);  // Implementation under test.
+  // Ignore unused callback (logspam).
+  EXPECT_CALL(session_, GetTransport(_))
+      .WillOnce(Return(static_cast<cricket::Transport*>(NULL)));
   MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
   // The content_name known by the voice channel.
   const std::string kVcName("vcname");
@@ -1388,6 +1412,9 @@ TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) {
 // avoid duplication of code in test cases.
 TEST_F(StatsCollectorTest, TwoLocalTracksWithSameSsrc) {
   webrtc::StatsCollector stats(&session_);  // Implementation under test.
+  // Ignore unused callback (logspam).
+  EXPECT_CALL(session_, GetTransport(_))
+      .WillRepeatedly(Return(static_cast<cricket::Transport*>(NULL)));
   MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
   // The content_name known by the voice channel.
   const std::string kVcName("vcname");

talk/app/webrtc/statstypes.h

@@ -43,26 +43,55 @@ class StatsReport {
  public:
   StatsReport() : timestamp(0) { }
 
+  // TODO(tommi): Change this to be an enum type that holds all the
+  // kStatsValueName constants.
+  typedef const char* StatsValueName;
+
   std::string id;  // See below for contents.
   std::string type;  // See below for contents.
 
   struct Value {
-    std::string name;
+    Value() : name(NULL) {}
+    // The copy ctor can't be declared as explicit due to problems with STL.
+    Value(const Value& other) : name(other.name), value(other.value) {}
+    explicit Value(StatsValueName name) : name(name) {}
+    Value(StatsValueName name, const std::string& value)
+        : name(name), value(value) {
+    }
+
+    // TODO(tommi): Remove this operator once we don't need it.
+    // The operator is provided for compatibility with STL containers.
+    // The public |name| member variable is otherwise meant to be read-only.
+    Value& operator=(const Value& other) {
+      const_cast<StatsValueName&>(name) = other.name;
+      value = other.value;
+      return *this;
+    }
+
+    // TODO(tommi): Change implementation to do a simple enum value-to-static-
+    // string conversion when client code has been updated to use this method
+    // instead of the |name| member variable.
+    const char* display_name() const { return name; }
+
+    const StatsValueName name;
+
     std::string value;
   };
 
-  void AddValue(const std::string& name, const std::string& value);
+  void AddValue(StatsValueName name, const std::string& value);
-  void AddValue(const std::string& name, int64 value);
+  void AddValue(StatsValueName name, int64 value);
   template <typename T>
-  void AddValue(const std::string& name, const std::vector<T>& value);
+  void AddValue(StatsValueName name, const std::vector<T>& value);
-  void AddBoolean(const std::string& name, bool value);
+  void AddBoolean(StatsValueName name, bool value);
 
-  void ReplaceValue(const std::string& name, const std::string& value);
+  void ReplaceValue(StatsValueName name, const std::string& value);
 
   double timestamp;  // Time since 1970-01-01T00:00:00Z in milliseconds.
   typedef std::vector<Value> Values;
   Values values;
 
+  // TODO(tommi): These should all be enum values.
+
   // StatsReport types.
   // A StatsReport of |type| = "googSession" contains overall information
   // about the thing libjingle calls a session (which may contain one