Log First User Interaction in Page Load Metrics

components/page_load_metrics/browser/metrics_web_contents_observer.cc

Index: components/page_load_metrics/browser/metrics_web_contents_observer.cc
diff --git a/components/page_load_metrics/browser/metrics_web_contents_observer.cc b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
index 0f40c4ba17a3c29ea60e852e49da36ff7a357de7..ad04b426914503705ea23e6656762d948583c32a 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
@@ -16,6 +16,7 @@
 #include "components/page_load_metrics/browser/page_load_metrics_util.h"
 #include "components/page_load_metrics/common/page_load_metrics_messages.h"
 #include "components/page_load_metrics/common/page_load_timing.h"
+#include "content/browser/renderer_host/render_view_host_impl.h"

[Bryan McQuade, 2016/05/25 21:32:29]

My understanding is that you can't depend on content/browser from anywhere
outside of content. See this code search:
https://code.google.com/p/chromium/codesearch#search/&q=include%5C%20%5C%22co...

There are a few exceptions but for the most part only other content code is
allowed to depend on content/browser.

In your case, I believe this means you'll need to add virtual
AddInputEventCallback/RemoveInputEventCallback methods to RenderWidgetHost,
which are then implemented in RenderWidgetHostImpl.

 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_handle.h"
@@ -30,6 +31,8 @@
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(
     page_load_metrics::MetricsWebContentsObserver);
 
+class RenderViewHost;
+
 namespace page_load_metrics {
 
 namespace internal {
@@ -365,6 +368,16 @@ void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) {
   }
 }
 
+void PageLoadTracker::OnInputEvent(const blink::WebInputEvent& event) {
+  // Only log the first user interaction in a given page load.
+  if (user_interaction_time_.is_null())
+    user_interaction_time_ = base::TimeTicks::Now();
+
+  for (const auto& observer : observers_) {
+    observer->OnUserInput(event);
+  }
+}
+
 bool PageLoadTracker::UpdateTiming(const PageLoadTiming& new_timing,
                                    const PageLoadMetadata& new_metadata) {
   // Throw away IPCs that are not relevant to the current navigation.
@@ -411,12 +424,15 @@ void PageLoadTracker::AddObserver(
 PageLoadExtraInfo PageLoadTracker::GetPageLoadMetricsInfo() {
   base::TimeDelta first_background_time;
   base::TimeDelta first_foreground_time;
+  base::TimeDelta first_user_interaction_time;
   base::TimeDelta time_to_abort;
   base::TimeDelta time_to_commit;
   if (!background_time_.is_null())
     first_background_time = background_time_ - navigation_start_;
   if (!foreground_time_.is_null())
     first_foreground_time = foreground_time_ - navigation_start_;
+  if (!user_interaction_time_.is_null())
+    first_user_interaction_time = user_interaction_time_ - navigation_start_;
   if (abort_type_ != ABORT_NONE) {
     DCHECK_GT(abort_time_, navigation_start_);
     time_to_abort = abort_time_ - navigation_start_;
@@ -431,9 +447,9 @@ PageLoadExtraInfo PageLoadTracker::GetPageLoadMetricsInfo() {
     DCHECK(commit_time_.is_null());
   }
   return PageLoadExtraInfo(
-      first_background_time, first_foreground_time, started_in_foreground_,
-      commit_time_.is_null() ? GURL() : url_, time_to_commit, abort_type_,
-      time_to_abort, metadata_);
+      first_background_time, first_foreground_time, first_user_interaction_time,
+      started_in_foreground_, commit_time_.is_null() ? GURL() : url_,
+      time_to_commit, abort_type_, time_to_abort, metadata_);
 }
 
 void PageLoadTracker::NotifyAbort(UserAbortType abort_type,
@@ -514,11 +530,20 @@ MetricsWebContentsObserver* MetricsWebContentsObserver::CreateForWebContents(
                                              std::move(embedder_interface));
     web_contents->SetUserData(UserDataKey(), metrics);
   }
+
+  content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From(

[Bryan McQuade, 2016/05/25 20:47:04]

since we're unlistening in the destructor, let's listen in the construct rather
than in the CreateForWebContents method. We call the constructo in
CreateForWebContents and in the unit test, so in the latter case, if we keep
this code here, we'll attempt to unlisten in the destructor without having ever
attached ourselves as a listener.

+      metrics->web_contents()->GetRenderViewHost()->GetWidget());
+  rwhi->latency_tracker()->AddInputEventCallback(base::Bind(
+      &MetricsWebContentsObserver::OnInputEvent, base::Unretained(metrics)));
+
   return metrics;
 }
 
 MetricsWebContentsObserver::~MetricsWebContentsObserver() {
-  NotifyAbortAllLoads(ABORT_CLOSE);
+  content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From(
+      web_contents()->GetRenderViewHost()->GetWidget());

[Bryan McQuade, 2016/05/25 19:02:47]

My understanding is that a RenderViewHost's lifetime may be shorter than a
WebContents, so if the RVH associated with the web contents changes, and the
RenderWidgetHost is bound to the RVH and not the WebContents, we'll stop getting
events once the RVH changes.

That said, my understanding of these relationships is not great, so I could very
well be mistaken. My understanding is that an RVH is bound to a render process,
which can change for a given WebContents if e.g. the user navigates between
origins in the same tab (since we use different render processes for different
origins).

Anyway, it is worth digging into this to make sure this either is or is not an
issue for us. Can you dig in further and see what you find? Or if another
reviewer knows for sure, please reply & let us know.

[Bryan McQuade, 2016/05/25 20:47:04]

I think you can address this issue by implementing RenderViewHostChanged:

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

+  rwhi->latency_tracker()->RemoveInputEventCallback(base::Bind(
+      &MetricsWebContentsObserver::OnInputEvent, base::Unretained(this)));
 }
 
 bool MetricsWebContentsObserver::OnMessageReceived(
@@ -656,6 +681,21 @@ void MetricsWebContentsObserver::NavigationStopped() {
   NotifyAbortAllLoads(ABORT_STOP);
 }
 
+bool MetricsWebContentsObserver::OnInputEvent(
+    const blink::WebInputEvent& event) {
+  // ignore browser navigation or reload which comes with type Undefined

[tdresser, 2016/05/24 14:28:38]

Capitalization and end with .

+  if (event.type != blink::WebInputEvent::Type::Undefined) {
+    if (!committed_load_) {
+      RecordInternalError(ERR_USER_INTERACTION_WITH_NO_RELEVANT_LOAD);
+      return false;
+    }
+    committed_load_->OnInputEvent(event);
+    return true;
+  }
+
+  return false;
+}
+
 void MetricsWebContentsObserver::DidRedirectNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame())