PageLoadMetrics renderer and browser implementation.

components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc

Index: components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
diff --git a/components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc b/components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..924a9429e3439eee0d93f78d02f00780d349d160
--- /dev/null
+++ b/components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
@@ -0,0 +1,107 @@
+// 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/page_load_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/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::WebContentsObserver);
+
+namespace page_load_metrics {
+
+WebContentsObserver::WebContentsObserver(content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents) {}
+
+WebContentsObserver::~WebContentsObserver() {
+  RecordTimingHistograms();

[Bryan McQuade, 2015/09/02 18:26:45]

can we add a short comment here noting why it's correct to record timing
histograms as part of being destroyed? is this the codepath that gets invoked
when a tab is closed?

[Charlie Harrison, 2015/09/03 14:00:52]

This gets invoked when a tab is closed, and when we close Chrome completely. In
the former case RenderProcessGone works fine. This is a last resort.

+}
+
+bool WebContentsObserver::OnMessageReceived(
+    const IPC::Message& message,
+    content::RenderFrameHost* render_frame_host) {
+  bool handled = true;

[Bryan McQuade, 2015/09/02 18:26:45]

just to sanity check, can we add a DCHECK that this method is invoked on the
main browser thread? There's a part of me that worries this may get dispatched
on the IO thread, and if that's the case, we need to add some code to re-route
the dispatch of these methods to the main browser thread (since the other WCO
callbacks are invoked there).

Note that, to ease testing slightly, I think it's very reasonable to do
something like:

bool WebContentsObserver::OnMessageReceived(
    const IPC::Message& message,
    content::RenderFrameHost* render_frame_host) {
  // I'm sure there's a method for this already that you can use
  DCHECK(SomeMethodThatVerifiesWeAreOnTheRightThread());
  return DoDispatchMessage(message);
}

bool WebContentsObserver::DoDispatchMessage(const IPC::Message& message) {
  // the current implementation of OnMessageReceived goes here
}

Then in your tests you can dispatch messages directly to DoDispatchMessage. This
allows you to avoid having to fake out that this code is running on the main
thread in tests, which isn't going to validate anything (since it's stubbed out
using e.g. TestBrowserThreadBundle) and makes writing tests harder.

That said, I'm happy to write tests that verify use of right threads if the
tests we right are dispatching IPCs using the real IPC mechanism. I personally
don't think we'd need to do that though.

[Bryan McQuade, 2015/09/02 18:38:31]

actually, if you confirm that this does get invoked on the right thread, then
I'm ok just removing the dcheck once we know that, and not adding the extra
method. just want some verification that this doesn't get called on the IO
thread, and if it does, that we add code to re-route dispatch of these messages
to the browser main thread.

[Charlie Harrison, 2015/09/03 14:00:52]

Let's add a DCHECK. Do you know an easy way to see what thread we're in?

[Bryan McQuade, 2015/09/03 20:23:37]

I think you can do:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

+  IPC_BEGIN_MESSAGE_MAP(WebContentsObserver, message)
+    IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_TimingUpdated, OnTimingUpdated)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void WebContentsObserver::DidCommitNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame())
+    return;
+
+  if (!navigation_handle->HasCommittedDocument())
+    return;
+
+  const GURL& url = navigation_handle->GetURL();
+  const GURL& browser_url = GetLastCommittedURL();
+  if (!url.SchemeIsHTTPOrHTTPS() && !browser_url.SchemeIsHTTPOrHTTPS())

[Bryan McQuade, 2015/09/02 18:26:45]

does this want to be || rather than &&?

in particular, for the chrome://newtab case, where navigation handle returns an
https URL, but GetLastCommittedUrl returns chrome://newtab, seems we should bail
early if either one is not http/https.

let's also add a comment to explain what's going on here, and probably file a
bug noting the discrepancy in what the methods return, and that our code
currently depends on it, but longer term they should probably return the same
thing (and we should make sure that any changes in behavior do not break our
code).

[Charlie Harrison, 2015/09/03 14:00:52]

Done. Will draft a bug soon.

