DevTools: introduce RecordEnumeratedHistogram method.

DevTools: introduce RecordEnumeratedHistogram method.

ATM there is a recordActionUMA method in devtools_ui_bindings which is
responsible for UMA metrics. However, the method hard-codes the bucket
sizes, which is unfortunate.

The patch introduces new RecordEnumeratedHistogram method which gets
bucket sizes as an argument.

R=dgozman
BUG=468026
Committed: https://crrev.com/fc94461996881d54083195fab9721fb738bb83f4
Cr-Commit-Position: refs/heads/master@{#324219}

[lushnikov, 2015-04-03 13:38:48]

ptal

[lushnikov, 2015-04-03 13:58:51]

lushnikov@chromium.org changed reviewers:
+ tsepez@chromium.org

[lushnikov, 2015-04-03 13:58:52]

tsepez@chromium.org: Please review changes in
devtools_embedder_message_dispatcher.h

[dgozman, 2015-04-03 13:58:57]

dgozman@chromium.org changed reviewers:
- tsepez@chromium.org

[dgozman, 2015-04-03 13:58:58]

lgtm

[lushnikov, 2015-04-03 14:18:47]

FYI: blink-side https://codereview.chromium.org/1061503002/

[lushnikov, 2015-04-03 14:19:39]

lushnikov@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2015-04-03 17:46:52]

https://codereview.chromium.org/1056073003/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:755: CHECK(0 < bucket_size &&
bucket_size < 100);
CHECK-ing in the browser is bad when the renderer is at fault.  If any of the
conditions here aren't met (including an unknown name below), you want to
terminate the renderer (e.g. BadMessageReceived() if it's available in this
context).

[dgozman, 2015-04-06 10:07:04]

https://codereview.chromium.org/1056073003/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:759:
UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);
Please wrap in {}

[lushnikov, 2015-04-06 10:10:06]

https://codereview.chromium.org/1056073003/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:759:
UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);
On 2015/04/06 10:07:04, dgozman wrote:
> Please wrap in {}

Done.

[lushnikov, 2015-04-06 10:10:39]

Tom, please take another look

https://codereview.chromium.org/1056073003/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:755: CHECK(0 < bucket_size &&
bucket_size < 100);
On 2015/04/03 17:46:51, Tom Sepez wrote:
> CHECK-ing in the browser is bad when the renderer is at fault.  If any of the
> conditions here aren't met (including an unknown name below), you want to
> terminate the renderer (e.g. BadMessageReceived() if it's available in this
> context). 

Done.

[Tom Sepez, 2015-04-06 17:14:47]

LGTM, thanks.

[lushnikov, 2015-04-06 17:19:08]

jam@chromium.org: Please review changes in content/browser and
content/public/browser
isherman@chromium.org: Please review changes in tools/metrics/histograms

[lushnikov, 2015-04-06 17:19:26]

lushnikov@chromium.org changed reviewers:
+ isherman@chromium.org, jam@chromium.org

[jam, 2015-04-06 23:12:24]

lgtm

[Ilya Sherman, 2015-04-06 23:31:24]

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:745: void
DevToolsUIBindings::RecordActionUMA(const std::string& name, int action) {
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.

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:752: void
DevToolsUIBindings::RecordUMA(const std::string& name,
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.

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:754: int bucket_size) {
nit: I would name these parameters |sample| and |boundary_value| or
|bucket_count|.

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:759:
UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);
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...

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:759:
UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);
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.

[lushnikov, 2015-04-07 13:51:04]

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:745: void
DevToolsUIBindings::RecordActionUMA(const std::string& name, int action) {
I updated issue description; they will be removed in a follow-up:
https://codereview.chromium.org/1060573003/

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:752: void
DevToolsUIBindings::RecordUMA(const std::string& name,
On 2015/04/06 23:31:21, Ilya Sherman wrote:
> 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.

Done.

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:754: int bucket_size) {
On 2015/04/06 23:31:22, Ilya Sherman wrote:
> nit: I would name these parameters |sample| and |boundary_value| or
> |bucket_count|.

Done.

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:759:
UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);
Every early return will have a body of two lines (badmessage + return), making
the method look bulky; this version felt the most concise.

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:759:
UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);
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]

