Add Sparse Histogram Support to metricsPrivate

chrome/browser/extensions/api/metrics_private/metrics_apitest.cc

Index: chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
diff --git a/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc b/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
index f910a35279edd9860388eae837e309b56e313b30..dae08477be5334d7ea0b0eae60cb59c05cff8b15 100644
--- a/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
+++ b/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
@@ -38,6 +38,9 @@ struct RecordedHistogram {
       {"test.h.1", base::HISTOGRAM, 1, 100, 50, 1},          // custom
       {"test.h.2", base::LINEAR_HISTOGRAM, 1, 200, 50, 1},   // custom
       {"test.h.3", base::LINEAR_HISTOGRAM, 1, 101, 102, 2},  // percentage
+      {"test.sparse.1", base::SPARSE_HISTOGRAM, 0, 0, 0, 1},
+      {"test.sparse.2", base::SPARSE_HISTOGRAM, 0, 0, 0, 2},
+      {"test.sparse.3", base::SPARSE_HISTOGRAM, 0, 0, 0, 6},
       {"test.time", base::HISTOGRAM, 1, 10000, 50, 1},
       {"test.medium.time", base::HISTOGRAM, 1, 180000, 50, 1},
       {"test.long.time", base::HISTOGRAM, 1, 3600000, 50, 1},
@@ -47,6 +50,22 @@ struct RecordedHistogram {
       {"test.bucketchange.linear", base::LINEAR_HISTOGRAM, 1, 100, 10, 2},
       {"test.bucketchange.log", base::HISTOGRAM, 1, 100, 10, 2}, };
 
+// Represents a bucket in a sparse histogram.
+struct Bucket {
+  int histogramValue;

[Ilya Sherman, 2014/03/26 23:52:13]

nit: Please use hacker_case.

[robliao, 2014/03/27 00:02:44]

Done.

+  int count;
+};
+
+// We expect the following sparse histograms.
+struct SparseHistogram {
+  const char* name;
+  int bucket_count;
+  Bucket buckets[10];
+} g_sparse_histograms[] = {
+  {"test.sparse.1", 1, {{42, 1}}},
+  {"test.sparse.2", 1, {{24, 2}}},
+  {"test.sparse.3", 3, {{1, 1}, {2, 2}, {3, 3}}}};
+
 // This class observes and collects user action notifications that are sent
 // by the tests, so that they can be examined afterwards for correctness.
 class UserActionObserver {
@@ -99,6 +118,20 @@ void UserActionObserver::ValidateUserActions(const RecordedUserAction* recorded,
   }
 }
 
+void ValidateSparseHistogramSamples(
+    const char* name,
+    const scoped_ptr<base::HistogramSamples>& samples) {

[Ilya Sherman, 2014/03/26 23:52:13]

nit: Why pass the scoped_ptr, rather than the pointed-to object?

[robliao, 2014/03/27 00:02:44]

That would work too.
On 2014/03/26 23:52:13, Ilya Sherman wrote:

+  for (int i = 0; i  < arraysize(g_sparse_histograms); i++) {

[Ilya Sherman, 2014/03/26 23:52:13]

nit: ++i

[Ilya Sherman, 2014/03/26 23:52:13]

nit: "i  <" -> "i <" (one fewer spaces)

[robliao, 2014/03/27 00:02:44]

Done.

+    const SparseHistogram& sparse_histogram = g_sparse_histograms[i];
+    if (name == sparse_histogram.name) {

[Ilya Sherman, 2014/03/26 23:52:13]

Comparing C-strings is a pointer comparison, which is probably not what you
want.  The simplest fix is to change the type of the |name| parameter to be
std::string.

[robliao, 2014/03/27 00:02:44]

Oops, too much jumping between JS and C++>. Yup, I forgot the std::string.

Oddly enough, the code works (tested with bogus counts) and it shouldn't...
On 2014/03/26 23:52:13, Ilya Sherman wrote:

[Ilya Sherman, 2014/03/27 00:08:16]

Sometimes optimizing compilers are *too* helpful ;)

On 2014/03/27 00:02:44, robliao wrote:

[robliao, 2014/03/27 00:21:53]

I was thinking that, but these strings also come from JS Land at runtime.
On 2014/03/27 00:08:16, Ilya Sherman wrote:

+      for (int j = 0; j < sparse_histogram.bucket_count; j++) {

[Ilya Sherman, 2014/03/26 23:52:13]

nit: ++j

[robliao, 2014/03/27 00:02:44]

Done.

+        const Bucket& bucket = sparse_histogram.buckets[j];
+        EXPECT_EQ(bucket.count, samples->GetCount(bucket.histogramValue));
+      }
+    }
+  }
+}
+
 void ValidateHistograms(const RecordedHistogram* recorded,
                         int count) {
   base::StatisticsRecorder::Histograms histograms;
@@ -113,15 +146,19 @@ void ValidateHistograms(const RecordedHistogram* recorded,
     size_t j = 0;
     for (j = 0; j < histograms.size(); ++j) {
       base::HistogramBase* histogram(histograms[j]);
-
       if (r.name == histogram->histogram_name()) {
-        EXPECT_EQ(r.type, histogram->GetHistogramType());
-        EXPECT_TRUE(
-            histogram->HasConstructionArguments(r.min, r.max, r.buckets));
         scoped_ptr<base::HistogramSamples> snapshot =
             histogram->SnapshotSamples();
         base::HistogramBase::Count sample_count = snapshot->TotalCount();
-        EXPECT_EQ(sample_count, r.count);
+        EXPECT_EQ(r.count, sample_count);
+
+        EXPECT_EQ(r.type, histogram->GetHistogramType());
+        if (r.type == base::SPARSE_HISTOGRAM) {
+          ValidateSparseHistogramSamples(r.name, snapshot);
+        } else {
+          EXPECT_TRUE(
+              histogram->HasConstructionArguments(r.min, r.max, r.buckets));
+        }
         break;
       }
     }