Implement a site engagement score based on time-on-site.

chrome/browser/engagement/site_engagement_helper.cc

Index: chrome/browser/engagement/site_engagement_helper.cc
diff --git a/chrome/browser/engagement/site_engagement_helper.cc b/chrome/browser/engagement/site_engagement_helper.cc
index b107edf030c458bb1d9803395d91ab0dbc7652c6..aa523d456c2ee2b27ad933493784c66be1aabd84 100644
--- a/chrome/browser/engagement/site_engagement_helper.cc
+++ b/chrome/browser/engagement/site_engagement_helper.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/engagement/site_engagement_helper.h"
 
+#include "base/time/time.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/engagement/site_engagement_service_factory.h"
 #include "chrome/browser/prerender/prerender_contents.h"
@@ -11,36 +12,132 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 
+namespace {
+
+const int kSecondsBetweenUserInputCheck = 10;
+
+}  // anonymous namespace
+
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(SiteEngagementHelper);
 
 SiteEngagementHelper::~SiteEngagementHelper() {
+  timer_.Stop();
+  content::WebContents* contents = web_contents();
+  if (contents)
+    RemoveInputCallbacks(contents->GetRenderViewHost());
 }
 
 SiteEngagementHelper::SiteEngagementHelper(content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents) {
+    : content::WebContentsObserver(web_contents),
+      user_input_recorded_(false),
+      callbacks_added_(false),
+      ignore_page_(false) {
+  key_press_event_callback_ = base::Bind(
+      &SiteEngagementHelper::HandleKeyPressEvent, base::Unretained(this));
+  mouse_event_callback_ = base::Bind(&SiteEngagementHelper::HandleMouseEvent,
+                                     base::Unretained(this));
+  RegisterInputCallbacks(web_contents->GetRenderViewHost());

[benwells, 2015/09/16 00:23:32]

Should the callbacks be registered here? Won't they be registered in WasShown?

If they do need to eb registered here, shouldn't the timer be started too?

If there is a strong relationship between timer running and callbacks
registered, maybe these should be grouped together into functions like
BeginMonitoring and EndMonitoring, which would control the timer and callback
registration.

[dominickn, 2015/09/18 06:05:27]

There isn't a strong relationship between timer running and callbacks registered
at the moment: callbacks currently unregister themselves at first input to
reduce the overhead of watching for user input. This may change slightly if we
decide to score different input types differently (and this have to listen for
particular types), but I quite like having the callbacks unregister themselves
when they're not needed.

+}
+
+// Record that there was some user input, and defer handling of the input event.
+// web_contents() will return nullptr if the observed contents have been
+// deleted.
+bool SiteEngagementHelper::HandleKeyPressEvent(
+    const content::NativeWebKeyboardEvent& event) {
+  user_input_recorded_ = true;
+  content::WebContents* contents = web_contents();
+  if (contents)
+    RemoveInputCallbacks(contents->GetRenderViewHost());
+  return false;
+}
+
+bool SiteEngagementHelper::HandleMouseEvent(const blink::WebMouseEvent& event) {
+  user_input_recorded_ = true;
+  content::WebContents* contents = web_contents();
+  if (contents)
+    RemoveInputCallbacks(contents->GetRenderViewHost());
+  return false;
+}
+
+void SiteEngagementHelper::RegisterInputCallbacks(
+    content::RenderViewHost* host) {
+  if (!callbacks_added_) {

[benwells, 2015/09/16 00:23:32]

I wonder if this could be encapsulated into some sort of ScopedInputTracker,
which in its constructor sets up the listening and in its destructor kills it.
That way you wouldn't need callbacks_added_, the existence of your
ScopedInputTracker would be signal enough.

[dominickn, 2015/09/18 06:05:27]

I'm concerned whether the additional clarity comes at the overhead of
continually constructing and destructing the object.

+    host->AddKeyPressEventCallback(key_press_event_callback_);
+    host->AddMouseEventCallback(mouse_event_callback_);
+    callbacks_added_ = true;
+  }
+}
+
+void SiteEngagementHelper::RemoveInputCallbacks(
+    content::RenderViewHost* host) {
+  if (callbacks_added_) {
+    host->RemoveKeyPressEventCallback(key_press_event_callback_);
+    host->RemoveMouseEventCallback(mouse_event_callback_);
+    callbacks_added_ = false;
+  }
+}
+
+void SiteEngagementHelper::StartTimer() {
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromSeconds(kSecondsBetweenUserInputCheck),
+               this, &SiteEngagementHelper::TimerFired);
 }
 
 void SiteEngagementHelper::DidStartNavigationToPendingEntry(
     const GURL& url,
     content::NavigationController::ReloadType reload_type) {
+  content::WebContents* contents = web_contents();
   prerender::PrerenderContents* prerender_contents =
-      prerender::PrerenderContents::FromWebContents(web_contents());
+      prerender::PrerenderContents::FromWebContents(contents);
 
-  // Ignore pre-render loads.
-  if (prerender_contents != NULL)
-    return;
+  // Ignore pre-render loads and all schemes except HTTP and HTTPS.
+  ignore_page_ = (prerender_contents != NULL || !url.SchemeIsHTTPOrHTTPS());
+}
 
