Support using UMA_HISTOGRAM_CUSTOM_ENUMERATION from the renderer.

base/metrics/histogram.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Histogram is an object that aggregates statistics, and can summarize them in
 // various forms, including ASCII graphical, HTML, and numerically (as a
 // vector of numbers corresponding to each of the aggregating buckets).
 // See header file for details and examples.
 
 #include "base/metrics/histogram.h"
@@ skipping 221 matching lines @@
   Pickle pickle;
   pickle.WriteString(histogram.histogram_name());
   pickle.WriteInt(histogram.declared_min());
   pickle.WriteInt(histogram.declared_max());
   pickle.WriteSize(histogram.bucket_count());
   pickle.WriteUInt32(histogram.range_checksum());
   pickle.WriteInt(histogram.histogram_type());
   pickle.WriteInt(histogram.flags());
 
   snapshot.Serialize(&pickle);
+
+  if (histogram.histogram_type() == CUSTOM_HISTOGRAM)
+    histogram.cached_ranges()->Serialize(&pickle);

[jar (doing other things), 2011/11/11 19:43:38]

Maybe this should be a call to a base class method:
histogram.SerializeRange(&pickle).

Most classes implement that as a no-op (since they don't ned to recover the
data)... but the CustomHistogram class implements it as the real pickle.

This is driven by the thought that the Serialize() method above really doesn't
belong on the CachedRanges class (I think).

[Ami GONE FROM CHROMIUM, 2011/11/11 22:44:48]

I think having Histogram have two methods, a static SerializeHistogramInfo() and
a non-static SerializeRanges() is strictly less good than the current code.
Is there a reason SerializeHistogramInfo is static instead of being (virtual)
std::string Histogram::Serialize()
and taking the snapshot from *this?

I think making that change would make sense, as would de-staticifying the
Deserialize method (and separating out the deserialization from the creation of
a new singleton histogram).  Then CustomHistogram::Serialize() could override
Histogram::Serialize() and delegate to it to serialize the base state, and add
its own serialization of the ranges.

That seems like a different CL, though.

+
   return std::string(static_cast<const char*>(pickle.data()), pickle.size());
 }
 
 // static
 bool Histogram::DeserializeHistogramInfo(const std::string& histogram_info) {
   if (histogram_info.empty()) {
-      return false;
+    return false;
   }
 
   Pickle pickle(histogram_info.data(),
                 static_cast<int>(histogram_info.size()));
   std::string histogram_name;
   int declared_min;
   int declared_max;
   size_t bucket_count;
   uint32 range_checksum;
   int histogram_type;
   int pickle_flags;
   SampleSet sample;
 
   void* iter = NULL;
   if (!pickle.ReadString(&iter, &histogram_name) ||
       !pickle.ReadInt(&iter, &declared_min) ||
       !pickle.ReadInt(&iter, &declared_max) ||
       !pickle.ReadSize(&iter, &bucket_count) ||
       !pickle.ReadUInt32(&iter, &range_checksum) ||
       !pickle.ReadInt(&iter, &histogram_type) ||
       !pickle.ReadInt(&iter, &pickle_flags) ||
       !sample.Histogram::SampleSet::Deserialize(&iter, pickle)) {
     DLOG(ERROR) << "Pickle error decoding Histogram: " << histogram_name;
     return false;
   }
+
+  std::vector<Histogram::Sample> sample_ranges;
+  if (histogram_type == CUSTOM_HISTOGRAM) {
+    CachedRanges ranges(bucket_count, 0);
+    if (!ranges.Deserialize(&iter, pickle) ||
+        ranges.size() != bucket_count) {
+      DLOG(ERROR) << "Pickle error decoding ranges: " << histogram_name;
+      return false;
+    }
+    for (size_t i = 0; i < bucket_count; ++i)
+      sample_ranges.push_back(ranges.ranges(i));
+  }
+
   DCHECK(pickle_flags & kIPCSerializationSourceFlag);
   // Since these fields may have come from an untrusted renderer, do additional
   // checks above and beyond those in Histogram::Initialize()
   if (declared_max <= 0 || declared_min <= 0 || declared_max < declared_min ||
       INT_MAX / sizeof(Count) <= bucket_count || bucket_count < 2) {
     DLOG(ERROR) << "Values error decoding Histogram: " << histogram_name;
     return false;
   }
 
   Flags flags = static_cast<Flags>(pickle_flags & ~kIPCSerializationSourceFlag);
 
   DCHECK_NE(NOT_VALID_IN_RENDERER, histogram_type);
 
   Histogram* render_histogram(NULL);
 
   if (histogram_type == HISTOGRAM) {
     render_histogram = Histogram::FactoryGet(
         histogram_name, declared_min, declared_max, bucket_count, flags);
   } else if (histogram_type == LINEAR_HISTOGRAM) {
     render_histogram = LinearHistogram::FactoryGet(
         histogram_name, declared_min, declared_max, bucket_count, flags);
   } else if (histogram_type == BOOLEAN_HISTOGRAM) {
     render_histogram = BooleanHistogram::FactoryGet(histogram_name, flags);
+  } else if (histogram_type == CUSTOM_HISTOGRAM) {
+    render_histogram =
+        CustomHistogram::FactoryGet(histogram_name, sample_ranges, flags);
   } else {
     DLOG(ERROR) << "Error Deserializing Histogram Unknown histogram_type: "
                 << histogram_type;
     return false;
   }
 
   DCHECK_EQ(render_histogram->declared_min(), declared_min);
   DCHECK_EQ(render_histogram->declared_max(), declared_max);
   DCHECK_EQ(render_histogram->bucket_count(), bucket_count);
   DCHECK_EQ(render_histogram->range_checksum(), range_checksum);
@@ skipping 966 matching lines @@
     return false;
   if (ranges_.size() != other->ranges_.size())
     return false;
   for (size_t index = 0; index < ranges_.size(); ++index) {
     if (ranges_[index] != other->ranges_[index])
       return false;
   }
   return true;
 }
 
