Implement media playing engagement detection for the site engagement service.

Implement media engagement detection for the site engagement service.

This CL refactors engagement tracking into an abstract base class,
with subclasses for user input and media playing detection. Media
play and pause events are now periodically observed and
contribute to site engagement.

BUG=464234
Committed: https://crrev.com/0f08350dc3f91ba6a06665c3e00feb1d6afd469e
Cr-Commit-Position: refs/heads/master@{#358254}

[dominickn, 2015-11-03 02:29:10]

Description was changed from

==========
Implement media playing engagement detection for the site engagement service.

BUG=464234
==========

to

==========
Implement media engagement detection for the site engagement service.

This CL refactors engagement tracking into an abstract base class, with
subclasses for user input and media playing detection. Media play and pause
events are now periodically observed and contribute to site engagement.

BUG=464234
==========

[dominickn, 2015-11-03 02:29:35]

Description was changed from

==========
Implement media engagement detection for the site engagement service.

This CL refactors engagement tracking into an abstract base class, with
subclasses for user input and media playing detection. Media play and pause
events are now periodically observed and contribute to site engagement.

BUG=464234
==========

to

==========
Implement media engagement detection for the site engagement service.

This CL refactors engagement tracking into an abstract base class,
with subclasses for user input and media playing detection. Media
play and pause events are now periodically observed and
contribute to site engagement.

BUG=464234
==========

[dominickn, 2015-11-03 02:29:35]

dominickn@chromium.org changed reviewers:
+ calamity@chromium.org

[dominickn, 2015-11-03 02:30:05]

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

[dominickn, 2015-11-03 02:30:06]

calamity: please review site engagement

isherman: please review histograms.

Thanks!

[Ilya Sherman, 2015-11-03 02:36:34]

histograms lgtm

[calamity, 2015-11-03 04:49:46]

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_helper.cc (right):

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.cc:118: Pause();
Okay, now that this is checking on a set interval, we _definitely_ want some
noise on this delay (follow up CL work I reckon).

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.cc:177: }
nit: remove curlies.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_helper.h (right):

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:49: class EngagementTracker :
public content::WebContentsObserver {
I think the subclasses should be made into WebContentsObservers rather than this
base class.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:51: explicit
EngagementTracker(content::WebContents* web_contents,
This class is not so much a generic engagement tracker as it is some sort of
delayed, pausable tracker. I don't think the abstractions are right here. In
particular, is_tracking() will never mean anything for the media tracker because
it's immediately set to false when the timer is paused.

I think we should move is_tracking to the InputTracker. Essentially, this base
class manages our timer logic so subclasses should be hooking into the different
events of the timer. So, we could add a protected StopTracking() and call it
wherever is_tracking_ gets set to false.

WDYT?

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:74: // Starts the timer for
detecting user interaction.
nit: Update all comments.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:79: 
Data members should be private. Add accessors as necessary.

https://google.github.io/styleguide/cppguide.html#Inheritance

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:87: // touch gesture tap)
over time.
Comments for these concrete tracker classes should also specify the interval
behavior.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:90: explicit
InputTracker(content::WebContents* web_contents,
nit: explicit only for single argument constructors (and elsewhere).

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:148: g_decay_points =
decay_points;
Need to add controls for the points here (either now or in a follow up).

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:666:
SiteEngagementMetrics::ENGAGEMENT_WHEEL, 1);
If this gets any bigger, we should just make a utility method that takes a
map<EngagementType, int> and checks the numbers.

[dominickn, 2015-11-03 07:03:55]

Thanks!

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_helper.cc (right):

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.cc:118: Pause();
On 2015/11/03 04:49:45, calamity wrote:
> Okay, now that this is checking on a set interval, we _definitely_ want some
> noise on this delay (follow up CL work I reckon).

Acknowledged.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.cc:177: }
On 2015/11/03 04:49:45, calamity wrote:
> nit: remove curlies.

Done.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_helper.h (right):

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:49: class EngagementTracker :
public content::WebContentsObserver {
On 2015/11/03 04:49:46, calamity wrote:
> I think the subclasses should be made into WebContentsObservers rather than
this
> base class.

Done.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:51: explicit
EngagementTracker(content::WebContents* web_contents,
On 2015/11/03 04:49:46, calamity wrote:
> This class is not so much a generic engagement tracker as it is some sort of
> delayed, pausable tracker. I don't think the abstractions are right here. In
> particular, is_tracking() will never mean anything for the media tracker
because
> it's immediately set to false when the timer is paused.
> 
> I think we should move is_tracking to the InputTracker. Essentially, this base
> class manages our timer logic so subclasses should be hooking into the
different
> events of the timer. So, we could add a protected StopTracking() and call it
> wherever is_tracking_ gets set to false.
> 
> WDYT?

Great call. Done.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:74: // Starts the timer for
detecting user interaction.
On 2015/11/03 04:49:46, calamity wrote:
> nit: Update all comments.

Done.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:79: 
On 2015/11/03 04:49:46, calamity wrote:
> Data members should be private. Add accessors as necessary.
> 
> https://google.github.io/styleguide/cppguide.html#Inheritance

Done.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:87: // touch gesture tap)
over time.
On 2015/11/03 04:49:46, calamity wrote:
> Comments for these concrete tracker classes should also specify the interval
> behavior.

Done.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:90: explicit
InputTracker(content::WebContents* web_contents,
On 2015/11/03 04:49:46, calamity wrote:
> nit: explicit only for single argument constructors (and elsewhere).

Done.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:148: g_decay_points =
decay_points;
On 2015/11/03 04:49:46, calamity wrote:
> Need to add controls for the points here (either now or in a follow up).

Done. Don't know how I missed this.

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/1427913002/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:666:
SiteEngagementMetrics::ENGAGEMENT_WHEEL, 1);
On 2015/11/03 04:49:46, calamity wrote:
> If this gets any bigger, we should just make a utility method that takes a
> map<EngagementType, int> and checks the numbers.

Acknowledged.

[calamity, 2015-11-04 02:08:11]

lgtm

https://codereview.chromium.org/1427913002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_helper.h (right):

https://codereview.chromium.org/1427913002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:75: // Called when the timer
expires and engagement tracking is active.
nit: s/active/activated/?

https://codereview.chromium.org/1427913002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1427913002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:39: const char
kHiddenMediaPlayingPointsParam[] = "hidden_media_playing";
nit: These should both have _points suffixed.

[dominickn, 2015-11-04 04:20:32]

Thanks! Will need the dependency patchset to land before sending this one to the
CQ.

https://codereview.chromium.org/1427913002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_helper.h (right):

https://codereview.chromium.org/1427913002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.h:75: // Called when the timer
expires and engagement tracking is active.
On 2015/11/04 02:08:10, calamity wrote:
> nit: s/active/activated/?

Done.

https://codereview.chromium.org/1427913002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1427913002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:39: const char
kHiddenMediaPlayingPointsParam[] = "hidden_media_playing";
On 2015/11/04 02:08:10, calamity wrote:
> nit: These should both have _points suffixed.

Done.

[dominickn, 2015-11-05 01:49:45]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2015-11-05 01:49:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/1427913002/#ps80001
(title: "Addressing nits")

[commit-bot: I haz the power, 2015-11-05 01:50:59]

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

[commit-bot: I haz the power, 2015-11-05 01:58:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-05 01:58:56]

Patchset 5 (id:??) landed as
https://crrev.com/8658a8950b57449582033d6a8085e06fc34aa8c7
Cr-Commit-Position: refs/heads/master@{#357978}

[dominickn, 2015-11-05 12:05:14]

Description was changed from

==========
Implement media engagement detection for the site engagement service.

This CL refactors engagement tracking into an abstract base class,
with subclasses for user input and media playing detection. Media
play and pause events are now periodically observed and
contribute to site engagement.

BUG=464234

Committed: https://crrev.com/8658a8950b57449582033d6a8085e06fc34aa8c7
Cr-Commit-Position: refs/heads/master@{#357978}
==========

to

==========
Implement media engagement detection for the site engagement service.

This CL refactors engagement tracking into an abstract base class,
with subclasses for user input and media playing detection. Media
play and pause events are now periodically observed and
contribute to site engagement.

BUG=464234

Committed: https://crrev.com/8658a8950b57449582033d6a8085e06fc34aa8c7
Cr-Commit-Position: refs/heads/master@{#357978}
==========

[dominickn, 2015-11-06 02:39:32]

Description was changed from

==========
Implement media engagement detection for the site engagement service.

This CL refactors engagement tracking into an abstract base class,
with subclasses for user input and media playing detection. Media
play and pause events are now periodically observed and
contribute to site engagement.

BUG=464234

Committed: https://crrev.com/8658a8950b57449582033d6a8085e06fc34aa8c7
Cr-Commit-Position: refs/heads/master@{#357978}
==========

to

==========
Implement media engagement detection for the site engagement service.

This CL refactors engagement tracking into an abstract base class,
with subclasses for user input and media playing detection. Media
play and pause events are now periodically observed and
contribute to site engagement.

BUG=464234
==========

[dominickn, 2015-11-06 02:39:38]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2015-11-06 02:40:14]

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

[commit-bot: I haz the power, 2015-11-06 04:25:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-06 04:26:00]

Patchset 5 (id:??) landed as
https://crrev.com/0f08350dc3f91ba6a06665c3e00feb1d6afd469e
Cr-Commit-Position: refs/heads/master@{#358254}