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

chrome/browser/engagement/site_engagement_helper.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "chrome/browser/profiles/profile.h"
 #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)
+  // Ignore pre-render loads and all schemes except HTTP and HTTPS.
+  ignore_page_ = (prerender_contents != NULL || !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);
 
-  // Ignore all schemes except HTTP and HTTPS.
-  if (!url.SchemeIsHTTPOrHTTPS())
-    return;
+      // Service is null in incognito.
+      if (service)
+        service->HandleUserInput(url);
+      RegisterInputCallbacks(contents->GetRenderViewHost());
+    }
+  }
+  user_input_recorded_ = false;
+}
 
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-  SiteEngagementService* service =
-      SiteEngagementServiceFactory::GetForProfile(profile);
-  // Service is null in incognito.
-  if (!service)
-    return;
+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);
+}
 
-  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::WasShown() {
+  // When we come back into view, ensure that the input listening callbacks are
+  // registered.
+  user_input_recorded_ = false;
+  RegisterInputCallbacks(web_contents()->GetRenderViewHost());
+  StartTimer();
 }
+
+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());
+}