Record the fraction of the time without user input - Aura only

Record the fraction of the time without user input - Aura only.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the FractionOfTimeWithoutUserInputRecorder on other platforms in subsequent patches.

BUG=702198
Review-Url: https://codereview.chromium.org/2751403002
Cr-Commit-Position: refs/heads/master@{#459897}
Committed: https://chromium.googlesource.com/chromium/src/+/a3b17964413c5c6630917b8238d6f9681d6bca9e

[tdresser, 2017-03-16 13:57:28]

The CQ bit was checked by tdresser@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 13:57:40]

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

[tdresser, 2017-03-16 14:02:21]

Description was changed from

==========
Record the fraction of the time users spend idle.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

BUG=702198
==========

to

==========
Record the fraction of the time users spend idle.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the IdleFractionRecorder on other platforms in subsequent patches.

BUG=702198
==========

[tdresser, 2017-03-16 14:02:32]

Description was changed from

==========
Record the fraction of the time users spend idle.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the IdleFractionRecorder on other platforms in subsequent patches.

BUG=702198
==========

to

==========
Record the fraction of the time users spend idle - Aura only.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the IdleFractionRecorder on other platforms in subsequent patches.

BUG=702198
==========

[commit-bot: I haz the power, 2017-03-16 14:36:57]

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

[commit-bot: I haz the power, 2017-03-16 14:36:58]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tdresser, 2017-03-17 14:37:31]

The CQ bit was checked by tdresser@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 14:37:47]

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

[tdresser, 2017-03-17 14:37:54]

Description was changed from

==========
Record the fraction of the time users spend idle - Aura only.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the IdleFractionRecorder on other platforms in subsequent patches.

BUG=702198
==========

to

==========
Record the fraction of the time users spend idle - Aura only.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the FractionOfTimeWithoutUserInputRecorder on other platforms in
subsequent patches.

BUG=702198
==========

[commit-bot: I haz the power, 2017-03-17 15:23:07]

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

[commit-bot: I haz the power, 2017-03-17 15:23:08]

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

[tdresser, 2017-03-17 15:36:45]

The CQ bit was checked by tdresser@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 15:37:04]

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

[commit-bot: I haz the power, 2017-03-17 17:31:48]

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

[commit-bot: I haz the power, 2017-03-17 17:31:49]

Dry run: This issue passed the CQ dry run.

[tdresser, 2017-03-17 17:44:39]

Description was changed from

==========
Record the fraction of the time users spend idle - Aura only.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the FractionOfTimeWithoutUserInputRecorder on other platforms in
subsequent patches.

BUG=702198
==========

to

==========
Record the fraction of the time without user input - Aura only.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the FractionOfTimeWithoutUserInputRecorder on other platforms in
subsequent patches.

BUG=702198
==========

[tdresser, 2017-03-17 17:44:52]

tdresser@chromium.org changed reviewers:
+ sadrul@chromium.org

[tdresser, 2017-03-17 17:44:53]

PTAL

[sadrul, 2017-03-19 00:38:52]

