Added metrics to track the time between task switches done via the shelf.

Added metrics to track the time between task switches done via the shelf.

TEST=TaskSwitchTimeTrackerTest.HasLastActionTimeShouldBeFalseAfterConstruction
TEST=TaskSwitchTimeTrackerTest.HasLastActionTimeShouldBeTrueAfterOnTaskSwitch
TEST=TaskSwitchTimeTrackerTest.RecordAfterTwoTaskSwitches
TEST=TaskSwitchMetricsRecorderTest.VerifyTaskSwitchesFromTheShelfAreRecorded

BUG=469359, 471356
Committed: https://crrev.com/9444f5f41b7cdea355dac95e0de83573b4e31a3d
Cr-Commit-Position: refs/heads/master@{#329884}

[bruthig, 2015-05-13 18:22:39]

bruthig@chromium.org changed reviewers:
+ asvitkine@chromium.org, oshima@chromium.org, rkaplow@chromium.org,
tdanderson@chromium.org

[bruthig, 2015-05-13 18:22:40]

asvitkine@/rkaplow@: Can you please review my usage of histograms.  Specifically
in:
 - ash/metrics/task_switch_time_tracker.cc
 - tools/metrics/histograms/histograms.xml

oshima@: Can you please review from an owner's perspective?
 - ash/*

tdanderson@: Can you please review from a feature correctness perspective?
 - *

[tdanderson, 2015-05-13 19:27:18]

LGTM with a few nits below.

https://chromiumcodereview.appspot.com/1133123003/diff/1/ash/metrics/task_swi...
File ash/metrics/task_switch_metrics_recorder.h (right):

https://chromiumcodereview.appspot.com/1133123003/diff/1/ash/metrics/task_swi...
ash/metrics/task_switch_metrics_recorder.h:39: void
OnTaskSwitch(TaskSwitchSource task_switch_source);
nit: mention the fact this lazily initializes the map entry.

https://chromiumcodereview.appspot.com/1133123003/diff/1/ash/metrics/task_swi...
ash/metrics/task_switch_metrics_recorder.h:43: // |task_switch_source|. May
return |nullptr| if mapping does not exist yet.
nit: no || around nullptr since it's not an identifier name.

https://chromiumcodereview.appspot.com/1133123003/diff/1/ash/metrics/task_swi...
File ash/metrics/task_switch_time_tracker.cc (right):

https://chromiumcodereview.appspot.com/1133123003/diff/1/ash/metrics/task_swi...
ash/metrics/task_switch_time_tracker.cc:27: tick_clock_(new
base::DefaultTickClock()) {
Are you purposely not initializing |last_action_time_| to the zero time in the
constructor?

https://chromiumcodereview.appspot.com/1133123003/diff/1/ash/metrics/task_swi...
ash/metrics/task_switch_time_tracker.cc:59: 
Maybe have a CHECK() that the delta >= 0 ?

https://chromiumcodereview.appspot.com/1133123003/diff/1/ash/metrics/task_swi...
File ash/metrics/task_switch_time_tracker.h (right):

https://chromiumcodereview.appspot.com/1133123003/diff/1/ash/metrics/task_swi...
ash/metrics/task_switch_time_tracker.h:48: // Records the |last_action_time_|
and returns the previous value for
nit: specify that we're setting the member to be the current system time.

https://chromiumcodereview.appspot.com/1133123003/diff/1/tools/metrics/histog...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/1133123003/diff/1/tools/metrics/histog...
tools/metrics/histograms/histograms.xml:736: +    The number of seconds between
contiguous task switch user actions triggered
Perhaps clarify somehow that "contiguous" still means that other task switches
could take place in between, i.e., tapping the shelf, then tapping the tabstrip,
then tapping the shelf again will record an entry here.

[bruthig, 2015-05-13 21:16:21]

I've addressed tdanderson@'s comments from patch set 1.

asvitkine@/rkaplow@: Can you please review my usage of histograms.  Specifically
in:
 - ash/metrics/task_switch_time_tracker.cc
 - tools/metrics/histograms/histograms.xml

oshima@: Can you please review from an owner's perspective?
 - ash/*

tdanderson@: Can you please review the description in histogram.xml?

https://codereview.chromium.org/1133123003/diff/1/ash/metrics/task_switch_met...
File ash/metrics/task_switch_metrics_recorder.h (right):

https://codereview.chromium.org/1133123003/diff/1/ash/metrics/task_switch_met...
ash/metrics/task_switch_metrics_recorder.h:39: void
OnTaskSwitch(TaskSwitchSource task_switch_source);
On 2015/05/13 19:27:18, tdanderson wrote:
> nit: mention the fact this lazily initializes the map entry.

Done.

https://codereview.chromium.org/1133123003/diff/1/ash/metrics/task_switch_met...
ash/metrics/task_switch_metrics_recorder.h:43: // |task_switch_source|. May
return |nullptr| if mapping does not exist yet.
On 2015/05/13 19:27:18, tdanderson wrote:
> nit: no || around nullptr since it's not an identifier name.

Done.

https://codereview.chromium.org/1133123003/diff/1/ash/metrics/task_switch_tim...
File ash/metrics/task_switch_time_tracker.cc (right):

https://codereview.chromium.org/1133123003/diff/1/ash/metrics/task_switch_tim...
ash/metrics/task_switch_time_tracker.cc:27: tick_clock_(new
base::DefaultTickClock()) {
On 2015/05/13 19:27:18, tdanderson wrote:
> Are you purposely not initializing |last_action_time_| to the zero time in the
> constructor?

I added an explicit in-class initialization.

https://codereview.chromium.org/1133123003/diff/1/ash/metrics/task_switch_tim...
ash/metrics/task_switch_time_tracker.cc:59: 
On 2015/05/13 19:27:18, tdanderson wrote:
> Maybe have a CHECK() that the delta >= 0 ?

Done.

https://codereview.chromium.org/1133123003/diff/1/ash/metrics/task_switch_tim...
File ash/metrics/task_switch_time_tracker.h (right):

https://codereview.chromium.org/1133123003/diff/1/ash/metrics/task_switch_tim...
ash/metrics/task_switch_time_tracker.h:48: // Records the |last_action_time_|
and returns the previous value for
On 2015/05/13 19:27:18, tdanderson wrote:
> nit: specify that we're setting the member to be the current system time.

Done.

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

https://codereview.chromium.org/1133123003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:736: +    The number of seconds between
contiguous task switch user actions triggered
On 2015/05/13 19:27:18, tdanderson wrote:
> Perhaps clarify somehow that "contiguous" still means that other task switches
> could take place in between, i.e., tapping the shelf, then tapping the
tabstrip,
> then tapping the shelf again will record an entry here.

Updated.  I tried my best to be clear...

[tdanderson, 2015-05-13 21:19:57]

new description LGTM

[oshima, 2015-05-13 22:16:53]

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
File ash/metrics/task_switch_metrics_recorder.cc (right):

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:21: }
NOTREACHED()

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:55: CHECK(histogram_name);
If you assume wrong source value results in nullptr, then you need to return
nullptr in GetHistogramName, otherwise you'll get garbage.

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
File ash/metrics/task_switch_metrics_recorder.h (right):

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.h:26: kShelf
I assume you're going to add more?

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
File ash/metrics/task_switch_time_tracker.cc (right):

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_time_tracker.cc:21: const base::TimeDelta kMaxBucket =
base::TimeDelta::FromHours(1);
won't they create static initializer? (It's banned in chromium. see
http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html)

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_time_tracker.cc:72:
histogram_->CheckName(histogram_name_);
this leaves the code in release build. can you use #if?

[bruthig, 2015-05-13 23:41:10]

I've addressed oshima@'s comments from patch set 2.

asvitkine@/rkaplow@: Can you please review my usage of histograms.  Specifically
in:
 - ash/metrics/task_switch_time_tracker.cc
 - tools/metrics/histograms/histograms.xml

oshima@: Can you please re-review from an owner's perspective?
 - ash/*

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
File ash/metrics/task_switch_metrics_recorder.cc (right):

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:21: }
On 2015/05/13 22:16:52, oshima wrote:
> NOTREACHED()

Done.

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:55: CHECK(histogram_name);
On 2015/05/13 22:16:52, oshima wrote:
> If you assume wrong source value results in nullptr, then you need to return
> nullptr in GetHistogramName, otherwise you'll get garbage.

Done.

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
File ash/metrics/task_switch_metrics_recorder.h (right):

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.h:26: kShelf
On 2015/05/13 22:16:52, oshima wrote:
> I assume you're going to add more? 

Correct.

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
File ash/metrics/task_switch_time_tracker.cc (right):

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_time_tracker.cc:21: const base::TimeDelta kMaxBucket =
base::TimeDelta::FromHours(1);
On 2015/05/13 22:16:52, oshima wrote:
> won't they create static initializer? (It's banned in chromium. see
> http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html)

Done.

https://codereview.chromium.org/1133123003/diff/20001/ash/metrics/task_switch...
ash/metrics/task_switch_time_tracker.cc:72:
histogram_->CheckName(histogram_name_);
On 2015/05/13 22:16:52, oshima wrote:
> this leaves the code in release build. can you use #if?

Done.

[oshima, 2015-05-13 23:46:50]

lgtm with a nit

https://codereview.chromium.org/1133123003/diff/40001/ash/metrics/task_switch...
File ash/metrics/task_switch_metrics_recorder.cc (right):

https://codereview.chromium.org/1133123003/diff/40001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:57: CHECK(histogram_name);
nit: DCHECK will be sufficient unless you want to investigate crashes, or it has
security risk.

[bruthig, 2015-05-14 14:19:46]

I've addressed oshima@'s comments from patch set 3.

asvitkine@/rkaplow@: Can you please review my usage of histograms.  Specifically
in:
 - ash/metrics/task_switch_time_tracker.cc
 - tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/1133123003/diff/40001/ash/metrics/task_switch...
File ash/metrics/task_switch_metrics_recorder.cc (right):

https://codereview.chromium.org/1133123003/diff/40001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:57: CHECK(histogram_name);
On 2015/05/13 23:46:50, oshima wrote:
> nit: DCHECK will be sufficient unless you want to investigate crashes, or it
has
> security risk. 

Done.

[Alexei Svitkine (slow), 2015-05-14 14:51:54]

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
File ash/metrics/task_switch_metrics_recorder.cc (right):

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:13: const char kShelf_histogram[] =
"Ash.Shelf.TimeBetweenNavigateToTaskSwitches";
Nit: kShelfHistogram

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:60:
scoped_ptr<TaskSwitchTimeTracker>(
make_scoped_ptr?

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
File ash/metrics/task_switch_time_tracker.cc (right):

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
ash/metrics/task_switch_time_tracker.cc:14: // The number of buckets in the
histogram.
Please add a comment somewhere that changing any of these values requires
renaming all the histograms using this mechanism.

[bruthig, 2015-05-14 15:35:21]

I have addressed asvitkine@ comments in patch set 4.

asvitkine@/rkaplow@: Can you please re-review my usage of histograms. 
Specifically
in:
 - ash/metrics/task_switch_time_tracker.cc
 - tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
File ash/metrics/task_switch_metrics_recorder.cc (right):

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:13: const char kShelf_histogram[] =
"Ash.Shelf.TimeBetweenNavigateToTaskSwitches";
On 2015/05/14 14:51:54, Alexei Svitkine wrote:
> Nit: kShelfHistogram

Done.

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
ash/metrics/task_switch_metrics_recorder.cc:60:
scoped_ptr<TaskSwitchTimeTracker>(
On 2015/05/14 14:51:54, Alexei Svitkine wrote:
> make_scoped_ptr?

Done.

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
File ash/metrics/task_switch_time_tracker.cc (right):

https://codereview.chromium.org/1133123003/diff/60001/ash/metrics/task_switch...
ash/metrics/task_switch_time_tracker.cc:14: // The number of buckets in the
histogram.
On 2015/05/14 14:51:54, Alexei Svitkine wrote:
> Please add a comment somewhere that changing any of these values requires
> renaming all the histograms using this mechanism.

Done.

[Alexei Svitkine (slow), 2015-05-14 15:53:32]

lgtm

[bruthig, 2015-05-14 16:07:13]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-05-14 16:07:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1133123003/#ps80001
(title: "Addressed asvitkine@ comments in patch set 4.")

[commit-bot: I haz the power, 2015-05-14 16:08:22]

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

[commit-bot: I haz the power, 2015-05-14 16:51:01]

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

[commit-bot: I haz the power, 2015-05-14 16:51:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bruthig, 2015-05-14 17:29:45]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-05-14 17:29:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, oshima@chromium.org, tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1133123003/#ps100001
(title: "Added ASH_EXPORT to TaskSwitchTimeTracker.")

[commit-bot: I haz the power, 2015-05-14 17:31:40]

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

[commit-bot: I haz the power, 2015-05-14 18:36:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-14 18:37:47]

Patchset 6 (id:??) landed as
https://crrev.com/9444f5f41b7cdea355dac95e0de83573b4e31a3d
Cr-Commit-Position: refs/heads/master@{#329884}