[NoStatePrefetch] Add performance histograms.

chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h

Index: chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
diff --git a/chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h b/chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
new file mode 100644
index 0000000000000000000000000000000000000000..ee133fe69e9a53baec9c61a563de9a6514edf1ba
--- /dev/null
+++ b/chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
@@ -0,0 +1,39 @@
+// 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.
+
+#ifndef CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_NO_STATE_PREFETCH_PAGE_LOAD_METRICS_OBSERVER_H_
+#define CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_NO_STATE_PREFETCH_PAGE_LOAD_METRICS_OBSERVER_H_
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/page_load_metrics/page_load_metrics_observer.h"
+#include "chrome/browser/prerender/prerender_manager.h"
+#include "url/gurl.h"
+
+// Observer recording metrics related to NoStatePrefetch.
+class NoStatePrefetchPageLoadMetricsObserver
+    : public page_load_metrics::PageLoadMetricsObserver {
+ public:
+  NoStatePrefetchPageLoadMetricsObserver();
+  ~NoStatePrefetchPageLoadMetricsObserver() override;
+
+ private:
+  // page_load_metrics::PageLoadMetricsObserver overrides:
+  void OnStart(content::NavigationHandle* navigation_handle,
+               const GURL& currently_committed_url,
+               bool started_in_foreground) override;
+  void OnCommit(content::NavigationHandle* navigation_handle) override;
+  void OnFirstContentfulPaint(
+      const page_load_metrics::PageLoadTiming& timing,
+      const page_load_metrics::PageLoadExtraInfo& extra_info) override;
+
+  GURL url_;          // URL of the page being loaded.

[Charlie Harrison, 2016/09/15 13:41:33]

The URL is stored in the PageLoadExtraInfo. Let's not do a copy here (URLs can
be big).

You also set this in OnStart, which is before redirects take place. The one in
the extra info is the URL at commit time.

[droger, 2016/09/15 15:06:34]

Not done, I want the URL before redirects.

The fact that extra_info has the URL after redirects is surprising to me though,
and raises one concern:
So, just checking: the timings (such as time to first contentful paint) do
include the time spent in redirects, right?

[Charlie Harrison, 2016/09/15 15:22:19]

Yeah they include redirect time. Can you explain why you want the url before
redirects? The extra info has the url after redirects because usually consumers
want to know the url that the main resource actually came from.

If you really need the start url I think it should also go in the extra info, so
future consumers who want it don't have to do a similar copy. URLs can be pretty
big in some cases.

[droger, 2016/09/16 12:21:49]

When a navigation happens, I want to know if it was prefetched or not, and track
this in a histogram.
The way it is done is:
- when a prefetch starts, record the URL (which is the starting URL, before
redirects).
- when a subsequent page load happens, compare the URL with the prefetched URL
to see if they match

We could compare committed URLs instead, but comparing the starting URL is
better since the starting URL uniquely identifies a navigation whereas the
committed URL does not.

If we wanted to go further we could record the whole redirect chains and find
common suffixes. This would allow us to find "partial" uses of prefetches. I
don't think we have plans to do this though.
Done.

+  bool is_no_store_;  // True if the main resource has a "no-store" HTTP header.
+  bool navigation_committed_;

[Charlie Harrison, 2016/09/15 13:41:33]

It looks like this bool is only for a DCHECK. Is that really necessary?

[droger, 2016/09/15 15:06:34]

Indeed this is only for a DCHECK, specifically to check that
OnFirstContentfulPaint is called after OnCommit.
If this seems obvious to you, I can remove it (I'm not familiar with these
observers).

Otherwise, we could always argue that DCHECKs are not necessary, but I don't
really see why having this member would be a problem.

[Charlie Harrison, 2016/09/15 15:22:19]

I'm happy with DCHECKs in general I just feel like adding a member that is only
used for debugging is overkill. I'd prefer if you added it to
DispatchObserverTimingCallbacks.

[droger, 2016/09/16 12:21:49]

The documentation in PageLoadMetricsObserver actually mentions that OnCommit is
called before timing updates.
I added a DCHECK in DispatchObserverTimingCallbacks as suggested.

+  base::WeakPtr<prerender::PrerenderManager> prerender_manager_;

[Charlie Harrison, 2016/09/15 13:41:33]

Why is a weak pointer necessary? The lifetime of the observer is scoped to a WCO
which is scoped to a WebContents.

[droger, 2016/09/15 15:06:34]

I'm not that familiar with PrerenderManager lifetime, but I think you're right.
It is profile keyed service, so its lifetime is greater than the WebContents'.
Not sure why it supports weak pointers.

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(NoStatePrefetchPageLoadMetricsObserver);
+};
+
+#endif  // CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_NO_STATE_PREFETCH_PAGE_LOAD_METRICS_OBSERVER_H_