Add UMA actions and histogram for the NPAPI InfoBar.

Add UMA actions and histogram for the NPAPI InfoBar.

BUG=380227
Committed: https://crrev.com/ad9bbee14283c370141cc8a84bc7ec8bff78190a
Cr-Commit-Position: refs/heads/master@{#329473}

[Will Harris, 2015-05-12 00:37:01]

wfh@chromium.org changed reviewers:
+ asvitkine@chromium.org, bauerb@chromium.org

[Will Harris, 2015-05-12 00:37:26]

PTAL :)

[Will Harris, 2015-05-12 00:38:35]

https://codereview.chromium.org/1130793006/diff/1/chrome/browser/plugins/chro...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/1130793006/diff/1/chrome/browser/plugins/chro...
chrome/browser/plugins/chrome_plugin_service_filter.cc:95:
UserMetricsAction("NPAPIRemovalInfobar.Shown.Removed.Java"));
this code shamelessly stolen from plugin_infobar_delegates.cc

[Bernhard Bauer, 2015-05-12 10:03:14]

lgtm

https://codereview.chromium.org/1130793006/diff/1/tools/metrics/actions/actio...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1130793006/diff/1/tools/metrics/actions/actio...
tools/metrics/actions/actions.xml:8215: <description>Please enter the
description of the metric.</description>
Please do :)

[Will Harris, 2015-05-12 16:02:40]

https://codereview.chromium.org/1130793006/diff/1/tools/metrics/actions/actio...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1130793006/diff/1/tools/metrics/actions/actio...
tools/metrics/actions/actions.xml:8215: <description>Please enter the
description of the metric.</description>
On 2015/05/12 10:03:13, Bernhard Bauer wrote:
> Please do :)

Done.

[Alexei Svitkine (slow), 2015-05-12 16:29:07]

https://codereview.chromium.org/1130793006/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/1130793006/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:92:
UserMetricsAction("NPAPIRemovalInfobar.Shown.Removed"));
I think these would be better captured as a histogram, rather than user actions.

User actions carry timing information (i.e. A happened at t=138384, B happened
at t=2474742) which can be useful, but in this case it seems you just want to
see their frequencies, so a histogram would be sufficient - and more efficient,
in addition to being better supported by our dashboards (e.g. you can slice by
things like os flavor).

(It's still ok to keep user actions for the top level items i.e. Shown and
LinkClicked - since these can help understand users' interactions with Chrome as
a whole).

[Will Harris, 2015-05-12 17:25:25]

Changed to histogram. PTAL.

https://codereview.chromium.org/1130793006/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/1130793006/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:92:
UserMetricsAction("NPAPIRemovalInfobar.Shown.Removed"));
On 2015/05/12 16:29:07, Alexei Svitkine wrote:
> I think these would be better captured as a histogram, rather than user
actions.
> 
> User actions carry timing information (i.e. A happened at t=138384, B happened
> at t=2474742) which can be useful, but in this case it seems you just want to
> see their frequencies, so a histogram would be sufficient - and more
efficient,
> in addition to being better supported by our dashboards (e.g. you can slice by
> things like os flavor).
> 
> (It's still ok to keep user actions for the top level items i.e. Shown and
> LinkClicked - since these can help understand users' interactions with Chrome
as
> a whole).

Done.

[Alexei Svitkine (slow), 2015-05-12 17:38:16]

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:43: enum PluginGroup {
Nit: Add a comment that this is used in UMA and entries shouldn't be re-ordered
or removed.

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:103: std::string name =
base::UTF16ToUTF8(plugin_name);
Nit: Move this closer to where it's used - i.e. above the loop.

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:123: for (auto type :
types) {
Nit: const auto&

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:135: } else {
Does it make sense to add a DCHECK on the message_id here?

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:139:
UserMetricsAction("NPAPIRemovalInfobar.Shown.RemovedSoon"));
Are the Removed and RemovedSoon actions necessary? You should be able to get the
total this is hit just from the histogram.

https://codereview.chromium.org/1130793006/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1130793006/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:25049: +<histogram
name="NPAPIRemovalInfobar.Removed.PluginGroup" enum="PluginGroup">
Nit: Can you use an existing prefix instead of adding a new one? e.g. Plugin.*

https://codereview.chromium.org/1130793006/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:59971: +  <int value="0"
label="GROUP_NAME_ADOBE_READER">Adobe Reader</int>
Nit: I would just put the human readable names in the label="" key and not
include the enum names.

[Will Harris, 2015-05-12 18:25:48]

All done. Also added two plugins we want to histogram. PTAL.

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:43: enum PluginGroup {
On 2015/05/12 17:38:15, Alexei Svitkine wrote:
> Nit: Add a comment that this is used in UMA and entries shouldn't be
re-ordered
> or removed.

Done.

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:103: std::string name =
base::UTF16ToUTF8(plugin_name);
On 2015/05/12 17:38:15, Alexei Svitkine wrote:
> Nit: Move this closer to where it's used - i.e. above the loop.

Done.

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:123: for (auto type :
types) {
On 2015/05/12 17:38:15, Alexei Svitkine wrote:
> Nit: const auto&

Done.

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:135: } else {
On 2015/05/12 17:38:15, Alexei Svitkine wrote:
> Does it make sense to add a DCHECK on the message_id here?

Done.

https://codereview.chromium.org/1130793006/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:139:
UserMetricsAction("NPAPIRemovalInfobar.Shown.RemovedSoon"));
On 2015/05/12 17:38:15, Alexei Svitkine wrote:
> Are the Removed and RemovedSoon actions necessary? You should be able to get
the
> total this is hit just from the histogram.

Done.

https://codereview.chromium.org/1130793006/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1130793006/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:25049: +<histogram
name="NPAPIRemovalInfobar.Removed.PluginGroup" enum="PluginGroup">
On 2015/05/12 17:38:15, Alexei Svitkine wrote:
> Nit: Can you use an existing prefix instead of adding a new one? e.g. Plugin.*

Done.

https://codereview.chromium.org/1130793006/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:59971: +  <int value="0"
label="GROUP_NAME_ADOBE_READER">Adobe Reader</int>
On 2015/05/12 17:38:15, Alexei Svitkine wrote:
> Nit: I would just put the human readable names in the label="" key and not
> include the enum names.

Done.

[Alexei Svitkine (slow), 2015-05-12 18:33:02]

lgtm % nit

https://codereview.chromium.org/1130793006/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1130793006/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:59981: +  <int value="9" label="Other
Plugin"/>
Nit: I'd put this first in the list (and update the enum in C++), so that if we
ever add new entries, it won't be in the middle of the list.

[Will Harris, 2015-05-12 19:02:55]

Thanks for the reviews!

https://codereview.chromium.org/1130793006/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1130793006/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:59981: +  <int value="9" label="Other
Plugin"/>
On 2015/05/12 18:33:01, Alexei Svitkine wrote:
> Nit: I'd put this first in the list (and update the enum in C++), so that if
we
> ever add new entries, it won't be in the middle of the list.

Done. Great spot.

[Will Harris, 2015-05-12 19:03:26]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2015-05-12 19:03:26]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1130793006/#ps80001
(title: "put unknown at top of histogram")

[commit-bot: I haz the power, 2015-05-12 19:05:15]

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

[commit-bot: I haz the power, 2015-05-12 20:28:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-12 20:29:02]

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