Serialize/Deserialize support in HistogramBase

Serialize/Deserialize support in HistogramBase

BUG=139612, 167343
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176449

[kaiwang, 2012-12-28 19:27:54]

ping...

[Ilya Sherman, 2012-12-29 00:17:29]

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:167:
DCHECK(histogram.bucket_ranges()->HasValidChecksum());
What happened to this DCHECK?

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:263: DCHECK_EQ(flags & render_histogram->flags(),
flags);
nit: What happened to this DCHECK?

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:64: // in this process. So need to clear IPC flag
first.
nit: "in this process. So need to clear IPC flag" -> "in this process, so we
need to clear the IPC flag"

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:71: bool ValidateRangeChecksum(const HistogramBase*
histogram,
nit: Please pass by const-reference.

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:325: pickle->WriteInt(flags()) &&
I'm curious, why did you move flags to be written earlier in the pickle than it
had been?

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:332: return false;
Optional nit: IMO it's tidier to write this as

return pickle->WriteString(histogram_name()) && ...

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:775: }
It would be good to share more of this code, rather than repeating it three
times.  Why not move this logic into the method currently named
ReadHistogramArguments, and pass the histogram type as a parameter into that
method?  If the type is a custom histogram you'd include the block to
deserialize the ranges; otherwise, you'd just skip it.

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.h:496: PickleIterator* iter);
Why does HistogramBase need to know about the Histogram class?  That seems like
a bad dependency...

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.h:582: };
nit: Please move this to be above the comment that's describing
FactoryGetWithRangeDescription.

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
File base/metrics/histogram_base.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram_base.h:92: // serialize the samples.
nit: "Note: This only serializes the construction parameters for the histogram;
it does not serialize the samples."

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
File base/metrics/histogram_base_unittest.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram_base_unittest.cc:18: virtual void SetUp() {
nit: OVERRIDE

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram_base_unittest.cc:23: }
nit: Why not do this in the constructor?

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram_base_unittest.cc:25: virtual void TearDown() {
nit: OVERRIDE

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram_base_unittest.cc:35: StatisticsRecorder*
statistics_recorder_;
nit: Please make this a scoped_ptr<>

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram_unittest.cc:429: EXPECT_EQ(0, range);
I thought the 0 and INT_MAX ranges weren't serialized?

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/sparse_hi...
File base/metrics/sparse_histogram.h (left):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/sparse_hi...
base/metrics/sparse_histogram.h:53: HistogramBase::Count redundant_count_;
Could you remind me of how the redundant count was going to be used, and why
it's no longer relevant?

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/sparse_hi...
File base/metrics/sparse_histogram.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/sparse_hi...
base/metrics/sparse_histogram.h:61: // Protects access to |sample_counts_| and
|redundant_count_|.
nit: Please update this comment.

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/sparse_hi...
File base/metrics/sparse_histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/sparse_hi...
base/metrics/sparse_histogram_unittest.cc:65: }
It seems like a more fruitful test would be to test that you can serialize a
histogram and deserialize it back to its original form.  There's currently no
test coverage for the deserialization code...

https://chromiumcodereview.appspot.com/11682003/diff/1/chrome/common/metrics/...
File chrome/common/metrics/metrics_service_base.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/chrome/common/metrics/...
chrome/common/metrics/metrics_service_base.cc:41:
log_manager_.current_log()->RecordHistogramDelta(histogram.histogram_name(),
This is a fine change, but why is it part of this CL?

https://chromiumcodereview.appspot.com/11682003/diff/1/content/browser/histog...
File content/browser/histogram_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/content/browser/histog...
content/browser/histogram_synchronizer.cc:280: if (histogram) {
Optional nit: IMO it's tidier to write "if (!histogram) continue" to reduce the
level of indentation.

https://chromiumcodereview.appspot.com/11682003/diff/1/content/browser/histog...
content/browser/histogram_synchronizer.cc:286:
histogram->AddSamplesFromPickle(&iter);
Why isn't this done as part of DeserializeHistogramInfo() (which I would suggest
renaming to DeserializeHistogram())?  Would you ever want to deserialize just
the construction parameters, and not the samples?

https://chromiumcodereview.appspot.com/11682003/diff/1/content/common/child_h...
File content/common/child_histogram_message_filter.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/content/common/child_h...
content/common/child_histogram_message_filter.cc:63: // histograms.
nit: "Note." -> "Note:"; "to all" -> "for all the"; "histograms. So the" ->
"histograms, so that the"; "distinguish them with" -> "distinguish them from".

[kaiwang, 2013-01-08 00:51:40]

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc#old...
base/metrics/histogram.cc:167:
DCHECK(histogram.bucket_ranges()->HasValidChecksum());
Added to Histogram::SerializeInfoImpl

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc#old...
base/metrics/histogram.cc:263: DCHECK_EQ(flags & render_histogram->flags(),
flags);
Actually I really don't understand what's this DCHECK is for... Could you
enlighten me?

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:64: // in this process. So need to clear IPC flag
first.
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> nit: "in this process. So need to clear IPC flag" -> "in this process, so we
> need to clear the IPC flag"

Done.

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:71: bool ValidateRangeChecksum(const HistogramBase*
histogram,
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> nit: Please pass by const-reference.

Done.

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:325: pickle->WriteInt(flags()) &&
histogram_name and flags are fields from HistogramBase

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:332: return false;
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> Optional nit: IMO it's tidier to write this as
> 
> return pickle->WriteString(histogram_name()) && ...

Done.

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:775: }
I think the good thing of current code is it distributes the
serializing/deserializing code. (A good thing about polymorphism: limit
switch/case :) Or the function you suggested will look exactly like the old
Histogram::DeserializeHistogramInfo().

Please note I intentionly kept most of the serializing logic. E.g. declared_min,
declared_max are not needed in Boolean/CustomHistogram. The old big
Histogram::Deserialize function resulted in current situation that many
unnecessary fields get written and read.

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.h#newc...
base/metrics/histogram.h:496: PickleIterator* iter);
I need a function to know all kind of histograms to deserialize (like the old
Histogram::Deserialize...).

