Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* …

base/metrics/local_histogram_macros.h

Index: base/metrics/local_histogram_macros.h
diff --git a/base/metrics/local_histogram_macros.h b/base/metrics/local_histogram_macros.h
new file mode 100644
index 0000000000000000000000000000000000000000..50d87b7b19c06dc238838eda370b96342d6b11e5
--- /dev/null
+++ b/base/metrics/local_histogram_macros.h
@@ -0,0 +1,89 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.

[Mark P, 2016/09/23 19:30:42]

Consider histogram_macros_local instead for better ordering / grouping when
listing files.

[rkaplow, 2016/09/26 20:16:11]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_METRICS_LOCAL_HISTOGRAM_MACROS_H_
+#define BASE_METRICS_LOCAL_HISTOGRAM_MACROS_H_
+
+

[Mark P, 2016/09/23 19:30:42]

nit: remove extra blank line

[rkaplow, 2016/09/26 20:16:11]

Done.

+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/metrics/histogram_macros_internal.h"
+#include "base/time/time.h"
+
+// TODO(rkaplow): Migrate all LOCAL_* usage within Chromium to include this
+// file instead of the histogram_macros.h file.
+
+
+//------------------------------------------------------------------------------
+// Count histograms. These are used for collecting numeric data. Note that we
+// have macros for more specialized use cases below (memory, time, percentages).
+// For usage details, see the equivalents in histogram_macros.h.
+
+#define LOCAL_HISTOGRAM_COUNTS_100(name, sample)                               \

[Mark P, 2016/09/23 19:30:42]

FYI, in this file, it looks like you merely copied the macros to here.  I didn't
review them closely.

[rkaplow, 2016/09/26 20:16:11]

I just did reordering in general - I don't think I added any new ones here. 

+    LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 100, 50)
+
+#define LOCAL_HISTOGRAM_COUNTS_10000(name, sample)                             \
+    LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 10000, 50)
+
+#define LOCAL_HISTOGRAM_COUNTS_1000000(name, sample)                           \
+    LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 1000000, 50)
+
+#define LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count)    \
+    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                                \
+        name, sample, min, max, bucket_count, base::HistogramBase::kNoFlags)
+
+
+//------------------------------------------------------------------------------

[Mark P, 2016/09/23 19:30:43]

optional nit: here and below the two blanks lines seem too much for me.

[rkaplow, 2016/09/26 20:16:11]

Done.

+// Timing histograms. These are used for collecting timing data (generally
+// latencies).
+// For usage details, see the equivalents in histogram_macros.h.

[Mark P, 2016/09/23 19:30:42]

nit: I know this duplicates the same sentence as above.  However, the spacing
looks odd.  Either combine with the previous line or put a blank line // before
the usage details line (and do the same elsewhere).

[rkaplow, 2016/09/26 20:16:11]

Done.

+
+#define LOCAL_HISTOGRAM_TIMES(name, sample) LOCAL_HISTOGRAM_CUSTOM_TIMES(      \
+    name, sample, base::TimeDelta::FromMilliseconds(1),                        \
+    base::TimeDelta::FromSeconds(10), 50)
+
+#define LOCAL_HISTOGRAM_CUSTOM_TIMES(name, sample, min, max, bucket_count)     \
+    STATIC_HISTOGRAM_POINTER_BLOCK(name, AddTime(sample),                      \
+        base::Histogram::FactoryTimeGet(name, min, max, bucket_count,          \
+                                        base::HistogramBase::kNoFlags))
+
+
+//------------------------------------------------------------------------------
+// Memory histograms.
+// For usage details, see the equivalents in histogram_macros.h.
+
+#define LOCAL_HISTOGRAM_MEMORY_KB(name, sample) LOCAL_HISTOGRAM_CUSTOM_COUNTS( \
+    name, sample, 1000, 500000, 50)
+
+
+//------------------------------------------------------------------------------
+// Enumeration histograms.
+

[Mark P, 2016/09/23 19:30:42]

Missing usage details line?

[rkaplow, 2016/09/26 20:16:11]

Done.

+#define LOCAL_HISTOGRAM_ENUMERATION(name, sample, enum_max)                    \
+   INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(                                   \
+        name, sample, enum_max,                                                \
+        base::HistogramBase::kNoFlags)
+
+#define LOCAL_HISTOGRAM_BOOLEAN(name, sample)                                  \
+    STATIC_HISTOGRAM_POINTER_BLOCK(name, AddBoolean(sample),                   \
+        base::BooleanHistogram::FactoryGet(name, base::Histogram::kNoFlags))
+
+#define LOCAL_HISTOGRAM_PERCENTAGE(name, under_one_hundred)                    \

[Mark P, 2016/09/23 19:30:43]

A questionable section to put this in..  Maybe it's own section?  I don't feel
strongly though.

[rkaplow, 2016/09/26 20:16:11]

agreed - I ended up putting it with enums since it acts as an enum under the
hood although usage wise it's not a bit different.

I'm fine moving it if anyone feels like it should be somewhere different.

+    LOCAL_HISTOGRAM_ENUMERATION(name, under_one_hundred, 101)
+
+
+//------------------------------------------------------------------------------
+// Deprecated histograms. Not recommended for current use.
+
+// Legacy non-explicit version. We suggest using LOCAL_HISTOGRAM_COUNTS_1000000
+// instead.
+#define LOCAL_HISTOGRAM_COUNTS(name, sample)                                   \

[Mark P, 2016/09/23 19:30:43]

This is only used in 7 files in the codepath (including two within
base/metrics).  It should be easy to retire/remove it.  Then we can get rid of
this whole section.
Best done in another changelist though.

[rkaplow, 2016/09/26 20:16:11]

Ack

+    LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 1000000, 50)
+
+#define LOCAL_HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges)        \

[Mark P, 2016/09/23 19:30:42]

Appears to not be used anywhere.  You can probably remove this.

[rkaplow, 2016/09/26 20:16:11]

I did consider this. I was thinking it might make sense to leave since it makes
sense to mirror the UMA_ versions.

On the other hand, since this is not encouraged for use, and is not being used
at all now, seems fine to remove.

+    STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample),                          \
+        base::CustomHistogram::FactoryGet(name, custom_ranges,                 \
+                                          base::HistogramBase::kNoFlags))
+
+#endif  // BASE_METRICS_LOCAL_HISTOGRAM_MACROS_H_