Consolidate serialization code in base::HistogramDeltasSerializer.

Consolidate serialization code in base::HistogramDeltasSerializer
Before patch code lived partially in base/ and content/

BUG=305019
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230268

[Vitaly Buka (NO REVIEWS), 2013-10-16 19:01:46]

Hi Ilya, please review my changes.
Thank you in advance.

[Ilya Sherman, 2013-10-17 04:37:42]

Thanks!

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer.cc (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.cc:18: void
DeserializeHistogramAndAddSamples(PickleIterator* iter) {
nit: Please add documentation for this function.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.cc:36: histogram_snapshot_manager_(new
HistogramSnapshotManager(this)) {
nit: Why heap-allocate this, rather than including it as a direct member
variable?

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.cc:79: caller_name_.c_str()),
Why StringPrintf rather than just using the concatenation operator, i.e. """
"Histogram.Inconsistencies" + caller_name_ """?

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.cc:79: caller_name_.c_str()),
UMA_HISTOGRAM_ENUMERATION requires the histogram name to be a runtime constant. 
You'll need to expand out the macro if the name can vary, as it seems to here.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer.h (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:21: class BASE_EXPORT
HistogramDeltasSerializer : private HistogramFlattener {
Why does this class use private inheritance?  That's usually a good tip that
you're doing something you probably oughtn't to.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:23: typedef std::vector<std::string>
SerializedDeltas;
nit: I'd prefer that you spell out the full typename, rather than using a
typedef.  The typedef doesn't save you very many characters, but it does
obfuscate the code a bit.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:27: 
nit: It's typical to omit this blank line between the constructor and the
destructor, I think.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:31: // previous call.
nit: Missing word from this comment?

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:34: // Deserialize deltas and add
samples to corresponding histograms.
nit: "to corresponding histograms" -> "to corresponding histograms, creating
them if necessary".

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:50: SerializedDeltas histograms_;
nit: serialized_deltas_ would probably be a clearer name for this variable.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:50: SerializedDeltas histograms_;
nit: Please document all of these variables.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer_unittest.cc (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer_unittest.cc:21:
serializer.GetNewDeltas();
I don't follow what the purpose of this line is...

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer_unittest.cc:36: // The histogram has
kIPCSerializationSourceFlag. So samples will be ignored.
Did you mean to deserialize the histograms at some point before this part of the
test?

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
File content/child/child_histogram_message_filter.cc (left):

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
content/child/child_histogram_message_filter.cc:59: // Push snapshots into our
pickled_histograms_ vector.
nit: What happened to this line of the comment?

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
content/child/child_histogram_message_filter.cc:58: }
Why lazy-initialize this rather than including it as a direct class member
variable?

[Vitaly Buka (NO REVIEWS), 2013-10-17 10:15:46]

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer.cc (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.cc:18: void
DeserializeHistogramAndAddSamples(PickleIterator* iter) {
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: Please add documentation for this function.

Done. Recovered old doc string.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.cc:36: histogram_snapshot_manager_(new
HistogramSnapshotManager(this)) {
Recently I was asked often to do so. I had impression we are encouraged to do so
to avoid includes. But after re-reading
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts I see that
it's not true.
 
Done.

On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: Why heap-allocate this, rather than including it as a direct member
> variable?

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.cc:79: caller_name_.c_str()),
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> Why StringPrintf rather than just using the concatenation operator, i.e. """
> "Histogram.Inconsistencies" + caller_name_ """?

Done.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.cc:79: caller_name_.c_str()),
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> UMA_HISTOGRAM_ENUMERATION requires the histogram name to be a runtime
constant. 
> You'll need to expand out the macro if the name can vary, as it seems to here.

Done.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer.h (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:21: class BASE_EXPORT
HistogramDeltasSerializer : private HistogramFlattener {
There is nothing wrong with private inheritance. But here is OK to keep public.
Done.

On 2013/10/17 04:37:42, Ilya Sherman wrote:
> Why does this class use private inheritance?  That's usually a good tip that
> you're doing something you probably oughtn't to.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:23: typedef std::vector<std::string>
SerializedDeltas;
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: I'd prefer that you spell out the full typename, rather than using a
> typedef.  The typedef doesn't save you very many characters, but it does
> obfuscate the code a bit.

Done.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:27: 
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: It's typical to omit this blank line between the constructor and the
> destructor, I think.

Done.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:31: // previous call.
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: Missing word from this comment?

Done.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:34: // Deserialize deltas and add
samples to corresponding histograms.
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: "to corresponding histograms" -> "to corresponding histograms, creating
> them if necessary".

Done.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:50: SerializedDeltas histograms_;
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: serialized_deltas_ would probably be a clearer name for this variable.

Done.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:50: SerializedDeltas histograms_;
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: Please document all of these variables.

Done.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer_unittest.cc (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer_unittest.cc:21:
serializer.GetNewDeltas();
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> I don't follow what the purpose of this line is...

Done. It's unnecessary.

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer_unittest.cc:36: // The histogram has
kIPCSerializationSourceFlag. So samples will be ignored.
No. I just ported this test from prev implementation.
But I believe here author just checks values the where set in lines 28-31
We will de-serialize later and check at lines 38-41

On 2013/10/17 04:37:42, Ilya Sherman wrote:
> Did you mean to deserialize the histograms at some point before this part of
the
> test?

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
File content/child/child_histogram_message_filter.cc (left):

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
content/child/child_histogram_message_filter.cc:59: // Push snapshots into our
pickled_histograms_ vector.
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> nit: What happened to this line of the comment?

I guess with current implementation we don't need to know here that it's
"pickled_histograms_ vector".

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
content/child/child_histogram_message_filter.cc:58: }
On 2013/10/17 04:37:42, Ilya Sherman wrote:
> Why lazy-initialize this rather than including it as a direct class member
> variable?

Here, in addition to includes optimization, we can avoid initialization if
metrics server is not enabled.

[Vitaly Buka (NO REVIEWS), 2013-10-17 10:22:33]

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer_unittest.cc (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer_unittest.cc:36: // The histogram has
kIPCSerializationSourceFlag. So samples will be ignored.
Let me fix my comment.
I guess author just checks here that histogram is the same as it was set few
lines above.
Later we de-serialize and check that counters has double value.


On 2013/10/17 10:15:47, Vitaly Buka wrote:
> No. I just ported this test from prev implementation.
> But I believe here author just checks values the where set in lines 28-31
> We will de-serialize later and check at lines 38-41
> 
> On 2013/10/17 04:37:42, Ilya Sherman wrote:
> > Did you mean to deserialize the histograms at some point before this part of
> the
> > test?
>

[Ilya Sherman, 2013-10-17 22:41:45]

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer.h (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:21: class BASE_EXPORT
HistogramDeltasSerializer : private HistogramFlattener {
On 2013/10/17 10:15:47, Vitaly Buka wrote:
> There is nothing wrong with private inheritance. But here is OK to keep
public.
> Done.

FYI, from the style guide: "Composition is often more appropriate than
inheritance. When using inheritance, make it public."

> On 2013/10/17 04:37:42, Ilya Sherman wrote:
> > Why does this class use private inheritance?  That's usually a good tip that
> > you're doing something you probably oughtn't to.
>

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer_unittest.cc (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer_unittest.cc:36: // The histogram has
kIPCSerializationSourceFlag. So samples will be ignored.
I think you're overlooking line 77 of the original test.

On 2013/10/17 10:22:33, Vitaly Buka wrote:
> Let me fix my comment.
> I guess author just checks here that histogram is the same as it was set few
> lines above.
> Later we de-serialize and check that counters has double value.
> 
> 
> On 2013/10/17 10:15:47, Vitaly Buka wrote:
> > No. I just ported this test from prev implementation.
> > But I believe here author just checks values the where set in lines 28-31
> > We will de-serialize later and check at lines 38-41
> > 
> > On 2013/10/17 04:37:42, Ilya Sherman wrote:
> > > Did you mean to deserialize the histograms at some point before this part
of
> > the
> > > test?
> > 
>

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
File content/child/child_histogram_message_filter.cc (left):

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
content/child/child_histogram_message_filter.cc:59: // Push snapshots into our
pickled_histograms_ vector.
On 2013/10/17 10:15:47, Vitaly Buka wrote:
> On 2013/10/17 04:37:42, Ilya Sherman wrote:
> > nit: What happened to this line of the comment?
> 
> I guess with current implementation we don't need to know here that it's
> "pickled_histograms_ vector".

Well, the vector is renamed, but IMO it's still useful to document that calling
PrepareDeltas() has a side-effect of calling into RecordDelta() which updates
the serialized_deltas_ vector.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
File base/metrics/histogram_delta_serializer.cc (right):

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.cc:39:
inconsistent_stapshot_histogram_(NULL) {
nit: No need to explicitly initialize these histograms to NULL, since you
immediately reset their values below.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.cc:78: Pickle pickle(it->data(),
static_cast<int>(it->size()));
nit: Please use checked_numeric_cast here.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
File base/metrics/histogram_delta_serializer.h (right):

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.h:29: // call.
nit: I'd suggest a wording more like: "Computes deltas in histogram bucket
counts relative to the previous call to this method.  Returns the deltas in
serialized form."  Feel free to improve my wording further, of course.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.h:49: // Output buffet for serialized
deltas.
nit: "buffet" -> "buffer"

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.h:55: HistogramBase*
inconsistent_stapshot_histogram_;
nit: "stapshot" -> "snapshot"

[Vitaly Buka (NO REVIEWS), 2013-10-18 05:33:33]

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer.h (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer.h:21: class BASE_EXPORT
HistogramDeltasSerializer : private HistogramFlattener {
Thanks.

On 2013/10/17 22:41:46, Ilya Sherman wrote:
> On 2013/10/17 10:15:47, Vitaly Buka wrote:
> > There is nothing wrong with private inheritance. But here is OK to keep
> public.
> > Done.
> 
> FYI, from the style guide: "Composition is often more appropriate than
> inheritance. When using inheritance, make it public."
> 
> > On 2013/10/17 04:37:42, Ilya Sherman wrote:
> > > Why does this class use private inheritance?  That's usually a good tip
that
> > > you're doing something you probably oughtn't to.
> > 
>

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
File base/metrics/histogram_delta_serializer_unittest.cc (right):

https://codereview.chromium.org/27460003/diff/2001/base/metrics/histogram_del...
base/metrics/histogram_delta_serializer_unittest.cc:36: // The histogram has
kIPCSerializationSourceFlag. So samples will be ignored.
Thanks. My understanding of test was incorrect. 


On 2013/10/17 22:41:46, Ilya Sherman wrote:
> I think you're overlooking line 77 of the original test.
> 
> On 2013/10/17 10:22:33, Vitaly Buka wrote:
> > Let me fix my comment.
> > I guess author just checks here that histogram is the same as it was set few
> > lines above.
> > Later we de-serialize and check that counters has double value.
> > 
> > 
> > On 2013/10/17 10:15:47, Vitaly Buka wrote:
> > > No. I just ported this test from prev implementation.
> > > But I believe here author just checks values the where set in lines 28-31
> > > We will de-serialize later and check at lines 38-41
> > > 
> > > On 2013/10/17 04:37:42, Ilya Sherman wrote:
> > > > Did you mean to deserialize the histograms at some point before this
part
> of
> > > the
> > > > test?
> > > 
> > 
>

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
File content/child/child_histogram_message_filter.cc (left):

https://codereview.chromium.org/27460003/diff/2001/content/child/child_histog...
content/child/child_histogram_message_filter.cc:59: // Push snapshots into our
pickled_histograms_ vector.
What if just change interface to make this more clear.

On 2013/10/17 22:41:46, Ilya Sherman wrote:
> On 2013/10/17 10:15:47, Vitaly Buka wrote:
> > On 2013/10/17 04:37:42, Ilya Sherman wrote:
> > > nit: What happened to this line of the comment?
> > 
> > I guess with current implementation we don't need to know here that it's
> > "pickled_histograms_ vector".
> 
> Well, the vector is renamed, but IMO it's still useful to document that
calling
> PrepareDeltas() has a side-effect of calling into RecordDelta() which updates
> the serialized_deltas_ vector.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
File base/metrics/histogram_delta_serializer.cc (right):

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.cc:39:
inconsistent_stapshot_histogram_(NULL) {
On 2013/10/17 22:41:47, Ilya Sherman wrote:
> nit: No need to explicitly initialize these histograms to NULL, since you
> immediately reset their values below.

Done.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.cc:78: Pickle pickle(it->data(),
static_cast<int>(it->size()));
On 2013/10/17 22:41:47, Ilya Sherman wrote:
> nit: Please use checked_numeric_cast here.

Done.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
File base/metrics/histogram_delta_serializer.h (right):

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.h:29: // call.
On 2013/10/17 22:41:47, Ilya Sherman wrote:
> nit: I'd suggest a wording more like: "Computes deltas in histogram bucket
> counts relative to the previous call to this method.  Returns the deltas in
> serialized form."  Feel free to improve my wording further, of course.

Done.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.h:49: // Output buffet for serialized
deltas.
On 2013/10/17 22:41:47, Ilya Sherman wrote:
> nit: "buffet" -> "buffer"

Done.

https://codereview.chromium.org/27460003/diff/21001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.h:55: HistogramBase*
inconsistent_stapshot_histogram_;
On 2013/10/17 22:41:47, Ilya Sherman wrote:
> nit: "stapshot" -> "snapshot"

Done.

[Vitaly Buka (NO REVIEWS), 2013-10-18 17:27:40]

+avi for content

[Vitaly Buka (NO REVIEWS), 2013-10-18 17:31:59]

+darin

[Ilya Sherman, 2013-10-19 00:27:43]

LGTM, thanks.

https://codereview.chromium.org/27460003/diff/36001/base/metrics/histogram_de...
File base/metrics/histogram_delta_serializer.cc (right):

https://codereview.chromium.org/27460003/diff/36001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.cc:93: }
nit: De-indent this line.

[Vitaly Buka (NO REVIEWS), 2013-10-21 19:13:54]

Thanks for review!

https://codereview.chromium.org/27460003/diff/36001/base/metrics/histogram_de...
File base/metrics/histogram_delta_serializer.cc (right):

https://codereview.chromium.org/27460003/diff/36001/base/metrics/histogram_de...
base/metrics/histogram_delta_serializer.cc:93: }
On 2013/10/19 00:27:44, Ilya Sherman wrote:
> nit: De-indent this line.

Done.

[darin (slow to review), 2013-10-21 21:59:57]

https://codereview.chromium.org/27460003/diff/186001/base/metrics/histogram_d...
File base/metrics/histogram_delta_serializer.cc (right):

https://codereview.chromium.org/27460003/diff/186001/base/metrics/histogram_d...
base/metrics/histogram_delta_serializer.cc:33: }
nit: "}  // namespace"

https://codereview.chromium.org/27460003/diff/186001/base/metrics/histogram_d...
File base/metrics/histogram_delta_serializer.h (right):

https://codereview.chromium.org/27460003/diff/186001/base/metrics/histogram_d...
base/metrics/histogram_delta_serializer.h:5: #ifndef
BASE_METRICS_HISTOGRAM_DELTA_SERIALIZER_BASE_H_
nit: include guard is wrong

https://codereview.chromium.org/27460003/diff/186001/base/metrics/histogram_d...
base/metrics/histogram_delta_serializer.h:22: class BASE_EXPORT
HistogramDeltasSerializer : public HistogramFlattener {
nit: the file name and class name do not precisely match. "Delta[s]"

https://codereview.chromium.org/27460003/diff/186001/base/metrics/histogram_d...
base/metrics/histogram_delta_serializer.h:36: static void
DeserializeAndAddSamples(const std::vector<std::string>& deltas);
nit: Is this class really a Serializer if it provides a method to deserialize as
well? Maybe HistogramDeltaSerialization would be a better name?

nit: Use "serialized_deltas" or "deltas" for the variable name in both
functions? Be consistent.

[Vitaly Buka (NO REVIEWS), 2013-10-21 23:05:32]

https://chromiumcodereview.appspot.com/27460003/diff/186001/base/metrics/hist...
File base/metrics/histogram_delta_serializer.cc (right):

https://chromiumcodereview.appspot.com/27460003/diff/186001/base/metrics/hist...
base/metrics/histogram_delta_serializer.cc:33: }
On 2013/10/21 21:59:58, darin wrote:
> nit: "}  // namespace"

Done.

https://chromiumcodereview.appspot.com/27460003/diff/186001/base/metrics/hist...
File base/metrics/histogram_delta_serializer.h (right):

https://chromiumcodereview.appspot.com/27460003/diff/186001/base/metrics/hist...
base/metrics/histogram_delta_serializer.h:5: #ifndef
BASE_METRICS_HISTOGRAM_DELTA_SERIALIZER_BASE_H_
On 2013/10/21 21:59:58, darin wrote:
> nit: include guard is wrong

Done.

https://chromiumcodereview.appspot.com/27460003/diff/186001/base/metrics/hist...
base/metrics/histogram_delta_serializer.h:22: class BASE_EXPORT
HistogramDeltasSerializer : public HistogramFlattener {
On 2013/10/21 21:59:58, darin wrote:
> nit: the file name and class name do not precisely match. "Delta[s]"

Done.

https://chromiumcodereview.appspot.com/27460003/diff/186001/base/metrics/hist...
base/metrics/histogram_delta_serializer.h:36: static void
DeserializeAndAddSamples(const std::vector<std::string>& deltas);
On 2013/10/21 21:59:58, darin wrote:
> nit: Is this class really a Serializer if it provides a method to deserialize
as
> well? Maybe HistogramDeltaSerialization would be a better name?
> 
> nit: Use "serialized_deltas" or "deltas" for the variable name in both
> functions? Be consistent.

Done.

[darin (slow to review), 2013-10-22 22:31:28]

LGTM

[commit-bot: I haz the power, 2013-10-22 22:35:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/27460003/326001

[commit-bot: I haz the power, 2013-10-23 01:16:07]

Change committed as 230268