Another option is a global function DeserializeHistogramInfo. Which do you
prefer?

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram.h#newc...
base/metrics/histogram.h:582: };
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> nit: Please move this to be above the comment that's describing
> FactoryGetWithRangeDescription.

Done.

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_base.h
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_base.h...
base/metrics/histogram_base.h:92: // serialize the samples.
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> nit: "Note: This only serializes the construction parameters for the
histogram;
> it does not serialize the samples."

Done.

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_base_u...
File base/metrics/histogram_base_unittest.cc (right):

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_base_u...
base/metrics/histogram_base_unittest.cc:18: virtual void SetUp() {
changed to constructor

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_base_u...
base/metrics/histogram_base_unittest.cc:23: }
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> nit: Why not do this in the constructor?

Done.

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_base_u...
base/metrics/histogram_base_unittest.cc:25: virtual void TearDown() {
changed to destructor

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_base_u...
base/metrics/histogram_base_unittest.cc:35: StatisticsRecorder*
statistics_recorder_;
StatisticsRecorder constructor is private...

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_unitte...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/11682003/diff/1/base/metrics/histogram_unitte...
base/metrics/histogram_unittest.cc:429: EXPECT_EQ(0, range);
oops, didn't unload the newest code

https://codereview.chromium.org/11682003/diff/1/base/metrics/sparse_histogram.h
File base/metrics/sparse_histogram.h (left):

https://codereview.chromium.org/11682003/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.h:53: HistogramBase::Count redundant_count_;
redundant_count_ is just a duplicated count to detect memory corruption. It's
already included in SampleMap.

https://codereview.chromium.org/11682003/diff/1/base/metrics/sparse_histogram.h
File base/metrics/sparse_histogram.h (right):

https://codereview.chromium.org/11682003/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.h:61: // Protects access to |sample_counts_| and
|redundant_count_|.
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

https://codereview.chromium.org/11682003/diff/1/base/metrics/sparse_histogram...
File base/metrics/sparse_histogram_unittest.cc (right):

https://codereview.chromium.org/11682003/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram_unittest.cc:65: }
All deserializing tests are in histogram_base_unittests.cc (because the
deserialize function is there).
I added a TODO. Because currently SparseHistogram is not supported by
StatisticsRecorder, it's hard to test.

https://codereview.chromium.org/11682003/diff/1/chrome/common/metrics/metrics...
File chrome/common/metrics/metrics_service_base.cc (right):

https://codereview.chromium.org/11682003/diff/1/chrome/common/metrics/metrics...
chrome/common/metrics/metrics_service_base.cc:41:
log_manager_.current_log()->RecordHistogramDelta(histogram.histogram_name(),
In this CL, I changed most of the serializing related code to use HistogramBase
instead of Histogram.
For RecordHistogramDelta, only name is used, so I directly changed it to only
depend on name.

https://codereview.chromium.org/11682003/diff/1/content/browser/histogram_syn...
File content/browser/histogram_synchronizer.cc (right):

https://codereview.chromium.org/11682003/diff/1/content/browser/histogram_syn...
content/browser/histogram_synchronizer.cc:280: if (histogram) {
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> Optional nit: IMO it's tidier to write "if (!histogram) continue" to reduce
the
> level of indentation.

Done.

https://codereview.chromium.org/11682003/diff/1/content/browser/histogram_syn...
content/browser/histogram_synchronizer.cc:286:
histogram->AddSamplesFromPickle(&iter);
IMO
1. histogram is in base/, so it should know nothing about inter process. 
2. We are actually not "deserializing" the histogram with samples, but find the
histogram and then add the diff. So it's a bit odd to put them into a same
function.

https://codereview.chromium.org/11682003/diff/1/content/common/child_histogra...
File content/common/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/11682003/diff/1/content/common/child_histogra...
content/common/child_histogram_message_filter.cc:63: // histograms.
On 2012/12/29 00:17:30, Ilya Sherman wrote:
> nit: "Note." -> "Note:"; "to all" -> "for all the"; "histograms. So the" ->
> "histograms, so that the"; "distinguish them with" -> "distinguish them from".

Done.

[Ilya Sherman, 2013-01-08 22:31:53]

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:263: DCHECK_EQ(flags & render_histogram->flags(),
flags);
On 2013/01/08 00:51:40, kaiwang wrote:
> Actually I really don't understand what's this DCHECK is for... Could you
> enlighten me?

I'm also not sure; perhaps Jim can comment?  I think there are two
possibilities: (1) the code has evolved so that this is no longer needed, or (2)
this is checking something subtle, that we should continue to check.

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.cc:775: }
On 2013/01/08 00:51:40, kaiwang wrote:
> I think the good thing of current code is it distributes the
> serializing/deserializing code. (A good thing about polymorphism: limit
> switch/case :) Or the function you suggested will look exactly like the old
> Histogram::DeserializeHistogramInfo().

I'm not convinced that this is true: HistogramBase::DeserializeHistogramInfo()
still has a switch stmt in it.  IMO it would be better to just inline all of the
shared code into that function (or a helper function that that one calls),
rather than repeating it in so many places.

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram.h:496: PickleIterator* iter);
On 2013/01/08 00:51:40, kaiwang wrote:
> I need a function to know all kind of histograms to deserialize (like the old
> Histogram::Deserialize...).
> 
> Another option is a global function DeserializeHistogramInfo. Which do you
> prefer?

If you keep roughly the current design, I'd prefer to make the deserialization
functions public.  HistogramBase should not know about private portions of these
classes.

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
File base/metrics/histogram_base_unittest.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/base/metrics/histogram...
base/metrics/histogram_base_unittest.cc:35: StatisticsRecorder*
statistics_recorder_;
On 2013/01/08 00:51:40, kaiwang wrote:
> StatisticsRecorder constructor is private...

Yuck.  Ok.

https://chromiumcodereview.appspot.com/11682003/diff/1/chrome/common/metrics/...
File chrome/common/metrics/metrics_service_base.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/chrome/common/metrics/...
chrome/common/metrics/metrics_service_base.cc:41:
log_manager_.current_log()->RecordHistogramDelta(histogram.histogram_name(),
On 2013/01/08 00:51:40, kaiwang wrote:
> In this CL, I changed most of the serializing related code to use
HistogramBase
> instead of Histogram.
> For RecordHistogramDelta, only name is used, so I directly changed it to only
> depend on name.

Ok, fair enough :)

https://chromiumcodereview.appspot.com/11682003/diff/1/content/browser/histog...
File content/browser/histogram_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/1/content/browser/histog...
content/browser/histogram_synchronizer.cc:286:
histogram->AddSamplesFromPickle(&iter);
On 2013/01/08 00:51:40, kaiwang wrote:
> IMO
> 1. histogram is in base/, so it should know nothing about inter process.

But it already does know about inter-process communication -- the
kIPCSerializationSourceFlag is in the base namespace...

> 2. We are actually not "deserializing" the histogram with samples, but find
the
> histogram and then add the diff. So it's a bit odd to put them into a same
> function.

It just seems a fragile to rely on the fact that DeserializeHistogramInfo()
leaves the iterator in just the right place for you to be able to pick it up and
continue reading from it here.  If the two functions did not read from the same
"stream", I would be less concerned about their separation.

[Ilya Sherman, 2013-01-09 05:48:37]

LGTM with nits:

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram.cc:64: // in this process. So we need to clear the IPC
flag.
nit: "process. So" -> "process, so"

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram.h:497: static HistogramBase*
DeserializeInfoImpl(PickleIterator* iter);
IMO this should be public, rather than private+friend.

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
File base/metrics/histogram_base.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram_base.h:41: HistogramBase*
DeserializeHistogramInfo(PickleIterator* iter);
nit: Please tuck this into an anonymous namespace in the implementation file.

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram_base.h:44: bool
DeserializeHistogramAndAddSamples(PickleIterator* iter);
nit: Why does this function need a return value?  histogram_synchronizer doesn't
seem to make use of the value, and I'm not 100% sure what true vs. false
means...

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram_base.h:95: // Note. This only serializes the construction
arguments of the histogram, but
nit: "Note." -> "Note:"; "but not" -> "but does not"

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/spars...
File base/metrics/sparse_histogram.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/spars...
base/metrics/sparse_histogram.h:60: mutable base::Lock lock_;  // Protects
access to |samples_|.
nit: It's more typical for the comment for a member variable to be on the
previous line rather than on the same line.

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/browser/hi...
File content/browser/histogram_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/browser/hi...
content/browser/histogram_synchronizer.cc:278: 
nit: No need for this newline

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/common/chi...
File content/common/child_histogram_message_filter.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/common/chi...
content/common/child_histogram_message_filter.cc:62: // the histograms. So the
receiving process can distinguish them with the
nit: "distinguish them with" -> "distinguish them from"

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/common/chi...
content/common/child_histogram_message_filter.cc:62: // the histograms. So the
receiving process can distinguish them with the
nit: "histograms. So" -> "histograms, so that"

[kaiwang, 2013-01-10 23:02:24]

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram.cc:64: // in this process. So we need to clear the IPC
flag.
On 2013/01/09 05:48:37, Ilya Sherman wrote:
> nit: "process. So" -> "process, so"

Done.

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram.h:497: static HistogramBase*
DeserializeInfoImpl(PickleIterator* iter);
Note it only deserialize part of the info. type field is deserialized with
DeserializeHistogramInfo.
So no client code should directly call this function to deserialize a histogram

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
File base/metrics/histogram_base.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram_base.h:41: HistogramBase*
DeserializeHistogramInfo(PickleIterator* iter);
It's used by unittest.
Although this is not directly used by client, I think it at least makes the
histogram more feature complete... Don't see the harm putting it here...

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram_base.h:44: bool
DeserializeHistogramAndAddSamples(PickleIterator* iter);
On 2013/01/09 05:48:37, Ilya Sherman wrote:
> nit: Why does this function need a return value?  histogram_synchronizer
doesn't
> seem to make use of the value, and I'm not 100% sure what true vs. false
> means...

Done.

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/histo...
base/metrics/histogram_base.h:95: // Note. This only serializes the construction
arguments of the histogram, but
On 2013/01/09 05:48:37, Ilya Sherman wrote:
> nit: "Note." -> "Note:"; "but not" -> "but does not"

Done.

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/spars...
File base/metrics/sparse_histogram.h (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/base/metrics/spars...
base/metrics/sparse_histogram.h:60: mutable base::Lock lock_;  // Protects
access to |samples_|.
On 2013/01/09 05:48:37, Ilya Sherman wrote:
> nit: It's more typical for the comment for a member variable to be on the
> previous line rather than on the same line.

Done.

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/browser/hi...
File content/browser/histogram_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/browser/hi...
content/browser/histogram_synchronizer.cc:278: 
On 2013/01/09 05:48:37, Ilya Sherman wrote:
> nit: No need for this newline

Done.

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/common/chi...
File content/common/child_histogram_message_filter.cc (right):

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/common/chi...
content/common/child_histogram_message_filter.cc:62: // the histograms. So the
receiving process can distinguish them with the
On 2013/01/09 05:48:37, Ilya Sherman wrote:
> nit: "distinguish them with" -> "distinguish them from"

Done.

https://chromiumcodereview.appspot.com/11682003/diff/33001/content/common/chi...
content/common/child_histogram_message_filter.cc:62: // the histograms. So the
receiving process can distinguish them with the
On 2013/01/09 05:48:37, Ilya Sherman wrote:
> nit: "histograms. So" -> "histograms, so that"

Done.

[kaiwang, 2013-01-10 23:05:27]

Reviewers, I need your OWNER review:

jar: base and chrome/common/metrics
jam: content

Thanks a lot!

[jam, 2013-01-11 01:16:27]

On 2013/01/10 23:05:27, kaiwang wrote:
> Reviewers, I need your OWNER review:
> 
> jar: base and chrome/common/metrics
> jam: content
> 
> Thanks a lot!

owners lgtm for content, i defer to people actually familiar with this code for
review

[jar (doing other things), 2013-01-11 05:52:13]

Based on Isherman's approval, base LGTM.

[commit-bot: I haz the power, 2013-01-11 06:45:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/11682003/37002

[commit-bot: I haz the power, 2013-01-11 07:03:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-11 18:58:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/11682003/56001

[commit-bot: I haz the power, 2013-01-11 21:52:49]

Change committed as 176449