Support using UMA_HISTOGRAM_CUSTOM_ENUMERATION from the renderer.

Support using UMA_HISTOGRAM_CUSTOM_ENUMERATION from the renderer.

BUG=103633
TEST=trybots & manually checking that chrome://histograms/Aspect now shows the two custom enum histograms from r108951, with correctly specific buckets.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109823

[Ami GONE FROM CHROMIUM, 2011-11-10 04:14:21]

Jim: please review.

BTW, I tried to write some unittests for this and realized after a bit that
DeserializeHistogramInfo is all about the global registry of histograms, making
it hard to unittest (and explaining why there are no tests for the existing
pickling business).  I think DHI should be pulled apart to support testing
(separate the unpickling from the global registry business) but that's probably
fodder for a different CL.

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

Thanks for spending the time on this!!

I think the function is correct... but I had suggestions about where the methods
should be placed.  If you have a strong argument to the contrary... that would
be fine as well.

WDYT of the comments??

Thanks,

Jim

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:244: histogram.cached_ranges()->Serialize(&pickle);
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).

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1303: 
nit: remove extra blank line

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1304: bool CachedRanges::Serialize(Pickle* pickle)
const {
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.

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1312: bool CachedRanges::Deserialize(void** iter,
const Pickle& pickle) {
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]

Thanks & please take another look.

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:244: histogram.cached_ranges()->Serialize(&pickle);
On 2011/11/11 19:43:38, jar wrote:
> 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).

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.

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1303: 
On 2011/11/11 19:43:38, jar wrote:
> nit: remove extra blank line

Done.

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1304: bool CachedRanges::Serialize(Pickle* pickle)
const {
On 2011/11/11 19:43:38, jar wrote:
> 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.

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.

http://codereview.chromium.org/8506038/diff/2001/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1312: bool CachedRanges::Deserialize(void** iter,
const Pickle& pickle) {
On 2011/11/11 19:43:38, jar wrote:
> 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.

See above.

[jar (doing other things), 2011-11-12 00:08:56]

The interesting question is: Who (what class) should be able to determine a
proper argument to its constructor that would be needed to create a given range
vector?  Each of these classes, derived from Histogram, does some funky little
things to the constructor arguments that are provided, in order to establish the
"right" range vector.  For instance, they always create an over-flow bucket, and
always have a 0 bucket.  

I claim this sort of knowledge should be encapsulated in the class.  The two
(file) static function you created have exactly that sort of information about
the structure of CustomHistogram, and how its constructor argument is handled. 
As a result, they should both be (at a minimum) members of the CustomHistogram
class.

Once I realized they needed to be methods in the class, and I saw that the
serialize method took a "this" as an argument, it was clear it should be member
function (rather than static member function).  The deserialize needs to be a
static method, as it is run with no instance yet in sight.

Finally, looking at your code, I saw you called the serialize method *iff* we
were a specific derived class... and that smelled just like dynamic dispatch
(handled in a non-C++ way). Hence it was clear that putting a no-op method in
the base class would allow the override, and all would be well.

I think it ends up being basically your CL... with the tiny bit of adjustment
for which class gets which methods, and how.

[Ami GONE FROM CHROMIUM, 2011-11-12 04:26:55]

PTAL - coded up your suggestion.
I still don't think this approach is better, but it's your baby, so your call :)

[jar (doing other things), 2011-11-13 01:47:33]

lgtm

[commit-bot: I haz the power, 2011-11-13 02:01:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/8506038/4003

[commit-bot: I haz the power, 2011-11-13 02:43:09]

Try job failure for 8506038-4003 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-13 03:08:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/8506038/14001

[commit-bot: I haz the power, 2011-11-13 04:19:17]

Change committed as 109823