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 7f1bfe5961390c7145db1c591798936834f36878..ba529632136a45877853ea433566c32b6993fcc0 100644
--- a/chrome/browser/engagement/site_engagement_helper.cc
+++ b/chrome/browser/engagement/site_engagement_helper.cc
@@ -15,7 +15,9 @@
 
 namespace {
 
-double g_seconds_between_user_input_check = 10;
+int g_seconds_between_user_input_check = 10;
+int g_seconds_tracking_delay_after_navigation = 10;
+int g_seconds_tracking_delay_after_show = 5;
 
 }  // anonymous namespace
 
@@ -24,6 +26,8 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(SiteEngagementHelper);
 SiteEngagementHelper::InputTracker::InputTracker(SiteEngagementHelper* helper)
     : helper_(helper),
       pause_timer_(new base::Timer(true, false)),
+      host_(nullptr),
+      is_tracking_(false),
       callbacks_added_(false) {
   key_press_event_callback_ =
       base::Bind(&SiteEngagementHelper::InputTracker::HandleKeyPressEvent,
@@ -33,7 +37,7 @@ SiteEngagementHelper::InputTracker::InputTracker(SiteEngagementHelper* helper)
                  base::Unretained(this));
 }
 
-SiteEngagementHelper::InputTracker::~InputTracker() { }
+SiteEngagementHelper::InputTracker::~InputTracker() {}
 
 // 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
