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 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
 
 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,
                  base::Unretained(this));
   mouse_event_callback_ =
       base::Bind(&SiteEngagementHelper::InputTracker::HandleMouseEvent,
                  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
 // deleted; if the contents exist, record engagement for the site. Once the
 // timer finishes running, the callbacks detecting user input will be registered
 // again.
 bool SiteEngagementHelper::InputTracker::HandleKeyPressEvent(
     const content::NativeWebKeyboardEvent& event) {
   // 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;
 }
 
 bool SiteEngagementHelper::InputTracker::HandleMouseEvent(
     const blink::WebMouseEvent& event) {
   // Only respond to mouse down with a button or mouse move events (e.g. a click
   // is a mouse down and mouse up) to avoid cases where multiple events come in
   // before we can pause tracking.
   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");
   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());
+  input_tracker_.StopTracking();
+
+  record_engagement_ = params.url.SchemeIsHTTPOrHTTPS();
 
   // Ignore all schemes except HTTP and HTTPS.
-  record_engagement_ = params.url.SchemeIsHTTPOrHTTPS();
-
-  if (!ShouldRecordEngagement())
+  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(
     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 (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;
+}