Skeleton code of SparseHistogram

base/metrics/sparse_histogram.cc

Index: base/metrics/sparse_histogram.cc
===================================================================
--- base/metrics/sparse_histogram.cc	(revision 0)
+++ base/metrics/sparse_histogram.cc	(revision 0)
@@ -0,0 +1,45 @@
+// 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.
+
+#include "base/metrics/sparse_histogram.h"
+
+#include "base/metrics/statistics_recorder.h"
+#include "base/synchronization/lock.h"
+
+using std::string;

[Ilya Sherman, 2012/08/04 01:18:35]

nit: This sort of using statement is not permitted by the Chromium style guide.

[kaiwang, 2012/08/08 03:59:33]

are you sure about this?
This is allowed in google style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

I didn't see it forbidden in chrome style guide..

[Ilya Sherman, 2012/08/08 05:00:29]

Sorry, you're right, this sort of using statement is allowed -- I read the "std"
part and mentally translated it into the more usual "using namespace std". 
However, I think it's generally discouraged to use the using directive for
members of the std:: namespace.  At least, I haven't seen it done elsewhere.

+
+namespace base {
+
+// static
+HistogramBase* SparseHistogram::FactoryGet(const string& name,
+                                           Flags flags) {

[Ilya Sherman, 2012/08/04 01:18:35]

nit: Should this be |int32 flags|, as it is everywhere else in this CL?

[kaiwang, 2012/08/08 03:59:33]

Done. Good catch 

+  HistogramBase* histogram = new SparseHistogram(name);

[Ilya Sherman, 2012/08/04 01:18:35]

nit: I believe this is an intentional leak, right?  If so, please add a comment
to that effect -- unprotected |new|s are surprising in Chromium code, and should
be documented.

+  histogram->SetFlags(flags);
+  return histogram;
+}

[Ilya Sherman, 2012/08/04 01:18:35]

Is this method eventually going to look up the histogram in a global lookup
table or something like that?  If so, please add documentation to indicate that
this is meant to be updated.  Otherwise, can you help me understand the
advantage of the factory method over the constructor?

[kaiwang, 2012/08/08 03:59:33]

You are right. FactoryGet will eventually register and get SparseHistogram from
StatisticsRecorder.

Now StatisticsRecorder only supports Histogram. Need more refactor CLs

Added TODO.

+
+SparseHistogram::~SparseHistogram() {}
+
+void SparseHistogram::Add(Sample value) {
+  base::AutoLock auto_lock(*lock_);
+  sample_[value]++;
+}
+
+void SparseHistogram::SnapshotSample(
+    SparseHistogram::SampleCounts* sample) const {
+  base::AutoLock auto_lock(*lock_);
+  *sample = sample_;
+}
+
+void SparseHistogram::WriteHTMLGraph(string* output) const {

[Ilya Sherman, 2012/08/04 01:18:35]

nit: Please add a TODO, linked to a crbug, to implement this.  (Though I wish
this weren't part of base; it's really not core functionality :/)

[kaiwang, 2012/08/08 03:59:33]

Added todo.
Most code is in histogram.cc. Seems not easy to move it out of base...

+}
+
+void SparseHistogram::WriteAscii(string* output) const {
+}
+
+SparseHistogram::SparseHistogram(const string& name)
+    : HistogramBase(name),
+      lock_(new base::Lock()) {}
+
+}  // namespace base
Property changes on: base/metrics/sparse_histogram.cc
___________________________________________________________________
Added: svn:eol-style
   + LF