DevTools: introduce RecordEnumeratedHistogram method.

chrome/browser/devtools/devtools_ui_bindings.cc

 // Copyright 2014 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 "chrome/browser/devtools/devtools_ui_bindings.h"
 
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/metrics/histogram.h"
@@ skipping 724 matching lines @@
   } else {
     remote_targets_handler_.reset();
   }
 }
 
 void DevToolsUIBindings::SendMessageToBrowser(const std::string& message) {
   if (agent_host_.get())
     agent_host_->DispatchProtocolMessage(message);
 }
 
 void DevToolsUIBindings::RecordActionUMA(const std::string& name, int action) {

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

What is the backward compatibility reason that makes this method important to
preserve?  Don't we ship the dev tools as part of Chrome?  In other words, I'd
expect us to be able to guarantee that there are no remaining callers of this
method, since we ship the calling code in the same binary as the called code.

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

I updated issue description; they will be removed in a follow-up:
https://codereview.chromium.org/1060573003/

   if (name == kDevToolsActionTakenHistogram)
     UMA_HISTOGRAM_ENUMERATION(name, action, kDevToolsActionTakenBoundary);
   else if (name == kDevToolsPanelShownHistogram)
     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) {
   if (!android_bridge_) {
     callback.Run(nullptr);
     return;
   }
   android_bridge_->SendJsonRequest(browser_id, url,
       base::Bind(&DevToolsUIBindings::JsonReceived,
                  weak_factory_.GetWeakPtr(),
@@ skipping 248 matching lines @@
   if (frontend_loaded_)
     return;
   frontend_loaded_ = true;
 
   // Call delegate first - it seeds importants bit of information.
   delegate_->OnLoadCompleted();
 
   UpdateTheme();
   AddDevToolsExtensionsToClient();
 }