Add a delay to Site Engagement tracking after navigation.

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 9f78d8d2a0b0ea43c84168e66ca34da56c97d157..5e5b65eb20f2a512bc6efcf99020991f9a1c8a3f 100644
--- a/chrome/browser/engagement/site_engagement_helper.cc
+++ b/chrome/browser/engagement/site_engagement_helper.cc
@@ -15,7 +15,7 @@
 
 namespace {
 
-double g_seconds_between_user_input_check = 10;
+int kSecondsBetweenUserInputCheck = 10;
 
 }  // anonymous namespace
 
@@ -79,7 +79,7 @@ void SiteEngagementHelper::InputTracker::PauseTracking(
   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)));
 }
@@ -100,7 +100,11 @@ void SiteEngagementHelper::InputTracker::StopTracking(
   }
 }
 
-void SiteEngagementHelper::InputTracker::SetTimerForTesting(
+bool SiteEngagementHelper::InputTracker::IsTracking() {
+  return callbacks_added_;
+}
+
+void SiteEngagementHelper::InputTracker::SetPauseTimerForTesting(
     scoped_ptr<base::Timer> timer) {
   pause_timer_ = timer.Pass();
 }
@@ -113,8 +117,9 @@ SiteEngagementHelper::~SiteEngagementHelper() {
 
 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");
@@ -131,20 +136,17 @@ void SiteEngagementHelper::RecordUserInput() {
   }
 }
 
-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();
 
-  // Ignore all schemes except HTTP and HTTPS.
-  record_engagement_ = params.url.SchemeIsHTTPOrHTTPS();
+  record_engagement_ = false;
 
-  if (!ShouldRecordEngagement())
+  // Ignore all schemes except HTTP and HTTPS.
+  if (!params.url.SchemeIsHTTPOrHTTPS())
     return;
 
   Profile* profile =
@@ -155,7 +157,11 @@ void SiteEngagementHelper::DidNavigateMainFrame(
   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(
@@ -163,29 +169,38 @@ void SiteEngagementHelper::RenderViewHostChanged(
     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;
-}