[Prerender] first contentful paint histograms.

chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f7319e0d8db76dca7658082b51d35e8e8f411605
--- /dev/null
+++ b/chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc
@@ -0,0 +1,74 @@
+// Copyright 2016 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/page_load_metrics/observers/prerender_page_load_metrics_observer.h"
+
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/prerender/prerender_manager.h"
+#include "chrome/browser/prerender/prerender_manager_factory.h"
+#include "content/public/browser/web_contents.h"
+
+// static
+std::unique_ptr<page_load_metrics::PageLoadMetricsObserver>
+PrerenderPageLoadMetricsObserver::CreateIfNeeded(
+    content::WebContents* web_contents) {
+  prerender::PrerenderManager* manager =
+      prerender::PrerenderManagerFactory::GetForBrowserContext(
+          web_contents->GetBrowserContext());
+  if (!manager)
+    return nullptr;
+
+  if (manager->PageLoadMetricsObserverDisabledForTesting())
+    return nullptr;
+
+  return base::WrapUnique<page_load_metrics::PageLoadMetricsObserver>(
+      new PrerenderPageLoadMetricsObserver(manager, web_contents));
+}
+
+PrerenderPageLoadMetricsObserver::PrerenderPageLoadMetricsObserver(
+    prerender::PrerenderManager* manager,
+    content::WebContents* web_contents)
+    : prerender_manager_(manager),
+      web_contents_(web_contents),
+      was_hidden_(false) {
+  DCHECK(prerender_manager_);
+}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+PrerenderPageLoadMetricsObserver::OnStart(

[pasko, 2016/11/21 14:44:36]

I am reading the comment at PageLoadMetricsObserver::OnStart that says:

  // Implementers of OnStart that only want
  // to process non-same-page navigations should also check to see that the page
  // load committed via OnCommit or committed_url in
  // PageLoadExtraInfo.

I think same-page navigations can happen in prerendered webcontents (after
commit - via client redirects, but before first paint), so we might have
multiple calls to OnStart() here, and would get start_ticks_ later than we
wanted, right?

[Bryan McQuade, 2016/11/21 14:56:54]

Good point, yes, same page navs are something to be aware of when implementing
OnStart. Fortunately, OnStart will only be called once per instance - a same
page nav will have its own new instance of a PrerenderPageLoadMetricsObserver.
Given that OnStart doesn't do any logging and only records state that would be
used later, and given that no additional callbacks will be invoked for same-page
navs in the current impl, we're ok here.

That said, I'll do 2 things:
* I'll update the OnStart comment to be clearer about this
* I'll reach out to the navigation team to see if we can get NavigationHandle to
report that it's a same page nav at construction time, so we can avoid this
complexity.

I don't think any changes are needed to this implementation currently, though.

[Charlie Harrison, 2016/11/21 15:06:56]

NavigationHandle has IsSamePage() which is constant throughout its lifetime.
That should be good enough for the MWCO.

Additionally, I'm pretty sure same page navs don't go through the
NavigationThrottle stack, so we will never call OnStart for those.

[mattcary, 2016/11/23 13:15:17]

Acknowledged.

+    content::NavigationHandle* navigation_handle,
+    const GURL& currently_committed_url,
+    bool started_in_foreground) {
+  if (!started_in_foreground)

[pasko, 2016/11/21 14:44:36]

From my reading .. |started_in_foreground| is true iff the WebContents was in
foreground when navigation started. This seems to be about the prerendered
webcontents, which makes me wonder .. is it ever (started_in_foreground ==
true)?

Seems that the test that checks PerceivedTTFCP.Visible does not execute this
codepath, is it true?

[Bryan McQuade, 2016/11/21 14:56:54]

A prerender should always have started_in_foreground = false, so I think we
could just DCHECK(!started_in_foreground); here - that's what the code in
PageLoadTracker has been doing:
if (embedder_interface_->IsPrerendering(
          navigation_handle->GetWebContents())) {
    DCHECK(!started_in_foreground_);
    ...
}

[mattcary, 2016/11/21 16:13:37]

Done.

+    was_hidden_ = true;
+
+  start_ticks_ = navigation_handle->NavigationStart();
+  return CONTINUE_OBSERVING;
+}
+
+void PrerenderPageLoadMetricsObserver::OnFirstContentfulPaint(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  if (start_ticks_.is_null()) {

[pasko, 2016/11/21 14:44:36]

is it null only in tests?

[Bryan McQuade, 2016/11/21 14:56:54]

Yeah - I'd like to avoid this special casing if at all possible - can we force
an invocation to OnStart in all tests? Having test-only code in the production
codepath seems like something we should strive to avoid if possible.

[mattcary, 2016/11/21 16:13:37]

It wasn't clear to me that OnStart is always going to be invoked. We have to do
something if it's not, for whatever reason, now or in the future. Check-failing
seems a little extreme as this is just histogram code.

I could also not record anything in this case; the point here, though, is that
PrerenderManager is able to do something sensible with invalid timings
(recording a failed status).

[Bryan McQuade, 2016/11/21 16:26:47]

OnStart will always be invoked for every instantiated observer - it's a bug if
that ever doesn't happen. You can see that we instantiate them then immediately
call OnStart on them here:
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo...

So I think you can DCHECK or even CHECK that start_ticks_ is non-null here.

[mattcary, 2016/11/23 13:15:17]

Done.

+    prerender_manager_->RecordPerceivedFirstContentfulPaint(
+        web_contents_, base::TimeTicks(), was_hidden_);
+  } else {
+    prerender_manager_->RecordPerceivedFirstContentfulPaint(
+        web_contents_, start_ticks_ + *timing.first_contentful_paint,
+        was_hidden_);
+  }
+}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+PrerenderPageLoadMetricsObserver::OnHidden(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  was_hidden_ = true;
+  return CONTINUE_OBSERVING;

[pasko, 2016/11/21 14:44:36]

I see other observers early give up on seeing something going to background,
would it simplify to do similarly here?

[mattcary, 2016/11/21 16:13:37]

I thought it would be a good idea to know how often we get hidden; if it's a
lot, but we just drop observations, we're going to have skewed results.

+}
+
+void PrerenderPageLoadMetricsObserver::SetNavigationStartTicksForTesting(
+    base::TimeTicks ticks) {
+  start_ticks_ = ticks;
+}