+

[jar (doing other things), 2011/11/11 19:43:38]

nit: remove extra blank line

[Ami GONE FROM CHROMIUM, 2011/11/11 22:44:48]

Done.

+bool CachedRanges::Serialize(Pickle* pickle) const {

[jar (doing other things), 2011/11/11 19:43:38]

I *think* that if you want to put this method in a class, it should be a method
on the CustomHistogram class.  That class has to produce a pickled vector, which
can then be used in a constructor... to get an equivalent histogram.  This 
CachedRanges class is really focusing on de-duping.

[Ami GONE FROM CHROMIUM, 2011/11/11 22:44:48]

I agree with your basic concern, which is that these are strange methods to
offer.
Instead, I moved both this method and the next to file-static in the .cc file.

+  for (size_t i = 0; i < ranges_.size(); ++i) {
+    if (!pickle->WriteInt(ranges_[i]))
+      return false;
+  }
+  return true;
+}
+
+bool CachedRanges::Deserialize(void** iter, const Pickle& pickle) {

[jar (doing other things), 2011/11/11 19:43:38]

I'm not convinced that this method should be in the CachedRanges class.  This is
especially clear when you immediately copy the results into a vector. 
Specifically, we never register this instance directly (using the classes de-dup
ability).

I think this should be a static method on the CustomHistogram class, which
inverts the pickle, producing the vector that can be handed back to the
CustomHistogram constructor.

[Ami GONE FROM CHROMIUM, 2011/11/11 22:44:48]

See above.

+  for (size_t i = 0; i < ranges_.size(); ++i) {
+    if (!pickle.ReadInt(iter, &ranges_[i]))
+      return false;
+  }
+  return true;
+}
+
 // static
 StatisticsRecorder::HistogramMap* StatisticsRecorder::histograms_ = NULL;
 // static
 StatisticsRecorder::RangesMap* StatisticsRecorder::ranges_ = NULL;
 // static
 base::Lock* StatisticsRecorder::lock_ = NULL;
 // static
 bool StatisticsRecorder::dump_on_exit_ = false;
 }  // namespace base