Start recording background video watch time.

Start recording background video watch time.

YouTube data suggests background video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

This change also cleans up how watch time metrics are stored inside the
MediaInternals class as the number of histograms continues to grow. We
were lacking tests for this portion of watch time reporting, so those
have been added as well.

There are a couple of minor fixes to the WatchTimeReporter which should
only affect tests, but may result in small watch time increases as samples
are recorded earlier than previously (i.e. at player destruction versus
process destruction). 

BUG=none
TEST=new tests.

Review-Url: https://codereview.chromium.org/2780533004
Cr-Commit-Position: refs/heads/master@{#461321}
Committed: https://chromium.googlesource.com/chromium/src/+/c45b1c4ef18fa397013d1109dd9abe1f01543b3c

[DaleCurtis, 2017-03-31 01:05:02]

Patchset #2 (id:20001) has been deleted

[DaleCurtis, 2017-03-31 01:05:07]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2017-03-31 01:06:29]

Description was changed from

==========
Record backgrounded video watch time as audio only watch time.

Data suggests backgrounded video watch time is significant, so start
counting this within the audio only buckets.

BUG=
==========

to

==========
Record backgrounded video watch time as audio only watch time.

Data suggests backgrounded video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

BUG=tbd
TEST=new tests.
==========

[DaleCurtis, 2017-03-31 01:06:29]

dalecurtis@chromium.org changed reviewers:
+ holte@chromium.org, sandersd@chromium.org

[DaleCurtis, 2017-03-31 01:07:23]

Description was changed from

==========
Record backgrounded video watch time as audio only watch time.

Data suggests backgrounded video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

BUG=tbd
TEST=new tests.
==========

to

==========
Record backgrounded video watch time as audio only watch time.

Data suggests backgrounded video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

This change also cleans up how watch time metrics are stored inside the
MediaInternals class as the number of histograms continues to grow. We
were lacking tests for this portion of watch time reporting, so those
have been added as well.

BUG=tbd
TEST=new tests.
==========

[DaleCurtis, 2017-03-31 01:08:35]

holte@ for histograms.xml and media_internals_unittest.cc usage of
base::HistogramTester.
sandersd@ for everything.

I'm still writing more tests so am looking mostly for design feedback on the
WatchTimeReporter and MediaInternals changes. Thanks!

[Steven Holte, 2017-03-31 01:15:07]

lgtm

[sandersd (OOO until July 31), 2017-03-31 20:51:06]

Please change commit title to describe the behavior rather than the
implementation.

LGTM!