@@ -45,7 +49,7 @@ bool SiteEngagementHelper::InputTracker::HandleKeyPressEvent(
   // Only respond to raw key down to avoid multiple triggering on a single input
   // (e.g. keypress is a key down then key up).
   if (event.type == blink::WebInputEvent::RawKeyDown) {
-    PauseTracking(helper_->web_contents()->GetRenderViewHost());
+    PauseTracking();
     helper_->RecordUserInput(SiteEngagementMetrics::ENGAGEMENT_KEYPRESS);
   }
   return false;
@@ -59,62 +63,85 @@ bool SiteEngagementHelper::InputTracker::HandleMouseEvent(
   if ((event.button != blink::WebMouseEvent::ButtonNone &&
        event.type == blink::WebInputEvent::MouseDown) ||
       event.type == blink::WebInputEvent::MouseWheel) {
-    PauseTracking(helper_->web_contents()->GetRenderViewHost());
+    PauseTracking();
     helper_->RecordUserInput(SiteEngagementMetrics::ENGAGEMENT_MOUSE);
   }
   return false;
 }
 
 void SiteEngagementHelper::InputTracker::StartTracking(
-    content::RenderViewHost* host) {
-  if (!callbacks_added_) {
-    host->AddKeyPressEventCallback(key_press_event_callback_);
-    host->AddMouseEventCallback(mouse_event_callback_);
-    callbacks_added_ = true;
-  }
+    content::RenderViewHost* host,
+    base::TimeDelta initial_delay) {
+  DCHECK(!is_tracking_);
+  DCHECK(host);
+  host_ = host;
+  StartTimer(initial_delay);
+  is_tracking_ = true;
+}
+
+void SiteEngagementHelper::InputTracker::PauseTracking() {
+  RemoveCallbacks();
+  StartTimer(base::TimeDelta::FromSeconds(g_seconds_between_user_input_check));
+}
+
+void SiteEngagementHelper::InputTracker::SwitchRenderViewHost(
+    content::RenderViewHost* new_host) {
+  DCHECK(is_tracking_);
+  DCHECK(new_host);
+  bool callbacks_were_added = callbacks_added_;
+  RemoveCallbacks();

[dominickn, 2015/10/07 05:38:50]

I'm not sure it's safe to implement it this way: the old host may have been shut
down so the host_ pointer may be invalid at this point (see
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...)

[calamity, 2015/10/07 07:13:34]

Good catch. Fixed.

+  host_ = new_host;
+  if (callbacks_were_added)
+    AddCallbacks();
+}
+
+void SiteEngagementHelper::InputTracker::StopTracking() {
+  pause_timer_->Stop();
+  is_tracking_ = false;
+  RemoveCallbacks();
+  host_ = nullptr;
 }
 
-void SiteEngagementHelper::InputTracker::PauseTracking(
-    content::RenderViewHost* host) {
-  StopTracking(host);
+void SiteEngagementHelper::InputTracker::SetPauseTimerForTesting(
+    scoped_ptr<base::Timer> timer) {
+  pause_timer_ = timer.Pass();
+}
+
+void SiteEngagementHelper::InputTracker::StartTimer(base::TimeDelta delay) {
   pause_timer_->Start(
-      FROM_HERE,
-      base::TimeDelta::FromSeconds(g_seconds_between_user_input_check),
-      base::Bind(&SiteEngagementHelper::InputTracker::ResumeTracking,
+      FROM_HERE, delay,
+      base::Bind(&SiteEngagementHelper::InputTracker::AddCallbacks,
                  base::Unretained(this)));
 }
 
-void SiteEngagementHelper::InputTracker::ResumeTracking() {
+void SiteEngagementHelper::InputTracker::AddCallbacks() {
   content::WebContents* contents = helper_->web_contents();
-  if (contents)
-    StartTracking(contents->GetRenderViewHost());
+  if (!contents)
+    return;
+
+  host_->AddKeyPressEventCallback(key_press_event_callback_);
+  host_->AddMouseEventCallback(mouse_event_callback_);
+  callbacks_added_ = true;
 }
 
-void SiteEngagementHelper::InputTracker::StopTracking(
-    content::RenderViewHost* host) {
-  pause_timer_->Stop();
+void SiteEngagementHelper::InputTracker::RemoveCallbacks() {
   if (callbacks_added_) {
-    host->RemoveKeyPressEventCallback(key_press_event_callback_);
-    host->RemoveMouseEventCallback(mouse_event_callback_);
+    host_->RemoveKeyPressEventCallback(key_press_event_callback_);
+    host_->RemoveMouseEventCallback(mouse_event_callback_);
     callbacks_added_ = false;
   }
 }
 
-void SiteEngagementHelper::InputTracker::SetTimerForTesting(
-    scoped_ptr<base::Timer> timer) {
-  pause_timer_ = timer.Pass();
-}
-
 SiteEngagementHelper::~SiteEngagementHelper() {
   content::WebContents* contents = web_contents();
   if (contents)
-    input_tracker_.StopTracking(contents->GetRenderViewHost());
+    input_tracker_.StopTracking();
 }
 
 SiteEngagementHelper::SiteEngagementHelper(content::WebContents* contents)
     : content::WebContentsObserver(contents),
       input_tracker_(this),
-      record_engagement_(false) { }
+      record_engagement_(false) {}
 
 void SiteEngagementHelper::RecordUserInput(
     SiteEngagementMetrics::EngagementType type) {
@@ -132,31 +159,28 @@ 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());
+  input_tracker_.StopTracking();
 
-  // Ignore all schemes except HTTP and HTTPS.
   record_engagement_ = params.url.SchemeIsHTTPOrHTTPS();
 
-  if (!ShouldRecordEngagement())
+  // Ignore all schemes except HTTP and HTTPS.
+  if (!record_engagement_)
     return;
 
   Profile* profile =
-      Profile::FromBrowserContext(contents->GetBrowserContext());
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
   SiteEngagementService* service =
       SiteEngagementServiceFactory::GetForProfile(profile);
 
   if (service)
     service->HandleNavigation(params.url, params.transition);
 
-  input_tracker_.StartTracking(contents->GetRenderViewHost());
+  input_tracker_.StartTracking(
+      web_contents()->GetRenderViewHost(),
+      base::TimeDelta::FromSeconds(g_seconds_tracking_delay_after_navigation));
 }
 
 void SiteEngagementHelper::RenderViewHostChanged(
@@ -164,29 +188,36 @@ 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 (old_host)
-      input_tracker_.StopTracking(old_host);
-    input_tracker_.StartTracking(new_host);
+  if (input_tracker_.is_tracking()) {
+    input_tracker_.SwitchRenderViewHost(new_host);
   }
 }
 
 void SiteEngagementHelper::WasShown() {
   // Ensure that the input callbacks are registered when we come into view.
-  if (ShouldRecordEngagement())
-    input_tracker_.StartTracking(web_contents()->GetRenderViewHost());
+  if (record_engagement_) {
+    input_tracker_.StartTracking(
+        web_contents()->GetRenderViewHost(),
+        base::TimeDelta::FromSeconds(g_seconds_tracking_delay_after_show));
+  }
 }
 
 void SiteEngagementHelper::WasHidden() {
   // Ensure that the input callbacks are not registered when hidden.
-  if (ShouldRecordEngagement()) {
-    content::WebContents* contents = web_contents();
-    if (contents)
-      input_tracker_.StopTracking(contents->GetRenderViewHost());
-  }
+  input_tracker_.StopTracking();
 }
 
 // static
-void SiteEngagementHelper::SetSecondsBetweenUserInputCheck(double seconds) {
+void SiteEngagementHelper::SetSecondsBetweenUserInputCheck(int seconds) {
   g_seconds_between_user_input_check = seconds;
 }
+
+// static
+void SiteEngagementHelper::SetSecondsTrackingDelayAfterNavigation(int seconds) {
+  g_seconds_tracking_delay_after_navigation = seconds;
+}
+
+// static
+void SiteEngagementHelper::SetSecondsTrackingDelayAfterShow(int seconds) {
+  g_seconds_tracking_delay_after_show = seconds;
+}