Implement write methods for SparseHistogram

Implement write methods for SparseHistogram
Implementation based on existing methods in Histogram.

BUG=


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

[jrummell, 2013-04-02 23:20:04]

PTAL

[jar (doing other things), 2013-04-04 00:45:09]

Kai recently wrote this code, and he may have had plans that are different. 
I've added him as a reviewer.

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

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:115: // Get a local copy of the data so we are
consistent
nit: Period at end of line.

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:123: // and largest count in a bucket
nit: Upper case start of sentence, and add a period.

Same thing for comments below.

The comment also had me a bit confused.  Please consider instead:

Determine how wide the largest bucket range is (how many digits to print), so
that we'll be able to right-align starts for the graphical bars.
Determine which bucket has the largest sample count so that we can normalize the
graphical bar-width relative to that sample count.

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:125: Sample largest = 0;
nit: suggest largest_count as a better name.  I was getting confused relative to
max_size when reading.

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:184: const int k_line_length = 72;  // Maximal
horizontal width of graph.
nit: This method is also repeated in histogram.cc.

Can we put a method in base, as suggested in line 196?

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:196: std::string
SparseHistogram::GetAsciiBucketRange(const Sample sample) const {
nit: This code is repeated in histogram.cc.

Could we add a method in HistogramBase to do this work? Maybe the base method
should be something like GetSimpleAsciiBucketRange(), and we'd forward to it
here?

Same thing fro line 181 and line 174.

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

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.h:104: void WriteAsciiBucketValue(const Count
current, const Count total,
nit: one arg per line in declaration and definitions.

Same on line 198.

[jrummell, 2013-04-04 22:38:59]

Code refactored into histogram_base.

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

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:115: // Get a local copy of the data so we are
consistent
On 2013/04/04 00:45:09, jar wrote:
> nit: Period at end of line.

Done.

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:123: // and largest count in a bucket
On 2013/04/04 00:45:09, jar wrote:
> nit: Upper case start of sentence, and add a period.
> 
> Same thing for comments below.
> 
> The comment also had me a bit confused.  Please consider instead:
> 
> Determine how wide the largest bucket range is (how many digits to print), so
> that we'll be able to right-align starts for the graphical bars.
> Determine which bucket has the largest sample count so that we can normalize
the
> graphical bar-width relative to that sample count.

Done.

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:125: Sample largest = 0;
On 2013/04/04 00:45:09, jar wrote:
> nit: suggest largest_count as a better name.  I was getting confused relative
to
> max_size when reading.

Done.

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:184: const int k_line_length = 72;  // Maximal
horizontal width of graph.
On 2013/04/04 00:45:09, jar wrote:
> nit: This method is also repeated in histogram.cc.
> 
> Can we put a method in base, as suggested in line 196?

Done.

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.cc:196: std::string
SparseHistogram::GetAsciiBucketRange(const Sample sample) const {
On 2013/04/04 00:45:09, jar wrote:
> nit: This code is repeated in histogram.cc.
> 
> Could we add a method in HistogramBase to do this work? Maybe the base method
> should be something like GetSimpleAsciiBucketRange(), and we'd forward to it
> here?
> 
> Same thing fro line 181 and line 174.

Done.

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

https://codereview.chromium.org/13469020/diff/1/base/metrics/sparse_histogram...
base/metrics/sparse_histogram.h:104: void WriteAsciiBucketValue(const Count
current, const Count total,
On 2013/04/04 00:45:09, jar wrote:
> nit: one arg per line in declaration and definitions.
> 
> Same on line 198.

Done.

[jar (doing other things), 2013-04-04 23:03:34]

This L G T M.... but I'd like Kai to agree, since he should be responsible for
this code.

[kaiwang, 2013-04-05 02:00:58]

jrummell talked to me about the change. So I'm totally fine

[jar (doing other things), 2013-04-05 02:52:27]

Patch set 2 LGTM

[commit-bot: I haz the power, 2013-04-05 17:14:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/13469020/6001

[commit-bot: I haz the power, 2013-04-05 17:14:37]

Failed to apply patch for base/metrics/histogram_base.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file base/metrics/histogram_base.cc
  Hunk #1 FAILED at 13.
  Hunk #2 succeeded at 124 (offset 2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
base/metrics/histogram_base.cc.rej

Patch:       base/metrics/histogram_base.cc
Index: base/metrics/histogram_base.cc
diff --git a/base/metrics/histogram_base.cc b/base/metrics/histogram_base.cc
index
835a05b56efd4ad1087e1363cd4757c1f44f7899..65fd94875370db853618a92c2d66bd410fc2c04e
100644
--- a/base/metrics/histogram_base.cc
+++ b/base/metrics/histogram_base.cc
@@ -13,6 +13,7 @@
 #include "base/metrics/histogram_samples.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/pickle.h"
+#include "base/stringprintf.h"
 #include "base/values.h"
 
 namespace base {
@@ -122,4 +123,35 @@ void HistogramBase::WriteJSON(std::string* output) const {
   serializer.Serialize(root);
 }
 
+void HistogramBase::WriteAsciiBucketGraph(double current_size,
+                                          double max_size,
+                                          std::string* output) const {
+  const int k_line_length = 72;  // Maximal horizontal width of graph.
+  int x_count = static_cast<int>(k_line_length * (current_size / max_size)
+                                 + 0.5);
+  int x_remainder = k_line_length - x_count;
+
+  while (0 < x_count--)
+    output->append("-");
+  output->append("O");
+  while (0 < x_remainder--)
+    output->append(" ");
+}
+
+const std::string HistogramBase::GetSimpleAsciiBucketRange(
+    Sample sample) const {
+  std::string result;
+  if (kHexRangePrintingFlag & flags())
+    StringAppendF(&result, "%#x", sample);
+  else
+    StringAppendF(&result, "%d", sample);
+  return result;
+}
+
+void HistogramBase::WriteAsciiBucketValue(Count current,
+                                          double scaled_sum,
+                                          std::string* output) const {
+  StringAppendF(output, " (%d = %3.1f%%)", current, current/scaled_sum);
+}
+
 }  // namespace base

[commit-bot: I haz the power, 2013-04-05 17:38:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/13469020/19001

[commit-bot: I haz the power, 2013-04-05 21:21:59]

Change committed as 192636