Implement media playing engagement detection for the site engagement service.

chrome/browser/engagement/site_engagement_helper.h

Index: chrome/browser/engagement/site_engagement_helper.h
diff --git a/chrome/browser/engagement/site_engagement_helper.h b/chrome/browser/engagement/site_engagement_helper.h
index 9ba18079767d2f56a099f6eaf6c4a384dd671377..9c71b8e0ae95fa2e710f98664bf10ecf3c85d21d 100644
--- a/chrome/browser/engagement/site_engagement_helper.h
+++ b/chrome/browser/engagement/site_engagement_helper.h
@@ -37,22 +37,20 @@ class SiteEngagementHelper
   void WasHidden() override;
 
  private:
-  // Class to encapsulate the user input listening.
+  // Class to encapsulate the detection of site engagement.
   //
-  // Time on site is recorded by detecting any user input (mouse click,
-  // keypress, or touch gesture tap) per some discrete time unit. If there is
-  // user input, then record a positive site engagement.
+  // All engagement detection begins at some constant time delta following
+  // navigation or tab activation. Once engagement is recorded, detection is
+  // suspended for another constant time delta. For sites to continually record
+  // engagement, this overall design requires:
   //
-  // When input is detected, SiteEngagementHelper::RecordUserInput is called,
-  // and the input detection callbacks are paused for
-  // g_seconds_between_user_input_check. This ensures that there is minimal
-  // overhead in input listening, and that input over an extended length of time
-  // is required to continually increase the engagement score.
-  class InputTracker : public content::WebContentsObserver {
+  // 1. engagement at a non-trivial time after a site loads
+  // 2. continual engagement over a non-trivial length of time
+  class EngagementTracker : public content::WebContentsObserver {

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

I think the subclasses should be made into WebContentsObservers rather than this
base class.

[dominickn, 2015/11/03 07:03:54]

Done.

    public:
-    explicit InputTracker(content::WebContents* web_contents,
-                          SiteEngagementHelper* helper);
-    ~InputTracker() override;
+    explicit EngagementTracker(content::WebContents* web_contents,

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

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?

[dominickn, 2015/11/03 07:03:54]

Great call. Done.

+                               SiteEngagementHelper* helper);
+    ~EngagementTracker() override;
 
     // Begin tracking input after |initial_delay|.
     void Start(base::TimeDelta initial_delay);
@@ -70,32 +68,74 @@ class SiteEngagementHelper
     // Set the timer object for testing purposes.
     void SetPauseTimerForTesting(scoped_ptr<base::Timer> timer);
 
-   private:
+   protected:
     friend class SiteEngagementHelperTest;
 
     // Starts the timer for detecting user interaction.

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

nit: Update all comments.

[dominickn, 2015/11/03 07:03:54]

Done.

     void StartTimer(base::TimeDelta delay);
 
     // Callback for StartTimer that activates the user input tracking.
-    void StartTracking();
-
-    // content::WebContentsObserver overrides.
-    void DidGetUserInteraction(const blink::WebInputEvent::Type type) override;
+    virtual void StartTracking();
 

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

Data members should be private. Add accessors as necessary.

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

[dominickn, 2015/11/03 07:03:54]

Done.

     SiteEngagementHelper* helper_;
     scoped_ptr<base::Timer> pause_timer_;
     bool is_tracking_;
   };
 
+  // Class to encapsulate time-on-site engagement. Time-on-site is recorded by
+  // detecting user input on a focused WebContents (mouse click, keypress, or
+  // touch gesture tap) over time.

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

Comments for these concrete tracker classes should also specify the interval
behavior.

[dominickn, 2015/11/03 07:03:54]

Done.

+  class InputTracker : public EngagementTracker {
+   public:
+    explicit InputTracker(content::WebContents* web_contents,

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

nit: explicit only for single argument constructors (and elsewhere).

[dominickn, 2015/11/03 07:03:54]

Done.

+                          SiteEngagementHelper* helper);
+
+   private:
+    friend class SiteEngagementHelperTest;
+
+    // content::WebContentsObserver overrides.
+    void DidGetUserInteraction(const blink::WebInputEvent::Type type) override;
+  };
+
+  // Class to encapsulate media detection. Any media playing in a WebContents
+  // (focused or not) will accumulate engagement points. Media in a hidden
+  // WebContents will accumulate engagement more slowly than in an active
+  // WebContents. Media which has been muted will also accumulate engagement
+  // more slowly.
+  class MediaTracker : public EngagementTracker {
+   public:
+    explicit MediaTracker(content::WebContents* web_contents,
+                          SiteEngagementHelper* helper);
+
+   private:
+    friend class SiteEngagementHelperTest;
+
+    void StartTracking() override;
+
+    // content::WebContentsObserver overrides.
+    void MediaStartedPlaying() override;
+    void MediaPaused() override;
+    void WasShown() override;
+    void WasHidden() override;
+
+    bool is_hidden_;
+    bool is_playing_;
+  };
+
   explicit SiteEngagementHelper(content::WebContents* web_contents);
   friend class content::WebContentsUserData<SiteEngagementHelper>;
   friend class SiteEngagementHelperTest;
 
   // Ask the SiteEngagementService to record engagement via user input at the
-  // current contents location.
+  // current WebContents URL.
   void RecordUserInput(SiteEngagementMetrics::EngagementType type);
 
+  // Ask the SiteEngagementService to record engagement via media playing at the
+  // current WebContents URL.
+  void RecordMediaPlaying(bool is_hidden);
+
   InputTracker input_tracker_;
+  MediaTracker media_tracker_;
   bool record_engagement_;
 
   DISALLOW_COPY_AND_ASSIGN(SiteEngagementHelper);