Added UMA metrics for screen capture UI in the status area.

Added UMA metrics for screen capture UI in the status area.

Record user actions when:
- Screen capture is stopped from the default view
- Screen capture is stopped from the notification view.

BUG=620508
TEST=manual

Committed: https://crrev.com/54c8f535cf5fe486c9c992c2835b477ba592b07c
Cr-Commit-Position: refs/heads/master@{#404542}

[bruthig, 2016-07-07 20:01:05]

bruthig@chromium.org changed reviewers:
+ tdanderson@chromium.org

[bruthig, 2016-07-07 20:01:06]

Terry, can you take a first look?

[tdanderson, 2016-07-07 21:54:32]

lgtm with nuts

https://chromiumcodereview.appspot.com/2132773002/diff/20001/ash/common/syste...
File ash/common/system/chromeos/screen_security/screen_tray_item.cc (right):

https://chromiumcodereview.appspot.com/2132773002/diff/20001/ash/common/syste...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:172: void
ScreenTrayItem::RecordStoppedFromDefaultViewMetric() {}
Out of curiosity, does 'git cl format ash' complain about this style?

https://chromiumcodereview.appspot.com/2132773002/diff/20001/ash/metrics/user...
File ash/metrics/user_metrics_recorder.cc (right):

https://chromiumcodereview.appspot.com/2132773002/diff/20001/ash/metrics/user...
ash/metrics/user_metrics_recorder.cc:202: // UserMetricsActions must be on the
same line as the string literal. This
I find this a bit surprising. What's the reason for them needing to be on the
same line (I assume it's something to do with the fact we're inside case: cases)
?

nit: UserMetricsActions->UserMetricsAction.

https://chromiumcodereview.appspot.com/2132773002/diff/20001/tools/metrics/ac...
File tools/metrics/actions/actions.xml (right):

https://chromiumcodereview.appspot.com/2132773002/diff/20001/tools/metrics/ac...
tools/metrics/actions/actions.xml:13908: "Screen Capture" view in the
system menu.
nit: remove quotes

Consider rewording for clarity: "... via the screen capture row in the system
menu" or "... via the screen capture row's stop button in the system menu"

https://chromiumcodereview.appspot.com/2132773002/diff/20001/tools/metrics/ac...
tools/metrics/actions/actions.xml:13917: Screen Capture" notification.
nit: remove quotes

Is this notification part of the notifications center, or is it one of those
standalone pre-notification-center "notifications" ? May be worth clarifying
here if, for instance, this action ever gets used to measure something related
to the notification center as a whole.

[bruthig, 2016-07-08 14:26:53]

https://codereview.chromium.org/2132773002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/screen_security/screen_tray_item.cc (right):

https://codereview.chromium.org/2132773002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:172: void
ScreenTrayItem::RecordStoppedFromDefaultViewMetric() {}
On 2016/07/07 21:54:31, tdanderson wrote:
> Out of curiosity, does 'git cl format ash' complain about this style?

Nope, it will actually format it like this if the braces were on separate lines.

https://codereview.chromium.org/2132773002/diff/20001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/2132773002/diff/20001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.cc:202: // UserMetricsActions must be on the
same line as the string literal. This
On 2016/07/07 21:54:31, tdanderson wrote:
> I find this a bit surprising. What's the reason for them needing to be on the
> same line (I assume it's something to do with the fact we're inside case:
cases)
> ?
> 
> nit: UserMetricsActions->UserMetricsAction.

It is required for the metric processing tools.  More info can be found here:
https://cs.chromium.org/chromium/src/base/metrics/user_metrics.h?rcl=0&l=29

but quoted for convenience:
// WARNING: In calls to this function, UserMetricsAction and a
// string literal parameter must be on the same line, e.g.
//   RecordAction(UserMetricsAction("my extremely long action name"));
// This ensures that our processing scripts can associate this action's hash
// with its metric name. Therefore, it will be possible to retrieve the metric
// name from the hash later on.

https://codereview.chromium.org/2132773002/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2132773002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:13908: "Screen Capture" view in the
system menu.
On 2016/07/07 21:54:31, tdanderson wrote:
> nit: remove quotes
> 
> Consider rewording for clarity: "... via the screen capture row in the system
> menu" or "... via the screen capture row's stop button in the system menu"

Done.

https://codereview.chromium.org/2132773002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:13917: Screen Capture" notification.
On 2016/07/07 21:54:31, tdanderson wrote:
> nit: remove quotes
> 
> Is this notification part of the notifications center, or is it one of those
> standalone pre-notification-center "notifications" ? May be worth clarifying
> here if, for instance, this action ever gets used to measure something related
> to the notification center as a whole.

Done.

[bruthig, 2016-07-08 21:25:56]

bruthig@chromium.org changed reviewers:
+ jamescook@chromium.org, rkaplow@chromium.org

[bruthig, 2016-07-08 21:25:57]

jamescook@, can you please review changes in:
- ash/*

rkaplow@, can you please review changes in:
- tools/metrics/actions/actions.xml

[rkaplow, 2016-07-08 21:30:10]

lgtm

[James Cook, 2016-07-08 21:50:21]

LGTM with nit.

I'm concerned that we're adding a bunch of metrics here that we're only going to
use for a milestone or two (I presume to make some kind of UI decision). I doubt
we need them forever. Can you ask your PM about when these metrics can be
removed and file a bug to clean them up?

https://codereview.chromium.org/2132773002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/screen_security/screen_tray_item.h (right):

https://codereview.chromium.org/2132773002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/screen_security/screen_tray_item.h:121: virtual void
RecordStoppedFromDefaultViewMetric();
nit: I think it would be clearer if these were pure-virtual, and you put a
comment in the empty implementation in the subclass just saying it's intentional
that you aren't recording a metric.

[bruthig, 2016-07-08 22:11:11]

On 2016/07/08 21:50:21, James Cook wrote:
> LGTM with nit.
> 
> I'm concerned that we're adding a bunch of metrics here that we're only going
to
> use for a milestone or two (I presume to make some kind of UI decision). I
doubt
> we need them forever. Can you ask your PM about when these metrics can be
> removed and file a bug to clean them up?

I feel like we're only back filling missing metrics that would have been useful
all along.  It will be a lot easier to keep a pulse on what UI elements/features
are used if we have these types of metrics for all elements for the long term.
It just happens that we have a very short white list of the UI elements we are
currently interested in.
  
I'm not sure I understand the concern of keeping these metrics around long term?
 

> 
>
https://codereview.chromium.org/2132773002/diff/120001/ash/common/system/chro...
> File ash/common/system/chromeos/screen_security/screen_tray_item.h (right):
> 
>
https://codereview.chromium.org/2132773002/diff/120001/ash/common/system/chro...
> ash/common/system/chromeos/screen_security/screen_tray_item.h:121: virtual
void
> RecordStoppedFromDefaultViewMetric();
> nit: I think it would be clearer if these were pure-virtual, and you put a
> comment in the empty implementation in the subclass just saying it's
intentional
> that you aren't recording a metric.

[bruthig, 2016-07-08 22:14:44]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-07-08 22:14:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, jamescook@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2132773002/#ps160001
(title: "Merge branch 'master' into uma_screen_capture")

[commit-bot: I haz the power, 2016-07-08 22:15:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-07-08 22:17:14]

My concern is that we have hundreds (thousands?) of UMA metrics, most of which
are never used to make decisions in practice.

I presume these are being added because now someone is interested in them. I
would like to know if they are going to be interested forever, or if there's a
short-term decision they are trying to make.

[commit-bot: I haz the power, 2016-07-09 00:16:36]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-09 00:16:38]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-09 00:20:21]

Description was changed from

==========
Added UMA metrics for screen capture UI in the status area.

Record user actions when:
- Screen capture is stopped from the default view
- Screen capture is stopped from the notification view.

BUG=620508
TEST=manual
==========

to

==========
Added UMA metrics for screen capture UI in the status area.

Record user actions when:
- Screen capture is stopped from the default view
- Screen capture is stopped from the notification view.

BUG=620508
TEST=manual

Committed: https://crrev.com/54c8f535cf5fe486c9c992c2835b477ba592b07c
Cr-Commit-Position: refs/heads/master@{#404542}
==========

[commit-bot: I haz the power, 2016-07-09 00:20:22]

Patchset 9 (id:??) landed as
https://crrev.com/54c8f535cf5fe486c9c992c2835b477ba592b07c
Cr-Commit-Position: refs/heads/master@{#404542}

[bruthig, 2016-07-12 17:40:29]

On 2016/07/08 22:17:14, James Cook wrote:
> My concern is that we have hundreds (thousands?) of UMA metrics, most of which
> are never used to make decisions in practice.
> 
> I presume these are being added because now someone is interested in them. I
> would like to know if they are going to be interested forever, or if there's a
> short-term decision they are trying to make.

Created https://crbug.com/627532