Bug 1245910 - Enforce correct types in Histograms.json. r=gfritzsche

- Convert string values to integer and boolean values for Histograms
- Add compile time type checks on Histograms.json
- Remove string to int/bool coercion for the Histogram keys:
  - n_buckets
  - n_values
  - low
  - high
  - keyed
- Remove check_numeric (redundant with check_expression_types)

toolkit/components/telemetry/histogram_tools.py

@@ -37,16 +37,7 @@ def table_dispatch(kind, table, body):
 class DefinitionException(BaseException):
     pass
 
-def check_numeric_limits(dmin, dmax, n_buckets):
-    if type(dmin) != int:
-        raise DefinitionException, "minimum is not a number"
-    if type(dmax) != int:
-        raise DefinitionException, "maximum is not a number"
-    if type(n_buckets) != int:
-        raise DefinitionException, "number of buckets is not a number"
-
 def linear_buckets(dmin, dmax, n_buckets):
-    check_numeric_limits(dmin, dmax, n_buckets)
     ret_array = [0] * n_buckets
     dmin = float(dmin)
     dmax = float(dmax)
@@ -56,7 +47,6 @@ def linear_buckets(dmin, dmax, n_buckets):
     return ret_array
 
 def exponential_buckets(dmin, dmax, n_buckets):
-    check_numeric_limits(dmin, dmax, n_buckets)
     log_max = math.log(dmax);
     bucket_index = 2;
     ret_array = [0] * n_buckets
@@ -103,7 +93,6 @@ definition is a dict-like object that must contain at least the keys:
 
 The key 'cpp_guard' is optional; if present, it denotes a preprocessor
 symbol that should guard C/C++ definitions associated with the histogram."""
-        self.check_name(name)
         self.verify_attributes(name, definition)
         self._name = name
         self._description = definition['description']
@@ -153,15 +142,15 @@ the histogram."""
         self._nsITelemetry_kind = "nsITelemetry::HISTOGRAM_%s" % kind
 
     def low(self):
-        """Return the lower bound of the histogram.  May be a string."""
+        """Return the lower bound of the histogram."""
         return self._low
 
     def high(self):
-        """Return the high bound of the histogram.  May be a string."""
+        """Return the high bound of the histogram."""
         return self._high
 
     def n_buckets(self):
-        """Return the number of buckets in the histogram.  May be a string."""
+        """Return the number of buckets in the histogram."""
         return self._n_buckets
 
     def cpp_guard(self):
@@ -219,10 +208,13 @@ associated with the histogram.  Returns None if no guarding is necessary."""
             and not isinstance(definition['alert_emails'], list)):
             raise KeyError, 'alert_emails must be an array if present (in Histogram %s)' % name
 
+        Histogram.check_name(name)
+        Histogram.check_field_types(name, definition)
         Histogram.check_expiration(name, definition)
         Histogram.check_bug_numbers(name, definition)
 
-    def check_name(self, name):
+    @staticmethod
+    def check_name(name):
         if '#' in name:
             raise ValueError, '"#" not permitted for %s' % (name)
 
@@ -252,6 +244,31 @@ associated with the histogram.  Returns None if no guarding is necessary."""
         if not all(type(num) is int for num in bug_numbers):
             raise ValueError, 'bug_numbers array for "%s" should only contain integers' % (name)
 
+    @staticmethod
+    def check_field_types(name, definition):
+        type_checked_fields = {
+                "n_buckets": int,
+                "n_values": int,
+                "low": int,
+                "high": int,
+                "keyed": bool,
+                "expires_in_version": basestring,
+                "kind": basestring,
+                "description": basestring,
+                "cpp_guard": basestring,
+                "releaseChannelCollection": basestring
+            }
+        for key, key_type in type_checked_fields.iteritems():
+            if not key in definition:
+                continue
+            if not isinstance(definition[key], key_type):
+                if key_type is basestring:
+                    type_name = "string"
+                else:
+                    type_name = key_type.__name__
+                raise ValueError, ('value for key "{0}" in Histogram "{1}" '
+                        'should be {2}').format(key, name, type_name)
+
     @staticmethod
     def check_keys(name, definition, allowed_keys):
         for key in definition.iterkeys():
@@ -259,14 +276,9 @@ associated with the histogram.  Returns None if no guarding is necessary."""
                 raise KeyError, '%s not permitted for %s' % (key, name)
 
     def set_bucket_parameters(self, low, high, n_buckets):
-        def try_to_coerce_to_number(v):
+        self._low = low
-            try:
+        self._high = high
-                return eval(v, {})
+        self._n_buckets = n_buckets
-            except:
-                return v
-        self._low = try_to_coerce_to_number(low)
-        self._high = try_to_coerce_to_number(high)
-        self._n_buckets = try_to_coerce_to_number(n_buckets)
         if n_buckets_whitelist is not None and self._n_buckets > 100 and type(self._n_buckets) is int:
             if self._name not in n_buckets_whitelist:
                 raise KeyError, ('New histogram %s is not permitted to have more than 100 buckets. '
@@ -286,7 +298,7 @@ associated with the histogram.  Returns None if no guarding is necessary."""
     @staticmethod
     def enumerated_bucket_parameters(definition):
         n_values = definition['n_values']
-        return (1, n_values, "%s+1" % n_values)
+        return (1, n_values, n_values + 1)
 
     @staticmethod
     def exponential_bucket_parameters(definition):