DevTools: introduce RecordEnumeratedHistogram method.

chrome/browser/devtools/devtools_ui_bindings.cc

Index: chrome/browser/devtools/devtools_ui_bindings.cc
diff --git a/chrome/browser/devtools/devtools_ui_bindings.cc b/chrome/browser/devtools/devtools_ui_bindings.cc
index 03c3c97ae391af45b3c3c767c4f90f82e9308181..d75f072f2c6f3658ac46e94736d21a8821ac535e 100644
--- a/chrome/browser/devtools/devtools_ui_bindings.cc
+++ b/chrome/browser/devtools/devtools_ui_bindings.cc
@@ -749,6 +749,19 @@ void DevToolsUIBindings::RecordActionUMA(const std::string& name, int action) {
     UMA_HISTOGRAM_ENUMERATION(name, action, kDevToolsPanelShownBoundary);
 }
 
+void DevToolsUIBindings::RecordUMA(const std::string& name,

[Ilya Sherman, 2015/04/06 23:31:21]

nit: I would name this method "RecordEnumeratedHistogram" (and propagate that
name throughout this CL).  UMA is capable of recording many types of metrics. 
This method only allows a very specific type of metric.

[lushnikov, 2015/04/07 13:51:03]

Done.

+                                   int action,
+                                   int bucket_size) {

[Ilya Sherman, 2015/04/06 23:31:22]

nit: I would name these parameters |sample| and |boundary_value| or
|bucket_count|.

[lushnikov, 2015/04/07 13:51:03]

Done.

+  bool valid_name = name == kDevToolsActionTakenHistogram ||
+      name == kDevToolsPanelShownHistogram;
+  if (bucket_size >= 0 && bucket_size < 100 &&
+      action >= 0 && action < bucket_size && valid_name) {
+    UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);

[Ilya Sherman, 2015/04/06 23:31:22]

Optional: It might be cleaner to write this method as having a few early
returns, followed by the meat of the method.  Up to you, though.

[Ilya Sherman, 2015/04/06 23:31:22]

This will not work.  If you use the macros, you must guarantee that the
histogram name is a runtime constant, which it is not, in this case.  You should
instead inline the definition of the macro.  You can crib from here:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

[lushnikov, 2015/04/07 13:51:03]

Every early return will have a body of two lines (badmessage + return), making
the method look bulky; this version felt the most concise.

[lushnikov, 2015/04/07 13:51:03]

The name is guaranteed to be one of the constants via line 755 (valid_name
variable). This is exactly the same approach as used in RecordActionUMA (which
works).

[Ilya Sherman, 2015/04/07 21:45:08]

RecordActionUMA has an if/else, which guarantees that each histogram name
follows a different code path.  The issue is that the UMA_HISTOGRAM_ENUMERATION
macro declares a static variable for the histogram, to squeeze out maximum
efficiency in case the code is used in a tight loop.  However, that is
incompatible with passing runtime-variable names, as you do here.  Indeed, if
you try running a debug build and emit to both histograms, the code as-written
will trigger a DCHECK.

[lushnikov, 2015/04/07 23:03:07]

Done.

+  } else {
+    frontend_host_->BadMessageRecieved();
+  }
+}
+
 void DevToolsUIBindings::SendJsonRequest(const DispatchCallback& callback,
                                          const std::string& browser_id,
                                          const std::string& url) {