https://codereview.chromium.org/2780533004/diff/40001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/2780533004/diff/40001/content/browser/media/m...
content/browser/media/media_internals.cc:337: for (auto key :
watch_time_power_keys_) {
Recommend against using 'auto key' when the iterable is a member variable. It's
not so bad here where the declaration is only a few lines later, but it's still
not easy to read.

https://codereview.chromium.org/2780533004/diff/40001/content/browser/media/m...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/2780533004/diff/40001/content/browser/media/m...
content/browser/media/media_internals_unittest.cc:359: void
CycleReportingTimer() {
Seems like a misleading name since no timer is involved.

[DaleCurtis, 2017-04-01 00:39:53]

Description was changed from

==========
Record backgrounded video watch time as audio only watch time.

Data suggests backgrounded video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

This change also cleans up how watch time metrics are stored inside the
MediaInternals class as the number of histograms continues to grow. We
were lacking tests for this portion of watch time reporting, so those
have been added as well.

BUG=tbd
TEST=new tests.
==========

to

==========
Start recording background video watch time.

YouTube data suggests background video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

This change also cleans up how watch time metrics are stored inside the
MediaInternals class as the number of histograms continues to grow. We
were lacking tests for this portion of watch time reporting, so those
have been added as well.

There are a couple of minor fixes to the WatchTimeReporter which should
only affect tests, but may result in small watch time increases as samples
are recorded earlier than previously (i.e. at player destruction versus
process destruction). 

BUG=tbd
TEST=new tests.
==========

[DaleCurtis, 2017-04-01 00:40:01]

Description was changed from

==========
Start recording background video watch time.

YouTube data suggests background video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

This change also cleans up how watch time metrics are stored inside the
MediaInternals class as the number of histograms continues to grow. We
were lacking tests for this portion of watch time reporting, so those
have been added as well.

There are a couple of minor fixes to the WatchTimeReporter which should
only affect tests, but may result in small watch time increases as samples
are recorded earlier than previously (i.e. at player destruction versus
process destruction). 

BUG=tbd
TEST=new tests.
==========

to

==========
Start recording background video watch time.

YouTube data suggests background video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

This change also cleans up how watch time metrics are stored inside the
MediaInternals class as the number of histograms continues to grow. We
were lacking tests for this portion of watch time reporting, so those
have been added as well.

There are a couple of minor fixes to the WatchTimeReporter which should
only affect tests, but may result in small watch time increases as samples
are recorded earlier than previously (i.e. at player destruction versus
process destruction). 

BUG=none
TEST=new tests.
==========

[DaleCurtis, 2017-04-01 00:51:34]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2017-04-01 00:51:35]

Thanks for review. Comments addressed, and way more tests added.

https://codereview.chromium.org/2780533004/diff/40001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/2780533004/diff/40001/content/browser/media/m...
content/browser/media/media_internals.cc:337: for (auto key :
watch_time_power_keys_) {
On 2017/03/31 at 20:51:06, sandersd wrote:
> Recommend against using 'auto key' when the iterable is a member variable.
It's not so bad here where the declaration is only a few lines later, but it's
still not easy to read.

Changed to power_key.

https://codereview.chromium.org/2780533004/diff/40001/content/browser/media/m...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/2780533004/diff/40001/content/browser/media/m...
content/browser/media/media_internals_unittest.cc:359: void
CycleReportingTimer() {
On 2017/03/31 at 20:51:06, sandersd wrote:
> Seems like a misleading name since no timer is involved.

Renamed to CycleWatchTimeReporter().

[DaleCurtis, 2017-04-01 00:51:36]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2780533004/#ps60001
(title: "Add moar tests.")

[commit-bot: I haz the power, 2017-04-01 00:52:00]

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-04-01 01:21:49]

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

[commit-bot: I haz the power, 2017-04-01 01:21:50]

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

[DaleCurtis, 2017-04-01 05:25:11]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2017-04-01 05:25:12]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2780533004/#ps80001
(title: "Fix copy-init issues.")

[commit-bot: I haz the power, 2017-04-01 05:25:19]

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-04-01 07:14:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491024311755530,
"parent_rev": "6e189e277bcb69b2736c18a4c7b8b3eaac292f98", "commit_rev":
"c45b1c4ef18fa397013d1109dd9abe1f01543b3c"}

[commit-bot: I haz the power, 2017-04-01 07:15:47]

Description was changed from

==========
Start recording background video watch time.

YouTube data suggests background video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

This change also cleans up how watch time metrics are stored inside the
MediaInternals class as the number of histograms continues to grow. We
were lacking tests for this portion of watch time reporting, so those
have been added as well.

There are a couple of minor fixes to the WatchTimeReporter which should
only affect tests, but may result in small watch time increases as samples
are recorded earlier than previously (i.e. at player destruction versus
process destruction). 

BUG=none
TEST=new tests.
==========

to

==========
Start recording background video watch time.

YouTube data suggests background video watch time is significant, so start
counting this within a new set of Media.AudioVideo.Background buckets.

Background collection is done by creating an internal WatchTimeReporter
that is played/paused when hidden/shown occur. This allows us to keep
the logic for reporting watch time simple.

This change also cleans up how watch time metrics are stored inside the
MediaInternals class as the number of histograms continues to grow. We
were lacking tests for this portion of watch time reporting, so those
have been added as well.

There are a couple of minor fixes to the WatchTimeReporter which should
only affect tests, but may result in small watch time increases as samples
are recorded earlier than previously (i.e. at player destruction versus
process destruction). 

BUG=none
TEST=new tests.

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

[commit-bot: I haz the power, 2017-04-01 07:15:48]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c45b1c4ef18fa397013d1109dd9a...