Added UMA stats to overview mode

Added UMA stats to overview mode

This patch:
* Brought back WindowSelector.CycleTime histogram with different name
  (WindowCycleController.CycleTime).
* Renamed WindowSelector.Selection to WindowCycleController.Cycle and fixed that it
  counted incorrectly overview mode selections.
* Fixed a bug in which the number of items in overview mode were not counted
  correctly.
* Added stats for:
** Number of closed items.
** Number of times the enter key is used to select a window.
** Number of arrow key presses per session.
** Ratio between arrow key presses and number of items.

BUG=364590
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276710

[Nina, 2014-06-09 15:55:24]

Hi Terry, can you please give this patch a first look? Thanks!

[tdanderson, 2014-06-09 17:50:46]

Some nits below, otherwise LGTM. Send to Rob when ready.

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_grid.cc
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:311: window_selector_->on_window_removed();
Perhaps include a comment that this call is only for the purpose of collecting
metrics.

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:138: base::Time overview_start_time_;
Similar to my other comment, it might be good to include a comment for the new
member variables you're adding to say that they're only being used for metrics.

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:146: // The number of items that were removed
from the overview.
Similar to |num_key_presses_|, clarify that this counter is for a single
overview session only and is not cumulative.

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

https://codereview.chromium.org/320303002/diff/1/tools/metrics/actions/action...
tools/metrics/actions/actions.xml:10977: <description>Please enter the
description of the metric.</description>
Why are you making the <description> text different than all of the surrounding
ones? I looked at actions.xml in my local checkout and looks like only ~4
existing actions have this different text. What difference does it make?

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:696: + 
<owner>tdanderson@chromium.org</owner>
Get Rob's feedback on this, but I suspect you'll want the owners of these to be
Rob and Kuscher (not me).

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:700: +    key.
Specify that the count recorded represents the number of arrow key presses in a
single OM session.

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:709: +    the overview mode.
Specify that it's recorded as the number of items that are present at the start
of each session.

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:718: +    expressed as a percentage.
for a single OM session

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:725: +  <summary>The number of items
closed from the window overview.</summary>
for a single OM session

[Nina, 2014-06-09 18:12:51]

Terry, I got your comments either fixed or answered.
Rob, can you give the patch a look? Thanks!

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_grid.cc
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:311: window_selector_->on_window_removed();
On 2014/06/09 17:50:45, tdanderson wrote:
> Perhaps include a comment that this call is only for the purpose of collecting
> metrics.

Done.

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:138: base::Time overview_start_time_;
On 2014/06/09 17:50:46, tdanderson wrote:
> Similar to my other comment, it might be good to include a comment for the new
> member variables you're adding to say that they're only being used for
metrics.

Done.

https://codereview.chromium.org/320303002/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:146: // The number of items that were removed
from the overview.
On 2014/06/09 17:50:46, tdanderson wrote:
> Similar to |num_key_presses_|, clarify that this counter is for a single
> overview session only and is not cumulative.

I clarified this with a general comment.

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

https://codereview.chromium.org/320303002/diff/1/tools/metrics/actions/action...
tools/metrics/actions/actions.xml:10977: <description>Please enter the
description of the metric.</description>
On 2014/06/09 17:50:46, tdanderson wrote:
> Why are you making the <description> text different than all of the
surrounding
> ones? I looked at actions.xml in my local checkout and looks like only ~4
> existing actions have this different text. What difference does it make?

Well, the automatic tool that extracts these tags from the code decided to write
that comment. I suggest we leave them like that, after all, they are just
placeholder comments.

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:696: + 
<owner>tdanderson@chromium.org</owner>
On 2014/06/09 17:50:46, tdanderson wrote:
> Get Rob's feedback on this, but I suspect you'll want the owners of these to
be
> Rob and Kuscher (not me).

Rob was the one that suggested adding you. I'm sorry you're gonna be blamed if
this fails :)

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:700: +    key.
On 2014/06/09 17:50:46, tdanderson wrote:
> Specify that the count recorded represents the number of arrow key presses in
a
> single OM session.

Done.

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:709: +    the overview mode.
On 2014/06/09 17:50:46, tdanderson wrote:
> Specify that it's recorded as the number of items that are present at the
start
> of each session.

Done.

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:718: +    expressed as a percentage.
On 2014/06/09 17:50:46, tdanderson wrote:
> for a single OM session

Done.

https://codereview.chromium.org/320303002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:725: +  <summary>The number of items
closed from the window overview.</summary>
On 2014/06/09 17:50:46, tdanderson wrote:
> for a single OM session

Done.

[flackr, 2014-06-09 21:23:30]

Are you forgetting to upload a patchset 2?

[Nina, 2014-06-09 22:11:07]

Yes I was. Please have a look now.

[flackr, 2014-06-10 15:39:42]

https://codereview.chromium.org/320303002/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/320303002/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:177: num_closed_items_);
This metric seems like it is the same as num_items minus the total size of all
WindowGrid's at destruction time.

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