+    return;
+  RecordTimingHistograms();
+  current_timing_ = pending_timing_;
+  pending_timing_ = PageLoadTiming();
+}
+
+void WebContentsObserver::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 WebContentsObserver::OnTimingUpdated(const PageLoadTiming& timing) {
+  if (!GetLastCommittedURL().SchemeIsHTTPOrHTTPS())

[Bryan McQuade, 2015/09/02 18:26:45]

let's add a short comment here to note the specific reason why this is needed
(ntp).

[Charlie Harrison, 2015/09/03 14:00:52]

Done.

+    return;
+
+  if (timing.IsChild(current_timing_)) {

[Bryan McQuade, 2015/09/02 18:26:45]

in general (throughout this change), you can remove braces for single line if
statements, per the style guide.

+    current_timing_ = timing;
+  } else if (timing.IsChild(pending_timing_)) {

[Bryan McQuade, 2015/09/02 18:26:45]

I'm not following what's going on with these if/elseif/else's - maybe add some
comments to explain why we want to know if timing is a child of current_timing_,
and if it's a child of pending_timing_, etc.

[Charlie Harrison, 2015/09/03 14:00:52]

I think I'm going to remove the pending_timing following our discussion

+    pending_timing_ = timing;
+  } else {
+    NOTREACHED();
+  }
+}
+
+void WebContentsObserver::RecordTimingHistograms() {
+  PageLoadTiming timing = current_timing_;

[Bryan McQuade, 2015/09/02 18:26:45]

I don't think you need to take a function-scoped copy of current_timing_ here -
all the code in this class should run on the same thread, and we don't change
current_timing_ anywhere in the scope of the function.

Alternatively, if you want to pass a timing instance as a parameter to this
function, I think that's fine, but I'd pass it as a 'const PageLoadTiming&'
since there's no need to copy it.

[Charlie Harrison, 2015/09/03 14:00:52]

Done.

+  if (timing.IsComplete())
+    DCHECK(timing.IsOrdered());
+  else if (timing.IsEmpty())

[Bryan McQuade, 2015/09/02 18:26:45]

nit: I'd do the test for timing.IsEmpty() before the test for
timing.IsComplete() - I generally like to see the bail-out conditions for
functions as early as possible.

[Charlie Harrison, 2015/09/03 14:00:52]

Done.

+    return;
+
+  if (!timing.dom_content_loaded_event_start.is_zero()) {
+    PAGE_LOAD_HISTOGRAM("PageLoad.Timing.DOMContentLoadedEventFired",
+                        timing.dom_content_loaded_event_start);
+  }
+
+  if (!timing.load_event_start.is_zero()) {
+    PAGE_LOAD_HISTOGRAM("PageLoad.Timing.LoadEventFired",
+                        timing.load_event_start);
+  }
+
+  if (!timing.first_layout.is_zero()) {
+    PAGE_LOAD_HISTOGRAM("PageLoad.Timing.FirstLayout", timing.first_layout);
+  }
+}
+
+const GURL& WebContentsObserver::GetLastCommittedURL() {

[Bryan McQuade, 2015/09/02 18:26:45]

unless you need to override the implementation of this function in a test
(which, despite my having done it, i agree with randy is generally undesirable)
i think it's probably reasonable to just inline the implementation directly and
remove the function.

[Charlie Harrison, 2015/09/02 18:38:24]

The reason is so we don't have to mock an entire WebContents to unittest this
class. We can just subclass it and alter this method. If there's a better way
than completely mocking WebContents, which would be a pretty big undertaking,
I'd definitely do it.

[Bryan McQuade, 2015/09/02 18:40:27]

Ok, that seems like a good reason - in that case you'll just want to change the
method to protected in the header file, to indicate that it is intended to be
overridden by derived implementations. You might add a '// Overridden in
unittests' comment to go along with it.

[Bryan McQuade, 2015/09/03 12:04:29]

Actually, it's been a while since I used private virtual, but on reviewing that,
I think what you've done here is correct, and it can remain private virtual
(rather than protected). As you probably know, this allows derived classes to
override the behavior of the base class implementation, without being able to
call the base class implementation, which is I think what you want in this case.

+  return web_contents()->GetLastCommittedURL();
+}
+
+}  // namespace page_load_metrics