https://codereview.chromium.org/2751403002/diff/40001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:499: event->time_stamp());
Do you care whether the event is actually coming from user, or was generated
internally by aura (e.g. if a window moved/closed/opened etc.)? If so, you can
look at the EF_IS_SYNTHESIZED flag.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.cc (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.cc:14: const
base::TimeDelta DEFAULT_IDLE_TIMEOUT =
constexpr

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.cc:34: // duration.
s/with the page/<something more generic, like interacting with the system>/

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.cc:57:
std::max(interval_in_window_duration, base::TimeDelta());
Is this needed? (i.e. can interval_in_window_duration be negative?)

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.h (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.h:27: base::TimeDelta
idle_timeout_;
Move to private, add protected accessors instead.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder_unittest.cc (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:28: void
RecordToUma(float idle_fraction) override {
Instead of making this virtual (and override from here), can you use
HistogramTester (and HistogramTester::GetAllSamples()) instead?

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:33: void
SetWindowSize(base::TimeDelta window_size) {
set_window_size

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:37: void
SetIdleTimeout(base::TimeDelta idle_timeout) {
set_idle_timeout

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:41:
std::vector<std::pair<base::TimeTicks, base::TimeTicks>> active_intervals() {
const&

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:45:
std::vector<float> idle_fractions() { return idle_fractions_; }
ditto

[tdresser, 2017-03-24 14:56:31]

Thanks!
PTAL

https://codereview.chromium.org/2751403002/diff/40001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:499: event->time_stamp());
On 2017/03/19 00:38:51, sadrul wrote:
> Do you care whether the event is actually coming from user, or was generated
> internally by aura (e.g. if a window moved/closed/opened etc.)? If so, you can
> look at the EF_IS_SYNTHESIZED flag.

Done.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.cc (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.cc:14: const
base::TimeDelta DEFAULT_IDLE_TIMEOUT =
On 2017/03/19 00:38:51, sadrul wrote:
> constexpr

From https://chromium-cpp.appspot.com/

"Prefer to const for variables where possible."

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.cc:34: // duration.
On 2017/03/19 00:38:51, sadrul wrote:
> s/with the page/<something more generic, like interacting with the system>/

Went with "browser".

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.cc:57:
std::max(interval_in_window_duration, base::TimeDelta());
On 2017/03/19 00:38:51, sadrul wrote:
> Is this needed? (i.e. can interval_in_window_duration be negative?)

We're computing how much of the task is inside the current window, but if the
task starts after the current window ends, this will be negative. In this
situation, the interval end time will be greater than the window end time, and
we'll advance the window.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.h (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.h:27: base::TimeDelta
idle_timeout_;
I'm modifying these from the unit test (to keep our tests from failing when we
tweak these constants).

To make these members private, I'd need to add protected getters & setters, and
then expose the getters publicly on TestFractionOfTimeWithoutUserInputRecorder,
which feels like a lot of overhead for very little value.

Do you think this is worth doing?

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder_unittest.cc (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:28: void
RecordToUma(float idle_fraction) override {
On 2017/03/19 00:38:51, sadrul wrote:
> Instead of making this virtual (and override from here), can you use
> HistogramTester (and HistogramTester::GetAllSamples()) instead?

Done.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:33: void
SetWindowSize(base::TimeDelta window_size) {
On 2017/03/19 00:38:52, sadrul wrote:
> set_window_size

Done.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:37: void
SetIdleTimeout(base::TimeDelta idle_timeout) {
On 2017/03/19 00:38:52, sadrul wrote:
> set_idle_timeout

Done.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:41:
std::vector<std::pair<base::TimeTicks, base::TimeTicks>> active_intervals() {
On 2017/03/19 00:38:52, sadrul wrote:
> const&

Done.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:45:
std::vector<float> idle_fractions() { return idle_fractions_; }
On 2017/03/19 00:38:52, sadrul wrote:
> ditto

Removed.

[sadrul, 2017-03-24 17:29:33]

lgtm

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.cc (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.cc:14: const
base::TimeDelta DEFAULT_IDLE_TIMEOUT =
On 2017/03/24 14:56:30, tdresser wrote:
> On 2017/03/19 00:38:51, sadrul wrote:
> > constexpr
> 
> From https://chromium-cpp.appspot.com/
> 
> "Prefer to const for variables where possible."

I read that as 'Prefer constexpr to const'. Without a constexpr, you are adding
a static initializer.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.h (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.h:27: base::TimeDelta
idle_timeout_;
On 2017/03/24 14:56:31, tdresser wrote:
> I'm modifying these from the unit test (to keep our tests from failing when we
> tweak these constants).
> 
> To make these members private, I'd need to add protected getters & setters,
and
> then expose the getters publicly on
TestFractionOfTimeWithoutUserInputRecorder,
> which feels like a lot of overhead for very little value.
> 
> Do you think this is worth doing?

I don't really care strongly either way. (although I think the style-guide did
not/does not have an exception for this)

https://codereview.chromium.org/2751403002/diff/60001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.h (right):

https://codereview.chromium.org/2751403002/diff/60001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.h:31: virtual void
RecordToUma(float idle_fraction) const;
This no longer needs to be virtual.

[tdresser, 2017-03-27 18:59:26]

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.cc (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.cc:14: const
base::TimeDelta DEFAULT_IDLE_TIMEOUT =
On 2017/03/24 17:29:33, sadrul wrote:
> On 2017/03/24 14:56:30, tdresser wrote:
> > On 2017/03/19 00:38:51, sadrul wrote:
> > > constexpr
> > 
> > From https://chromium-cpp.appspot.com/
> > 
> > "Prefer to const for variables where possible."
> 
> I read that as 'Prefer constexpr to const'. Without a constexpr, you are
adding
> a static initializer.

Whoops, yeah, I repeatedly misread that. Thanks.

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.h (right):

https://codereview.chromium.org/2751403002/diff/40001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.h:27: base::TimeDelta
idle_timeout_;
On 2017/03/24 17:29:33, sadrul wrote:
> On 2017/03/24 14:56:31, tdresser wrote:
> > I'm modifying these from the unit test (to keep our tests from failing when
we
> > tweak these constants).
> > 
> > To make these members private, I'd need to add protected getters & setters,
> and
> > then expose the getters publicly on
> TestFractionOfTimeWithoutUserInputRecorder,
> > which feels like a lot of overhead for very little value.
> > 
> > Do you think this is worth doing?
> 
> I don't really care strongly either way. (although I think the style-guide did
> not/does not have an exception for this)

Done.

https://codereview.chromium.org/2751403002/diff/60001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder.h (right):

https://codereview.chromium.org/2751403002/diff/60001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder.h:31: virtual void
RecordToUma(float idle_fraction) const;
On 2017/03/24 17:29:33, sadrul wrote:
> This no longer needs to be virtual.

Done.

[tdresser, 2017-03-27 19:01:46]

tdresser@chromium.org changed reviewers:
+ isherman@chromium.org

[tdresser, 2017-03-27 19:01:46]

+isherman@ for histograms.xml.

https://codereview.chromium.org/2751403002/diff/80001/ui/events/fraction_of_t...
File ui/events/fraction_of_time_without_user_input_recorder_unittest.cc (right):

https://codereview.chromium.org/2751403002/diff/80001/ui/events/fraction_of_t...
ui/events/fraction_of_time_without_user_input_recorder_unittest.cc:32: void
set_window_size_for_test(base::TimeDelta window_size) {
sadrul: is this naming okay?

[Ilya Sherman, 2017-03-27 19:07:57]

Metrics LGTM % a nit:

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

https://codereview.chromium.org/2751403002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:15310: +<histogram
name="Event.FractionOfTimeWithoutUserInput">
nit: Please add a units attribute.

[tdresser, 2017-03-27 19:34:27]

Thanks.

I'll land this, Sadrul, if you disagree with setter naming, I can land a fix in
a followup.

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

https://codereview.chromium.org/2751403002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:15310: +<histogram
name="Event.FractionOfTimeWithoutUserInput">
On 2017/03/27 19:07:57, Ilya Sherman wrote:
> nit: Please add a units attribute.

Done.

[tdresser, 2017-03-27 19:34:32]

The CQ bit was checked by tdresser@chromium.org

[tdresser, 2017-03-27 19:34:33]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2751403002/#ps100001
(title: "Add units to histogram")

[commit-bot: I haz the power, 2017-03-27 19:35:41]

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

[commit-bot: I haz the power, 2017-03-27 22:01:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490643272797070,
"parent_rev": "bf6fed8ee477361932688fd9a5eb75039848e294", "commit_rev":
"a3b17964413c5c6630917b8238d6f9681d6bca9e"}

[commit-bot: I haz the power, 2017-03-27 22:02:21]

Description was changed from

==========
Record the fraction of the time without user input - Aura only.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the FractionOfTimeWithoutUserInputRecorder on other platforms in
subsequent patches.

BUG=702198
==========

to

==========
Record the fraction of the time without user input - Aura only.

We don't have a good sense of how long Chrome sits idle, without any
input. Gather this via UMA.

I'll plumb up the FractionOfTimeWithoutUserInputRecorder on other platforms in
subsequent patches.

BUG=702198

Review-Url: https://codereview.chromium.org/2751403002
Cr-Commit-Position: refs/heads/master@{#459897}
Committed:
https://chromium.googlesource.com/chromium/src/+/a3b17964413c5c6630917b8238d6...
==========

[commit-bot: I haz the power, 2017-03-27 22:02:22]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a3b17964413c5c6630917b8238d6...