SampleSet -> HistogramSamples (will be reused by SparseHistogram)

net/disk_cache/stats_histogram.h

 // Copyright (c) 2012 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 NET_DISK_CACHE_STATS_HISTOGRAM_H_
 #define NET_DISK_CACHE_STATS_HISTOGRAM_H_
 
 #include <string>
 
+#include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 
+namespace base {
+class BucketRanges;
+class SampleVector;
+class HistogramSamples;
+};

[Ilya Sherman, 2012/09/12 03:20:58]

nit: No need for this semicolon

[kaiwang, 2012/09/20 22:54:59]

Done.

+
 namespace disk_cache {
 
 class Stats;
 
 // This class provides support for sending the disk cache size stats as a UMA
 // histogram. We'll provide our own storage and management for the data, and a
 // SampleSet with a copy of our data.
 //
 // Class derivation of Histogram "deprecated," and should not be copied, and
 // may eventually go away.

[Ilya Sherman, 2012/09/12 03:20:58]

Any chance of implementing this TODO and migrating away from deriving from
Histogram instead?

[kaiwang, 2012/09/20 22:54:59]

Still have a more change needed. Feel free to file a CL and address this

 //
 class StatsHistogram : public base::Histogram {
  public:
-  class StatsSamples : public SampleSet {
-   public:
-    Counts* GetCounts() {
-      return &counts_;
-    }
-  };
-
   StatsHistogram(const std::string& name,
                  Sample minimum,
                  Sample maximum,
-                 size_t bucket_count)
-      : Histogram(name, minimum, maximum, bucket_count, NULL), init_(false) {}
+                 size_t bucket_count,
+                 base::BucketRanges* ranges,
+                 const Stats* stats);

[Ilya Sherman, 2012/09/12 03:20:58]

Can this have private visibility, since clients are seemingly expected to use
the FactoryGet() factory instead?

[Ilya Sherman, 2012/09/21 08:52:12]

Ping.

   virtual ~StatsHistogram();
 
-  static StatsHistogram* FactoryGet(const std::string& name);
+  static void InitializeBucketRanges(const Stats* stats,
+                                     base::BucketRanges* ranges);

[Ilya Sherman, 2012/09/12 03:20:58]

nit: Can this be tucked into an anonymous namespace in the implementation file?

[Ilya Sherman, 2012/09/21 08:52:12]

Ping.

+  static StatsHistogram* FactoryGet(const std::string& name,
+                                    const Stats* stats);
 
-  // We'll be reporting data from the given set of cache stats.
-  bool Init(const Stats* stats);
-
-  virtual Sample ranges(size_t i) const OVERRIDE;
-  virtual size_t bucket_count() const OVERRIDE;
-  virtual void SnapshotSample(SampleSet* sample) const OVERRIDE;
+  virtual scoped_ptr<base::SampleVector> SnapshotSamples() const OVERRIDE;
   virtual Inconsistencies FindCorruption(
-      const SampleSet& snapshot) const OVERRIDE;
+      const base::HistogramSamples& samples) const OVERRIDE;
 
  private:
-  bool init_;
-  static const Stats* stats_;
+  const Stats* stats_;

[Ilya Sherman, 2012/09/12 03:20:58]

nit: const Stats* const stats_?

[Ilya Sherman, 2012/09/21 08:52:12]

Ping.

[kaiwang, 2012/09/21 19:51:18]

For above 3 comments, I tried to follow the original code (public and const),
and code in base/metricts(Initialize). I'm trying to limit my change only to
SampleSet -> HistogramSamples, so it doesnot make sense to change more in this
CL.
Current OWNER looked good. If you feel these code is a problem, feel free to
file another CL.

   DISALLOW_COPY_AND_ASSIGN(StatsHistogram);
 };
 
 }  // namespace disk_cache
 
 #endif  // NET_DISK_CACHE_STATS_HISTOGRAM_H_