Thanks!

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:759:
UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);
On 2015/04/07 13:51:03, lushnikov wrote:
> 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).

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]

Thank you, Ilya, for the clarification.
Please, take another look!

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/120001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:759:
UMA_HISTOGRAM_ENUMERATION(name, action, bucket_size);
On 2015/04/07 21:45:08, Ilya Sherman wrote:
> On 2015/04/07 13:51:03, lushnikov wrote:
> > 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).
> 
> 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.

Done.

[Ilya Sherman, 2015-04-07 23:11:25]

LGTM, thanks.

https://codereview.chromium.org/1056073003/diff/160001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/160001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:756: sample < boundary_value)) {
nit: Please indent this line two more spaces.

https://codereview.chromium.org/1056073003/diff/160001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:763:
UMA_HISTOGRAM_ENUMERATION(name, sample, boundary_value);
Optional nit: It might be worth documenting why the code is written this way, so
that somebody doesn't come along later and introduce bugs by trying to apply a
natural-looking cleanup.

[lushnikov, 2015-04-07 23:21:43]

https://codereview.chromium.org/1056073003/diff/160001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1056073003/diff/160001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:756: sample < boundary_value)) {
On 2015/04/07 23:11:25, Ilya Sherman wrote:
> nit: Please indent this line two more spaces.

Done.

https://codereview.chromium.org/1056073003/diff/160001/chrome/browser/devtool...
chrome/browser/devtools/devtools_ui_bindings.cc:763:
UMA_HISTOGRAM_ENUMERATION(name, sample, boundary_value);
On 2015/04/07 23:11:25, Ilya Sherman wrote:
> Optional nit: It might be worth documenting why the code is written this way,
so
> that somebody doesn't come along later and introduce bugs by trying to apply a
> natural-looking cleanup.

Done.

[lushnikov, 2015-04-07 23:21:50]

The CQ bit was checked by lushnikov@chromium.org

[lushnikov, 2015-04-07 23:21:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, tsepez@chromium.org, jam@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1056073003/#ps180001
(title: "fix last nits")

[commit-bot: I haz the power, 2015-04-07 23:22:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056073003/180001

[commit-bot: I haz the power, 2015-04-08 00:45:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-08 00:45:07]

Failed to apply patch for content/browser/bad_message.h:
While running git apply --index -3 -p1;
  error: patch failed: content/browser/bad_message.h:37
  Falling back to three-way merge...
  Applied patch to 'content/browser/bad_message.h' with conflicts.
  U content/browser/bad_message.h

Patch:       content/browser/bad_message.h
Index: content/browser/bad_message.h
diff --git a/content/browser/bad_message.h b/content/browser/bad_message.h
index
835ae6fa5477bf2b89a1dc151ac6aa05b0e2cfd7..1ca459fe38d891d42bb0961b2f06539e93764725
100644
--- a/content/browser/bad_message.h
+++ b/content/browser/bad_message.h
@@ -37,6 +37,7 @@ enum BadMessageReason {
   SERVICE_WORKER_BAD_URL,
   WC_INVALID_FRAME_SOURCE,
   RWHVM_UNEXPECTED_FRAME_TYPE,
+  DFH_BAD_EMBEDDER_MESSAGE,
   // Please add new elements here. The naming convention is abbreviated class
   // name (e.g. RenderFrameHost becomes RFH) plus a unique description of the
   // reason.

[commit-bot: I haz the power, 2015-04-08 00:46:01]

The CQ bit was unchecked by commit-bot@chromium.org

[lushnikov, 2015-04-08 10:33:04]

The CQ bit was checked by lushnikov@chromium.org

[lushnikov, 2015-04-08 10:33:05]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
isherman@chromium.org, dgozman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1056073003/#ps200001
(title: "rebaseline")

[commit-bot: I haz the power, 2015-04-08 10:33:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056073003/200001

[commit-bot: I haz the power, 2015-04-08 12:29:12]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-04-08 12:30:25]

Patchset 11 (id:??) landed as
https://crrev.com/fc94461996881d54083195fab9721fb738bb83f4
Cr-Commit-Position: refs/heads/master@{#324219}