Bug 1442667 - Refactor internal_JSKeyedHistogram_Add r=chutten
authorAditya Bharti <adibhar97@gmail.com>
Sat, 24 Mar 2018 10:29:29 +0530
changeset 463562 bfa48d6c1c6c951cd3a5fa359a319d7a6417a47a
parent 463561 3c83fef21eb2d9002b2c80e3d3e8c52d11a24b37
child 463563 e94fdbab0e4a65d4a25ab6d5749976ec51e465ef
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten
bugs1442667, 1428893
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1442667 - Refactor internal_JSKeyedHistogram_Add r=chutten Use the type checking utility function introduced in bug 1428893 Added more tests to increase code coveragea and ensure no regressions
toolkit/components/telemetry/TelemetryHistogram.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -1582,53 +1582,41 @@ internal_JSKeyedHistogram_Add(JSContext 
       NS_ConvertASCIItoUTF16(gHistogramInfos[id].name()), 1);
     return true;
   }
 
   const uint32_t type = gHistogramInfos[id].histogramType;
 
   // If we don't have an argument for the count histogram, assume an increment of 1.
   // Otherwise, make sure to run some sanity checks on the argument.
-  uint32_t value = 1;
-  if ((type != nsITelemetry::HISTOGRAM_COUNT) || (args.length() == 2)) {
-    if (args.length() < 2) {
+
+  // Special case of only key argument and count histogram
+  if (args.length() == 1) {
+    if (!(type == nsITelemetry::HISTOGRAM_COUNT)) {
       LogToBrowserConsole(nsIScriptError::errorFlag,
-                          NS_LITERAL_STRING("Expected two arguments for this histogram type"));
+          NS_LITERAL_STRING("Need at least one argument for non count type histogram"));
       return true;
     }
 
-    if (type == nsITelemetry::HISTOGRAM_CATEGORICAL && args[1].isString()) {
-      // For categorical histograms we allow passing a string argument that specifies the label.
-
-      // Get label string.
-      nsAutoJSString label;
-      if (!label.init(cx, args[1])) {
-        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid string parameter"));
-        return true;
-      }
+    {
+      StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+      internal_Accumulate(id, NS_ConvertUTF16toUTF8(key), 1);
+    }
+    return true;
+  }
 
-      // Get label id value.
-      nsresult rv = gHistogramInfos[id].label_id(NS_ConvertUTF16toUTF8(label).get(), &value);
-      if (NS_FAILED(rv)) {
-        LogToBrowserConsole(nsIScriptError::errorFlag,
-                            NS_LITERAL_STRING("Unknown label for categorical histogram"));
-        return true;
-      }
-    } else {
-      // All other accumulations expect one numerical argument.
-      if (!(args[1].isNumber() || args[1].isBoolean())) {
-        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a number"));
-        return true;
-      }
+  if (args.length() < 2) {
+    LogToBrowserConsole(nsIScriptError::errorFlag,
+                        NS_LITERAL_STRING("Expected two arguments for this histogram type"));
+    return true;
+  }
 
-      if (!JS::ToUint32(cx, args[1], &value)) {
-        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed to convert argument"));
-        return true;
-      }
-    }
+  uint32_t value = 0;
+  if (!internal_JSHistogram_CoerceValue(cx, args[1], id, type, value)) {
+    return true;
   }
 
   {
     StaticMutexAutoLock locker(gTelemetryHistogramMutex);
     internal_Accumulate(id, NS_ConvertUTF16toUTF8(key), value);
   }
 
   return true;
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ -1121,8 +1121,33 @@ add_task(async function test_linear_mult
 
   h.add(valid);
   let s2 = h.snapshot();
   // Values >= INT32_MAX are accumulated as INT32_MAX - 1
   Assert.equal(s2.sum, valid.reduce((acc, cur) => acc + cur) - 3);
   Assert.equal(s2.counts[9], 1);
   Assert.deepEqual(s2.counts.slice(0, 3), [1, 3, 2]);
 });
+
+add_task(async function test_keyed_no_arguments() {
+  // Test for no accumulation when add is called with no arguments
+  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_LINEAR");
+  h.clear();
+
+  h.add();
+
+  let s = h.snapshot();
+  // Snapshot property sum is undefined until the first accumulation.
+  Assert.equal(s.sum, undefined);
+});
+
+add_task(async function test_keyed_categorical_invalid_string() {
+  // Test for no accumulation when add is called on a
+  // keyed categorical histogram with an invalid string label.
+  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_CATEGORICAL");
+  h.clear();
+
+  h.add("someKey", "#notALablel");
+
+  let s = h.snapshot();
+  // Snapshot property sum is undefined until the first accumulation.
+  Assert.equal(s.sum, undefined);
+});