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..8f7ec4b4bb715348af69e3d4068e61f423ea496f 100644
--- a/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
+++ b/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc
@@ -38,6 +38,8 @@ 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},

[Ilya Sherman, 2014/03/26 22:02:03]

Where are you testing which bucket was emitted to?

[robliao, 2014/03/26 22:30:50]

Can't really do this as there's no way to get the buckets out in a nice way. We
should definitely test for this once it gets implemented.

void SparseHistogram::GetCountAndBucketData(Count* count,
                                            int64* sum,
                                            ListValue* buckets) const {
  // TODO(kaiwang): Implement. (See HistogramBase::WriteJSON.)
}

On 2014/03/26 22:02:03, Ilya Sherman wrote:

[Ilya Sherman, 2014/03/26 22:44:14]

You're already extracting a HistogramSamples object for the histogram.  That has
a GetCount() method that will return the count for a specific bucket.  I don't
think GetCountAndBucketData() is relevant -- that's for generating the
chrome://histograms UI.

On 2014/03/26 22:30:50, robliao wrote:

       {"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},
@@ -116,8 +118,13 @@ void ValidateHistograms(const RecordedHistogram* recorded,
 
       if (r.name == histogram->histogram_name()) {
         EXPECT_EQ(r.type, histogram->GetHistogramType());
-        EXPECT_TRUE(
-            histogram->HasConstructionArguments(r.min, r.max, r.buckets));
+        if (r.type == base::SPARSE_HISTOGRAM) {
+          EXPECT_FALSE(
+              histogram->HasConstructionArguments(r.min, r.max, r.buckets));
+        } else {
+          EXPECT_TRUE(
+              histogram->HasConstructionArguments(r.min, r.max, r.buckets));
+        }

[not at google - send to devlin, 2014/03/26 20:39:45]

you could

EXPECT_EQ(r.type == base::HISTOGRAM,
          histogram->HasConstructionArguments(r.min, r.max, r.buckets));

[robliao, 2014/03/26 21:15:43]

While true, that seems to be a bit too cute for readability.

And it's not just histograms, it's also LINEAR_HISTOGRAM, so it would look like

EXPECT_EQ(r.type != base::SPARSE_HISTOGRAM,
histogram->HasConstructionArguments(r.min, r.max, r.buckets))
On 2014/03/26 20:39:45, kalman wrote:

[not at google - send to devlin, 2014/03/26 21:19:35]

ok. I don't think it's overly cute actually. Is there something you can assert
about the sparse case to make it more obvious? like what arguments is it
supposed to have...?

         scoped_ptr<base::HistogramSamples> snapshot =
             histogram->SnapshotSamples();
         base::HistogramBase::Count sample_count = snapshot->TotalCount();