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..ac7a1febaa4d578f8b6a48adfaa4ee86ffe7d99a 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();
   }
   return false;
@@ -59,62 +63,75 @@ 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();
   }
   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,
+    int initial_delay) {
+  DCHECK(!is_tracking_);
+  host_ = host;
+  if (!is_tracking_) {
+    StartTimer(initial_delay);
+    is_tracking_ = true;
   }
 }
 
-void SiteEngagementHelper::InputTracker::PauseTracking(
-    content::RenderViewHost* host) {
-  StopTracking(host);
+void SiteEngagementHelper::InputTracker::PauseTracking() {
+  RemoveCallbacks();
+  StartTimer(g_seconds_between_user_input_check);
+}
+
+void SiteEngagementHelper::InputTracker::StopTracking() {
+  pause_timer_->Stop();
+  is_tracking_ = false;
+  RemoveCallbacks();
+  host_ = nullptr;
+}
+
+void SiteEngagementHelper::InputTracker::SetPauseTimerForTesting(
+    scoped_ptr<base::Timer> timer) {
+  pause_timer_ = timer.Pass();
+}
+
+void SiteEngagementHelper::InputTracker::StartTimer(int delay) {
   pause_timer_->Start(
-      FROM_HERE,
-      base::TimeDelta::FromSeconds(g_seconds_between_user_input_check),
-      base::Bind(&SiteEngagementHelper::InputTracker::ResumeTracking,
+      FROM_HERE, base::TimeDelta::FromSeconds(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;

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

Is this check necessary?

[dominickn, 2015/10/06 04:11:45]

web_contents() can return null (e.g. the web contents is in the process of being
destroyed). This is to catch the race where the input timer fires and callbacks
are added just as the web contents is being destructed.

[calamity, 2015/10/07 04:23:50]

Acknowledged.

+
+  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() {
   TRACE_EVENT0("SiteEngagement", "RecordUserInput");
@@ -131,31 +148,27 @@ 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(),
+                               g_seconds_tracking_delay_after_navigation);
 }
 
 void SiteEngagementHelper::RenderViewHostChanged(
@@ -163,29 +176,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_.StopTracking();
+    input_tracker_.StartTracking(new_host, 0);
   }

[dominickn, 2015/10/06 04:11:45]

I think this means that the pause timer is reset to 0 each time the render view
host changes, including if the pause timer is already running (with time left on
the clock). I think is_tracking_ should replace callbacks_added_ (i.e. only true
if they input is truly being recorded). Then you could have:

if (pause_timer_.IsRunning()) {
  host_ = new_host;
}
else if (is_tracking_) {
  input_tracker_.StopTracking();
  input_tracker_.StartTracking(new_host, 0);
}

Then you only need one bool instead of two in the class.

[calamity, 2015/10/07 04:23:50]

I think it's useful to have a straightforward way to reason about the Start/Stop
state of the InputTracker as well as the callback state. The states of the
InputTracker are getting a bit hairy.

I guess I could make IsTracking() { return pause_timer_.IsRunning() ||
callbacks_added_; } if you want to remove the is_tracking_ bool?

I fixed the RVH switch timer issue and wrote a test.

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

I'm mainly concerned that IsTracking() and StartTracking() imply that we're
tracking input, when in fact we may or may not be (particularly the latter now
that I think about it). I acknowledge that the internal state is getting a bit
confusing. Perhaps the states should be:

IsTracking: callbacks are registered, waiting for input
IsActive: as above, but also true when the timer is going
Remove Tracking from Start/Pause/Stop.

I think that clarifies the state, and makes the naming more consistent with
what's actually going on in the tracker (it's "started", but not necessarily
"tracking").

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

Agreed. Done.

 }
 
 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(),
+                                 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;
+}