PageLoadMetrics renderer and browser implementation.

components/page_load_metrics/browser/metrics_web_contents_observer.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 "components/page_load_metrics/browser/metrics_web_contents_observer.h"
+
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "components/page_load_metrics/common/page_load_metrics_messages.h"
+#include "components/page_load_metrics/common/page_load_timing.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_details.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+#include "ipc/ipc_message.h"
+#include "ipc/ipc_message_macros.h"
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(
+    page_load_metrics::MetricsWebContentsObserver);
+
+namespace page_load_metrics {
+
+namespace {
+
+bool IsValidPageLoadTiming(const PageLoadTiming& timing) {
+  if (timing.IsEmpty())
+    return false;
+
+  // If we have a non-empty timing, it should always have a navigation start.

[nasko, 2015/09/14 23:32:24]

Maybe DCHECK these conditions if they should never happen and save the
if/return.

[Charlie Harrison, 2015/09/16 22:37:38]

Sure. I'm keeping the outer ifs for clarity though.

+  if (timing.navigation_start.is_null())
+    return false;
+
+  // If we have a dom content loaded event, we should have a response start.

[nasko, 2015/09/14 23:32:24]

nit: DOM

+  if (!timing.dom_content_loaded_event_start.is_zero()) {
+    if (timing.response_start.is_zero() ||
+        timing.dom_content_loaded_event_start < timing.response_start) {
+      return false;
+    }
+  }
+
+  // If we have a load event, we should have both a response start and a DCL.
+  if (!timing.load_event_start.is_zero()) {
+    if (timing.response_start.is_zero() ||
+        timing.dom_content_loaded_event_start.is_zero() ||
+        timing.load_event_start < timing.dom_content_loaded_event_start ||
+        timing.load_event_start < timing.response_start) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+}  // namespace
+
+MetricsWebContentsObserver::MetricsWebContentsObserver(
+    content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents), current_host_(nullptr) {}
+
+// As a tab helper, this object is tied to a single WebContent

[nasko, 2015/09/14 23:32:24]

How is this a tab helper? Isn't it WebContentsObserver?
nit: WebContents

[Charlie Harrison, 2015/09/16 22:37:38]

A tab helper is a WebContentsObserver. See
https://chromium.googlesource.com/chromium/src/+/master/docs/tab_helpers.md

[nasko, 2015/09/17 20:58:43]

Yes, tab helper is WebContentsObserver. WebContentsObserver needn't be a tab
helper though. I don't see how this object is a tab helper.

[Charlie Harrison, 2015/09/17 21:57:27]

Maybe my understanding of tab helper is wrong, but I thought any
WebContentsObservers attached to a tab in TabHelpers::AttachTabHelpers was
necessarily a tab helper.

[nasko, 2015/09/17 23:23:29]

I think this is my bad. I missed the part where this object is indeed added as a
tab helper. Apologies. 

+// for its entire lifetime.
+MetricsWebContentsObserver::~MetricsWebContentsObserver() {
+  RecordTimingHistograms();
+}
+
+void MetricsWebContentsObserver::FrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  if (render_frame_host == current_host_)
+    current_host_ = nullptr;
+}
+
+bool MetricsWebContentsObserver::OnMessageReceived(
+    const IPC::Message& message,
+    content::RenderFrameHost* render_frame_host) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(MetricsWebContentsObserver, message,
+                                   render_frame_host)
+    IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_TimingUpdated, OnTimingUpdated)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void MetricsWebContentsObserver::DidCommitNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->IsInMainFrame() && !navigation_handle->IsSamePage())
+    RecordTimingHistograms();
+  if (IsRelevantNavigation(navigation_handle)) {

[nasko, 2015/09/14 23:32:24]

nit: No need for {} for single line if statements.

+    current_timing_.reset(new PageLoadTiming());
+  }
+}
+
+// This will occur when the process for the main RenderFrameHost exits.
+// This will happen with a normal exit or a crash.
+void MetricsWebContentsObserver::RenderProcessGone(
+    base::TerminationStatus status) {
+  RecordTimingHistograms();
+}
+
+#define PAGE_LOAD_HISTOGRAM(name, sample)                           \
+  UMA_HISTOGRAM_CUSTOM_TIMES(name, sample,                          \
+                             base::TimeDelta::FromMilliseconds(10), \
+                             base::TimeDelta::FromMinutes(10), 100);
+
+void MetricsWebContentsObserver::OnTimingUpdated(
+    content::RenderFrameHost* render_frame_host,
+    const PageLoadTiming& timing) {
+  if (!current_timing_)
+    return;
+
+  // We may receive notifications from frames that have been navigated away
+  // from. We simply ignore them.
+  if (render_frame_host != web_contents()->GetMainFrame())
+    return;
+
+  // See comment in IsRelevantNavigation about
+  // browser/renderer url disagreement (newtab).

[nasko, 2015/09/14 23:32:24]

This comment doesn't seem related to the following call at all. Is this
intentional?

[Charlie Harrison, 2015/09/16 22:37:38]

It's related, I updated it for clarity.

+  if (!web_contents()->GetLastCommittedURL().SchemeIsHTTPOrHTTPS())
+    return;
+
+  // Throw away IPCs that are not relevant to the current navigation.
+  if (!current_timing_->navigation_start.is_null() &&
+      timing.navigation_start != current_timing_->navigation_start) {
+    // TODO(csharrison) uma log a counter here
+    return;
+  }
+
+  if (current_timing_->IsEmpty())
+    current_host_ = render_frame_host;

[nasko, 2015/09/14 23:32:24]

Please avoid storing this value. I don't see what value it brings to this code.

[Charlie Harrison, 2015/09/16 22:37:38]

Fair enough. It was just to sanity check that a single navigation will have a
consistent render_frame_host.

+  else
+    DCHECK_EQ(render_frame_host, current_host_);
+  *current_timing_ = timing;
+}
+
+void MetricsWebContentsObserver::RecordTimingHistograms() {
+  if (!current_timing_ || !IsValidPageLoadTiming(*current_timing_))
+    return;
+
+  if (!current_timing_->dom_content_loaded_event_start.is_zero()) {
+    PAGE_LOAD_HISTOGRAM(
+        "PageLoad.Timing.Navigation.To.DOMContentLoadedEventFired",
+        current_timing_->dom_content_loaded_event_start);
+  }
+
+  if (!current_timing_->load_event_start.is_zero()) {
+    PAGE_LOAD_HISTOGRAM("PageLoad.Timing.Navigation.To.LoadEventFired",
+                        current_timing_->load_event_start);
+  }
+
+  if (!current_timing_->first_layout.is_zero()) {
+    PAGE_LOAD_HISTOGRAM("PageLoad.Timing.Navigation.To.FirstLayout",
+                        current_timing_->first_layout);
+  }
+  current_timing_.reset();
+}
+
+bool MetricsWebContentsObserver::IsRelevantNavigation(
+    content::NavigationHandle* navigation_handle) {
+  // The url we see from the renderer side is not always the same as what
+  // we see from the browser side (e.g. chrome://newtab). We want to be
+  // sure here that we aren't logging UMA for internal pages.
+  const GURL& browser_url = web_contents()->GetLastCommittedURL();
+  return navigation_handle->IsInMainFrame() &&
+         !navigation_handle->IsSamePage() &&
+         navigation_handle->HasCommittedDocument() &&
+         navigation_handle->GetURL().SchemeIsHTTPOrHTTPS() &&
+         browser_url.SchemeIsHTTPOrHTTPS();

[nasko, 2015/09/14 23:32:24]

Why do you need to check both the browser url and the handle url? Aren't they
supposed to be the same?

[Charlie Harrison, 2015/09/16 22:37:38]

Yes except for special cases like https://www.google.com/_/chrome/newtab. I
updated the other comment to reflect this.

+}
+
+}  // namespace page_load_metrics