Support using UMA_HISTOGRAM_CUSTOM_ENUMERATION from the renderer.

base/metrics/histogram.cc

Index: base/metrics/histogram.cc
diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index 7077b090715d6bab39ab489435cbb97f3af133db..9ae16022ca95c16118914c6049ba22a8ab5e3816 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -239,13 +239,17 @@ std::string Histogram::SerializeHistogramInfo(const Histogram& histogram,
   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(),
@@ -271,6 +275,19 @@ bool Histogram::DeserializeHistogramInfo(const std::string& histogram_info) {
     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()
@@ -294,6 +311,9 @@ bool Histogram::DeserializeHistogramInfo(const std::string& histogram_info) {
         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;
@@ -1280,6 +1300,23 @@ bool CachedRanges::Equals(CachedRanges* other) const {
   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