https://codereview.chromium.org/320303002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10957: <description>Please enter the
description of the metric.</description>
Describe the metric and update owners. It seems this file was converted to have
these fields recently
(https://src.chromium.org/viewvc/chrome?revision=255680&view=revision) and the
standard convention in such cases is to update as it's modified.

https://codereview.chromium.org/320303002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10977: <description>Please enter the
description of the metric.</description>
Ditto.

https://codereview.chromium.org/320303002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/320303002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:713: +<histogram
name="Ash.WindowSelector.KeyPressesOverItemsRatio">
units="%"

[Nina, 2014-06-10 16:10:14]

Thanks for the comments, got them covered. Please have another look.

https://codereview.chromium.org/320303002/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/320303002/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:177: num_closed_items_);
On 2014/06/10 15:39:41, flackr wrote:
> This metric seems like it is the same as num_items minus the total size of all
> WindowGrid's at destruction time.

Right, I'll update the code to reflect this.

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

https://codereview.chromium.org/320303002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10957: <description>Please enter the
description of the metric.</description>
On 2014/06/10 15:39:42, flackr wrote:
> Describe the metric and update owners. It seems this file was converted to
have
> these fields recently
> (https://src.chromium.org/viewvc/chrome?revision=255680&view=revision) and the
> standard convention in such cases is to update as it's modified.

Done.

https://codereview.chromium.org/320303002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10977: <description>Please enter the
description of the metric.</description>
On 2014/06/10 15:39:41, flackr wrote:
> Ditto.

Done.

https://codereview.chromium.org/320303002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/320303002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:713: +<histogram
name="Ash.WindowSelector.KeyPressesOverItemsRatio">
On 2014/06/10 15:39:42, flackr wrote:
> units="%"

Done.

[flackr, 2014-06-10 17:08:06]

lgtm with nit.

https://codereview.chromium.org/320303002/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/320303002/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:180:
UMA_HISTOGRAM_COUNTS_100("Ash.WindowSelector.OverviewClosedItems",
nit: DCHECK(num_items_ > remaining_items) before this just to ensure if we add
items at some point we remember to update this.

[Nina, 2014-06-10 18:38:23]

Sky, please review ash/metrics/user_metrics_recorder.(h|cc)
Ilya, please review tools/metrics/*
Thanks!

https://codereview.chromium.org/320303002/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/320303002/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:180:
UMA_HISTOGRAM_COUNTS_100("Ash.WindowSelector.OverviewClosedItems",
On 2014/06/10 17:08:06, flackr wrote:
> nit: DCHECK(num_items_ > remaining_items) before this just to ensure if we add
> items at some point we remember to update this.

Changed to >= because it's legal not to close any item.

[sky, 2014-06-10 19:32:08]

LGTM

[Ilya Sherman, 2014-06-10 23:45:13]

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10991: </action>
Please mark the action as obsolete rather than deleting it.

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:685: -<histogram
name="Ash.WindowSelector.CycleTime" units="milliseconds">
Please mark the old histogram as <obsolete> rather than deleting it.

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

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:700: +    the enter key.
What is the starting event for a session -- bringing up the overview mode? 
Might be worth clarifying in the description.

[Nina, 2014-06-11 14:07:00]

Ilya, please review this patch again.

Cheers!

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10991: </action>
On 2014/06/10 23:45:13, Ilya Sherman wrote:
> Please mark the action as obsolete rather than deleting it.

Done.

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:685: -<histogram
name="Ash.WindowSelector.CycleTime" units="milliseconds">
On 2014/06/10 23:45:13, Ilya Sherman wrote:
> Please mark the old histogram as <obsolete> rather than deleting it.

Done.

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

https://codereview.chromium.org/320303002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:700: +    the enter key.
On 2014/06/10 23:45:13, Ilya Sherman wrote:
> What is the starting event for a session -- bringing up the overview mode? 
> Might be worth clarifying in the description.

Done.

[Ilya Sherman, 2014-06-11 17:27:48]

LGTM % nits.  Thanks.

https://codereview.chromium.org/320303002/diff/80001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/320303002/diff/80001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11010: overview mode.
nit: Please include the (rough) date on which this became obsolete.  Not sure if
there are great examples in this file, but there ought to be lots in
histograms.xml, which has been an XML file for longer.

https://codereview.chromium.org/320303002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/320303002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:707: +    WindowSelector.
Ditto.

[tdanderson, 2014-06-11 23:36:13]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-06-11 23:37:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/320303002/100001

[commit-bot: I haz the power, 2014-06-12 00:59:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15778)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18910)

[commit-bot: I haz the power, 2014-06-12 01:05:42]

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

[commit-bot: I haz the power, 2014-06-12 01:05:44]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[tdanderson, 2014-06-12 13:36:29]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-06-12 13:38:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/320303002/100001

[commit-bot: I haz the power, 2014-06-12 16:45:15]

Change committed as 276710