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..535e19ed46f0e51d751d646aaf271177e2fc7da9 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,121 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 
+namespace {
+
+const int kSecondsBetweenUserInputCheck = 10;

[calamity, 2015/09/11 07:19:56]

We're going to want some noise on top of this. Should check with security
people.

+
+}  // anonymous namespace
+
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(SiteEngagementHelper);
 
 SiteEngagementHelper::~SiteEngagementHelper() {
+  timer_.Stop();
+  content::WebContents* contents = web_contents();
+  if (contents) {
+    contents->GetRenderViewHost()->RemoveKeyPressEventCallback(
+        key_press_event_callback_);
+    contents->GetRenderViewHost()->RemoveMouseEventCallback(
+        mouse_event_callback_);
+  }
 }
 
 SiteEngagementHelper::SiteEngagementHelper(content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents) {
+    : content::WebContentsObserver(web_contents),
+      user_input_recorded_(false),
+      callbacks_added_(true) {
+  key_press_event_callback_ = base::Bind(
+      &SiteEngagementHelper::HandleKeyPressEvent, base::Unretained(this));
+  mouse_event_callback_ = base::Bind(&SiteEngagementHelper::HandleMouseEvent,
+                                     base::Unretained(this));

[calamity, 2015/09/11 07:19:56]

Probably want a WeakPtrFactory for this.

[dominickn, 2015/09/11 08:03:37]

I can't use a WeakPtrFactory because weak pointers can't be used for functions
with non-void return values (compile failure).

[calamity, 2015/09/14 02:08:33]

Lame. But this makes sense. The comment in bind_internal.h says that it just
wants you to have a return value so there's no undefined behavior.

You can get around this by making a private static method that looks like bool
HandleInputEvent(WeakPtr<SiteEngagementHelper> p, const NativeWebKeyboardEvent&)
and then just check if |p| is true for still alive-ness.

+  web_contents->GetRenderViewHost()->AddKeyPressEventCallback(
+      key_press_event_callback_);
+  web_contents->GetRenderViewHost()->AddMouseEventCallback(
+      mouse_event_callback_);
+}
+
+// Record that there was some user input, and defer handling of the input event.
+bool SiteEngagementHelper::HandleKeyPressEvent(
+    const content::NativeWebKeyboardEvent& event) {
+  user_input_recorded_ = true;
+  return false;
+}
+
+bool SiteEngagementHelper::HandleMouseEvent(const blink::WebMouseEvent& event) {
+  user_input_recorded_ = true;
+  return false;
 }
 
-void SiteEngagementHelper::DidStartNavigationToPendingEntry(
-    const GURL& url,
-    content::NavigationController::ReloadType reload_type) {
-  prerender::PrerenderContents* prerender_contents =
-      prerender::PrerenderContents::FromWebContents(web_contents());
+void SiteEngagementHelper::TimerFired() {
+  content::WebContents* contents = web_contents();
+  if (contents) {
+    GURL url = contents->GetVisibleURL();
+    if (user_input_recorded_) {
+      prerender::PrerenderContents* prerender_contents =
+          prerender::PrerenderContents::FromWebContents(contents);
 
-  // Ignore pre-render loads.
-  if (prerender_contents != NULL)
-    return;
+      // Ignore pre-render loads.
+      if (prerender_contents != NULL)
+        return;
 
-  // Ignore all schemes except HTTP and HTTPS.
-  if (!url.SchemeIsHTTPOrHTTPS())
-    return;
+      // Ignore all schemes except HTTP and HTTPS.
+      if (!url.SchemeIsHTTPOrHTTPS())
+        return;

[calamity, 2015/09/11 07:19:56]

These two checks feel like they should be done on navigation rather than on
timer fire.

[dominickn, 2015/09/11 08:03:38]

Done.

 
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-  SiteEngagementService* service =
-      SiteEngagementServiceFactory::GetForProfile(profile);
-  // Service is null in incognito.
-  if (!service)
-    return;
+      Profile* profile =
+          Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+      SiteEngagementService* service =
+          SiteEngagementServiceFactory::GetForProfile(profile);
+      // Service is null in incognito.
+      if (service)
+        service->HandleUserInput(url);
+    }
+  }
+  user_input_recorded_ = false;
+}
+
+void SiteEngagementHelper::StartTimer() {
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromSeconds(kSecondsBetweenUserInputCheck),
+               this, &SiteEngagementHelper::TimerFired);
+}
+
+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 && callbacks_added_) {
+    old_host->RemoveKeyPressEventCallback(key_press_event_callback_);
+    old_host->RemoveMouseEventCallback(mouse_event_callback_);
+  }
+  new_host->AddKeyPressEventCallback(key_press_event_callback_);
+  new_host->AddMouseEventCallback(mouse_event_callback_);
+  callbacks_added_ = true;

[calamity, 2015/09/11 07:19:56]

This only needs re-add the callbacks if they were already there, right?

[dominickn, 2015/09/11 08:03:37]

Done.

+}
+
+void SiteEngagementHelper::WasShown() {
+  // When we come back into view, ensure that the input listening callbacks are
+  // registered.
+  if (!callbacks_added_) {
+    content::WebContents* contents = web_contents();
+    contents->GetRenderViewHost()->AddKeyPressEventCallback(
+        key_press_event_callback_);
+    contents->GetRenderViewHost()->AddMouseEventCallback(mouse_event_callback_);
+    callbacks_added_ = true;
+  }
+  StartTimer();
+}
 
-  service->HandleNavigation(url);
+void SiteEngagementHelper::WasHidden() {
+  // Ensure that we are not recording user events for hidden web contents.
+  content::WebContents* contents = web_contents();
+  if (contents) {
+    contents->GetRenderViewHost()->RemoveKeyPressEventCallback(
+        key_press_event_callback_);
+    contents->GetRenderViewHost()->RemoveMouseEventCallback(
+        mouse_event_callback_);

[calamity, 2015/09/11 07:19:56]

It's probably worth making an Add/RemoveCallback(host) private method to do
this.

[dominickn, 2015/09/11 08:03:38]

Done.

+    callbacks_added_ = false;

[calamity, 2015/09/11 07:19:56]

This should move outside the condition.

[dominickn, 2015/09/11 08:03:37]

Done.

+  }
+  timer_.Stop();
 }