Add a delay to Site Engagement tracking after navigation.

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 "base/trace_event/trace_event.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 {
 
-double g_seconds_between_user_input_check = 10;
+int kSecondsBetweenUserInputCheck = 10;
 
 }  // anonymous namespace
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(SiteEngagementHelper);
 
 SiteEngagementHelper::InputTracker::InputTracker(SiteEngagementHelper* helper)
     : helper_(helper),
       pause_timer_(new base::Timer(true, false)),
       callbacks_added_(false) {
   key_press_event_callback_ =
@@ skipping 43 matching lines @@
     host->AddMouseEventCallback(mouse_event_callback_);
     callbacks_added_ = true;
   }
 }
 
 void SiteEngagementHelper::InputTracker::PauseTracking(
     content::RenderViewHost* host) {
   StopTracking(host);
   pause_timer_->Start(
       FROM_HERE,
-      base::TimeDelta::FromSeconds(g_seconds_between_user_input_check),
+      base::TimeDelta::FromSeconds(kSecondsBetweenUserInputCheck),
       base::Bind(&SiteEngagementHelper::InputTracker::ResumeTracking,
                  base::Unretained(this)));
 }
 
 void SiteEngagementHelper::InputTracker::ResumeTracking() {
   content::WebContents* contents = helper_->web_contents();
   if (contents)
     StartTracking(contents->GetRenderViewHost());
 }
 
 void SiteEngagementHelper::InputTracker::StopTracking(
     content::RenderViewHost* host) {
   pause_timer_->Stop();
   if (callbacks_added_) {
     host->RemoveKeyPressEventCallback(key_press_event_callback_);
     host->RemoveMouseEventCallback(mouse_event_callback_);
     callbacks_added_ = false;
   }
 }
 
-void SiteEngagementHelper::InputTracker::SetTimerForTesting(
+bool SiteEngagementHelper::InputTracker::IsTracking() {
+  return callbacks_added_;
+}
+
+void SiteEngagementHelper::InputTracker::SetPauseTimerForTesting(
     scoped_ptr<base::Timer> timer) {
   pause_timer_ = timer.Pass();
 }
 
 SiteEngagementHelper::~SiteEngagementHelper() {
   content::WebContents* contents = web_contents();
   if (contents)
     input_tracker_.StopTracking(contents->GetRenderViewHost());
 }
 
 SiteEngagementHelper::SiteEngagementHelper(content::WebContents* contents)
     : content::WebContentsObserver(contents),
+      navigation_timer_(new base::Timer(true, false)),
       input_tracker_(this),
-      record_engagement_(false) { }
+      record_engagement_(false) {}
 
 void SiteEngagementHelper::RecordUserInput() {
   TRACE_EVENT0("SiteEngagement", "RecordUserInput");
   content::WebContents* contents = web_contents();
   if (contents) {
     Profile* profile =
         Profile::FromBrowserContext(contents->GetBrowserContext());
     SiteEngagementService* service =
         SiteEngagementServiceFactory::GetForProfile(profile);
 
     // Service is null in incognito.
     if (service)
       service->HandleUserInput(contents->GetVisibleURL());
   }
 }
 
-bool SiteEngagementHelper::ShouldRecordEngagement() {
-  return record_engagement_;
-}
-
 void SiteEngagementHelper::DidNavigateMainFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
   content::WebContents* contents = web_contents();
   input_tracker_.StopTracking(contents->GetRenderViewHost());
+  navigation_timer_->Stop();
+
+  record_engagement_ = false;
 
   // Ignore all schemes except HTTP and HTTPS.
-  record_engagement_ = params.url.SchemeIsHTTPOrHTTPS();
-
-  if (!ShouldRecordEngagement())
+  if (!params.url.SchemeIsHTTPOrHTTPS())
     return;
 
   Profile* profile =
       Profile::FromBrowserContext(contents->GetBrowserContext());
   SiteEngagementService* service =
       SiteEngagementServiceFactory::GetForProfile(profile);
 
   if (service)
     service->HandleNavigation(params.url, params.transition);
 
-  input_tracker_.StartTracking(contents->GetRenderViewHost());
+  // Input tracking commences after a delay.
+  navigation_timer_->Start(
+      FROM_HERE, base::TimeDelta::FromSeconds(kSecondsBetweenUserInputCheck),
+      base::Bind(&SiteEngagementHelper::OnNavigationTimerFired,
+                 base::Unretained(this)));
 }
 
 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 (ShouldRecordEngagement()) {
+  if (input_tracker_.IsTracking()) {
     if (old_host)
       input_tracker_.StopTracking(old_host);
+
     input_tracker_.StartTracking(new_host);
   }
 }
 
+void SiteEngagementHelper::SetNavigationTimerForTesting(
+    scoped_ptr<base::Timer> timer) {
+  navigation_timer_ = timer.Pass();
+}
+
+void SiteEngagementHelper::OnNavigationTimerFired() {
+  record_engagement_ = true;
+  if (visible_)
+    input_tracker_.StartTracking(web_contents()->GetRenderViewHost());
+}
+
 void SiteEngagementHelper::WasShown() {
+  visible_ = true;
   // Ensure that the input callbacks are registered when we come into view.
-  if (ShouldRecordEngagement())
+  if (record_engagement_)
     input_tracker_.StartTracking(web_contents()->GetRenderViewHost());

[dominickn, 2015/10/06 00:04:41]

Is it correct that WasShown() should immediately trigger input tracking? If you
navigate somewhere and have a pop up trigger and immediately closed, then input
tracking would start immediately. Additionally, I've managed to trigger
DidNavigate prior to WasShown in testing (often when opening the browser to the
NTP and then navigating somewhere from there).

I think WasShown() should set up the navigation timer, perhaps with a smaller
timeout. Then you could rename the navigation timer to "engagament timer" or
something like that.

[calamity, 2015/10/06 03:39:55]

Agreed.

 }
 
 void SiteEngagementHelper::WasHidden() {
+  visible_ = false;
   // Ensure that the input callbacks are not registered when hidden.
-  if (ShouldRecordEngagement()) {
+  if (input_tracker_.IsTracking()) {
     content::WebContents* contents = web_contents();
     if (contents)
       input_tracker_.StopTracking(contents->GetRenderViewHost());
   }
 }
-
-// static

[dominickn, 2015/10/06 00:04:41]

Is there a particular reason for removing this function? Arguably
https://codereview.chromium.org/1373453002 should allow this value to be changed
via field trial - I'll add that if you agree (and in that case, the variable
name should stay the same).

[calamity, 2015/10/06 03:39:55]

Yeah, ok.

-void SiteEngagementHelper::SetSecondsBetweenUserInputCheck(double seconds) {
-  g_seconds_between_user_input_check = seconds;
-}