Consolidate serialization code in base::HistogramDeltasSerializer.

base/metrics/histogram_delta_serializer.h

+// Copyright 2013 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.
+
+#ifndef BASE_METRICS_HISTOGRAM_DELTA_SERIALIZER_BASE_H_
+#define BASE_METRICS_HISTOGRAM_DELTA_SERIALIZER_BASE_H_
+
+#include <string>
+#include <vector>
+
+#include "base/base_export.h"
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/metrics/histogram_flattener.h"
+
+namespace base {
+
+class HistogramSnapshotManager;
+
+// Serializes and restores histograms deltas.
+class BASE_EXPORT HistogramDeltasSerializer : private HistogramFlattener {

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

Why does this class use private inheritance?  That's usually a good tip that
you're doing something you probably oughtn't to.

[Vitaly Buka (NO REVIEWS), 2013/10/17 10:15:47]

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:

[Ilya Sherman, 2013/10/17 22:41:46]

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

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

Thanks.

On 2013/10/17 22:41:46, Ilya Sherman wrote:

+ public:
+  typedef std::vector<std::string> SerializedDeltas;

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

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.

[Vitaly Buka (NO REVIEWS), 2013/10/17 10:15:47]

Done.

+
+  // |caller_name| is string used in histograms for counting inconsistencies.
+  explicit HistogramDeltasSerializer(const std::string& caller_name);
+

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

nit: It's typical to omit this blank line between the constructor and the
destructor, I think.

[Vitaly Buka (NO REVIEWS), 2013/10/17 10:15:47]

Done.

+  virtual ~HistogramDeltasSerializer();
+
+  // Returns vector of strings with serialized histograms deltas changed a
+  // previous call.

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

nit: Missing word from this comment?

[Vitaly Buka (NO REVIEWS), 2013/10/17 10:15:47]

Done.

+  const SerializedDeltas& GetNewDeltas();
+
+  // Deserialize deltas and add samples to corresponding histograms.

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

nit: "to corresponding histograms" -> "to corresponding histograms, creating
them if necessary".

[Vitaly Buka (NO REVIEWS), 2013/10/17 10:15:47]

Done.

+  // Silently ignores errors in |deltas|.
+  static void DeserializeAndAddSamples(const SerializedDeltas& deltas);
+
+ private:
+  // HistogramFlattener implementation.
+  virtual void RecordDelta(const HistogramBase& histogram,
+                           const HistogramSamples& snapshot) OVERRIDE;
+  virtual void InconsistencyDetected(
+      HistogramBase::Inconsistency problem) OVERRIDE;
+  virtual void UniqueInconsistencyDetected(
+      HistogramBase::Inconsistency problem) OVERRIDE;
+  virtual void InconsistencyDetectedInLoggedCount(int amount) OVERRIDE;
+
+  std::string caller_name_;
+  scoped_ptr<HistogramSnapshotManager> histogram_snapshot_manager_;
+  SerializedDeltas histograms_;

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

nit: serialized_deltas_ would probably be a clearer name for this variable.

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

nit: Please document all of these variables.

[Vitaly Buka (NO REVIEWS), 2013/10/17 10:15:47]

Done.

[Vitaly Buka (NO REVIEWS), 2013/10/17 10:15:47]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(HistogramDeltasSerializer);
+};
+
+}  // namespace base
+
+#endif  // BASE_METRICS_HISTOGRAM_DELTA_SERIALIZER_BASE_H_