-  // Ignore all schemes except HTTP and HTTPS.
-  if (!url.SchemeIsHTTPOrHTTPS())
+void SiteEngagementHelper::TimerFired() {
+  if (ignore_page_)

[benwells, 2015/09/16 00:23:32]

I don't really like having this timer running all the time. Can we just start it
after movement / when Chrome isn't idle?

[dominickn, 2015/09/18 06:05:27]

I have changed this design such that the timer is now run after an input
callback is fired. When the timer finishes, the input callbacks are
re-registered. The difference is that instead of checking for user input in
discrete windows, this now enforces that user input must occur some amount of
time apart to record more engagement.

     return;
+  content::WebContents* contents = web_contents();
+  if (contents) {
+    GURL url = contents->GetVisibleURL();
+    if (user_input_recorded_) {
+      Profile* profile =
+          Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+      SiteEngagementService* service =
+          SiteEngagementServiceFactory::GetForProfile(profile);
 
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-  SiteEngagementService* service =
-      SiteEngagementServiceFactory::GetForProfile(profile);
-  // Service is null in incognito.
-  if (!service)
-    return;
+      // Service is null in incognito.
+      if (service)
+        service->HandleUserInput(url);
+      RegisterInputCallbacks(contents->GetRenderViewHost());
+    }
+  }
+  user_input_recorded_ = false;
+}
+
+void SiteEngagementHelper::RenderViewHostChanged(
+    content::RenderViewHost* old_host,
+    content::RenderViewHost* new_host) {
+  // On changing the render view host, we need to re-register the callbacks
+  // listening for user input.
+  if (old_host)
+    RemoveInputCallbacks(old_host);
+  RegisterInputCallbacks(new_host);
+}
+
+void SiteEngagementHelper::WasShown() {
+  // When we come back into view, ensure that the input listening callbacks are
+  // registered.
+  user_input_recorded_ = false;
+  RegisterInputCallbacks(web_contents()->GetRenderViewHost());
+  StartTimer();
+}
 
-  service->HandleNavigation(url);

[benwells, 2015/09/16 00:23:32]

Why is this now removed? I am guessing you're equating the initial engagement on
a site with navigation, but I *think* they are different events and should be
tracked separately.

[dominickn, 2015/09/18 06:05:27]

calamity@ wanted to remove navigation tracking entirely, but following today's
meetings I agree that it's important to keep around.

+void SiteEngagementHelper::WasHidden() {
+  // Ensure that we are not recording user events for hidden web contents.
+  timer_.Stop();
+  user_input_recorded_ = false;
+  content::WebContents* contents = web_contents();
+  if (contents)
+    RemoveInputCallbacks(contents->GetRenderViewHost());
 }