From 1f7a89a5899669a3f59871214852012bb7f44d25 Mon Sep 17 00:00:00 2001
From: Nathan Froyd <froydnj@mozilla.com>
Date: Tue, 8 May 2012 10:58:20 -0400
Subject: [PATCH] Bug 748907 - don't reflect no-data histograms; r=taras

---
 toolkit/components/telemetry/Telemetry.cpp    | 20 ++++++++++++++++++-
 toolkit/components/telemetry/TelemetryPing.js |  3 ++-
 .../telemetry/tests/unit/test_nsITelemetry.js |  8 ++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/toolkit/components/telemetry/Telemetry.cpp b/toolkit/components/telemetry/Telemetry.cpp
index 9412181aec6..6709df05e54 100644
--- a/toolkit/components/telemetry/Telemetry.cpp
+++ b/toolkit/components/telemetry/Telemetry.cpp
@@ -397,6 +397,15 @@ ReflectHistogramSnapshot(JSContext *cx, JSObject *obj, Histogram *h)
   return ReflectHistogramAndSamples(cx, obj, h, ss);
 }
 
+bool
+IsEmpty(const Histogram *h)
+{
+  Histogram::SampleSet ss;
+  h->SnapshotSample(&ss);
+
+  return ss.counts(0) == 0 && ss.sum() == 0;
+}
+
 JSBool
 JSHistogram_Add(JSContext *cx, unsigned argc, jsval *vp)
 {
@@ -860,7 +869,7 @@ TelemetryImpl::GetHistogramSnapshots(JSContext *cx, jsval *ret)
   // OK, now we can actually reflect things.
   for (HistogramIterator it = hs.begin(); it != hs.end(); ++it) {
     Histogram *h = *it;
-    if (!ShouldReflectHistogram(h)) {
+    if (!ShouldReflectHistogram(h) || IsEmpty(h)) {
       continue;
     }
 
@@ -923,6 +932,10 @@ TelemetryImpl::AddonHistogramReflector(AddonHistogramEntryType *entry,
     }
   }
 
+  if (IsEmpty(info.h)) {
+    return true;
+  }
+
   JSObject *snapshot = JS_NewObject(cx, NULL, NULL, NULL);
   if (!snapshot) {
     // Just consider this to be skippable.
@@ -1432,6 +1445,11 @@ TelemetrySessionData::SerializeHistogramData(Pickle &pickle)
     const Histogram *h = *it;
     const char *name = h->histogram_name().c_str();
 
+    // We don't check IsEmpty(h) here.  We discard no-data histograms on
+    // read-in, instead.  It's easier to write out the number of
+    // histograms required that way.  (The pickle interface doesn't make
+    // it easy to go back and overwrite previous data.)
+
     Histogram::SampleSet ss;
     h->SnapshotSample(&ss);
 
diff --git a/toolkit/components/telemetry/TelemetryPing.js b/toolkit/components/telemetry/TelemetryPing.js
index a83c4222915..233462c4e13 100644
--- a/toolkit/components/telemetry/TelemetryPing.js
+++ b/toolkit/components/telemetry/TelemetryPing.js
@@ -282,7 +282,8 @@ TelemetryPing.prototype = {
       for (let name in addonHistograms) {
         packedHistograms[name] = this.packHistogram(addonHistograms[name]);
       }
-      ret[addonName] = packedHistograms;
+      if (Object.keys(packedHistograms).length != 0)
+        ret[addonName] = packedHistograms;
     }
 
     return ret;
diff --git a/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js b/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
index 96296726aa5..b96a74d5dc1 100644
--- a/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
+++ b/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ -195,6 +195,9 @@ function test_addons() {
 
   // Check for reflection capabilities.
   var h1 = Telemetry.getAddonHistogram(addon_id, name1);
+  // Verify that although we've created storage for it, we don't reflect it into JS.
+  var snapshots = Telemetry.addonHistogramSnapshots;
+  do_check_false(name1 in snapshots[addon_id]);
   h1.add(1);
   h1.add(3);
   var s1 = h1.snapshot();
@@ -328,6 +331,11 @@ function run_test()
     expect_fail(function () nh("test::bucket_count", min, max, 1, histogram_type));
   }
 
+  // Instantiate the storage for this histogram and make sure it doesn't
+  // get reflected into JS, as it has no interesting data in it.
+  let h = Telemetry.getHistogramById("NEWTAB_PAGE_PINNED_SITES_COUNT");
+  do_check_false("NEWTAB_PAGE_PINNED_SITES_COUNT" in Telemetry.histogramSnapshots);
+
   test_boolean_histogram();
   test_getHistogramById();
   test_histogramFrom();