[NoStatePrefetch] Add performance histograms.

[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store" headers.

BUG=642371
TBR=jochen

Committed: https://crrev.com/c154315d263486c4ecd1cc9b1f73e82efeb2468a
Cr-Commit-Position: refs/heads/master@{#419733}

[droger, 2016-09-08 15:53:56]

droger@chromium.org changed reviewers:
+ mattcary@chromium.org, pasko@chromium.org

[droger, 2016-09-08 15:53:57]

This is not completely finished, but sending early if people are interested to
discuss it.

This uses the PageLoadMetricsObserver API to collect metrics.
The first version records the "time to first contentful paint", but we could
change that to whatever timing is more relevant for us.

I'm also not sure what we want to measure in details. My first idea would be to
record the timing for pages that were prefetched recently, and in another
histogram the timing for pages that were not prefetched recently.

I'm not sure what else we need. For example do we need to account for:
- no-store vs cacheable
- if the page was visited recently for real (as a regular navigation, not
prefetch).
- if the page was prefetched, but more than 5 minutes ago (no-longer avoiding
revalidation)

I'm open to discussion/brainstorming with people that are interested.

[droger, 2016-09-12 10:04:30]

On 2016/09/08 15:53:57, droger wrote:
> I'm open to discussion/brainstorming with people that are interested.

Matt and I discussed this and here's a summary:

- need histogram to record the age of a prefetch when it is used

- 3 histograms for page load time: pages not prefetched, pages prefetched less
than 5 min ago (skipping revalidation), prefetches older than 5 minutes. 
  For each of these we need two variations to distinguish whether the main
resource was no-store or not.

- If a page is loaded multiple times after a prefetch, only count the first as
being prefetched, and the later ones as normal loads (because the LOAD_PREFETCH
flags only work once).

- To do later: would be nice to get the time to generate the prefetch, and look
for pages that start loading before the prefetch is finished.

[droger, 2016-09-12 16:00:10]

Patchset #4 (id:60001) has been deleted

[droger, 2016-09-12 16:12:09]

Patchset #1 (id:1) has been deleted

[droger, 2016-09-12 16:12:17]

Patchset #1 (id:20001) has been deleted

[droger, 2016-09-12 16:12:41]

Patchset #2 (id:80001) has been deleted

[droger, 2016-09-12 16:15:17]

I implemented the histograms as discussed.

Note that in addition to the discussed splits (per prefetch age, per "no-store"
attribute), these are also split by origin type.

[mattcary, 2016-09-14 12:33:21]

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:475: ?
"NoStatePrefetchTTFCPWithoutRevalidation"
So do we know positively that revalidation actually happens as soon as we cross
this time? If not, then a more clear histogram name might be
*WithinPrefetchReuse or *OutsidePrefetchReuse, or similar.

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager_factory.h (right):

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager_factory.h:28: static
PrerenderManager* GetForBrowserContext(
Why change this? Is it just cleanup or is there a functional reason?

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_navigator.cc:388:
prerender::PrerenderManagerFactory::GetForBrowserContext(profile);
Here and elsewhere, the profile is gotten from the BrowserContext, above, and it
seems like it would be cleaner to pass that in rather than the profile?

[droger, 2016-09-14 14:10:54]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 14:11:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2016-09-14 14:15:42]

Thanks!

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:475: ?
"NoStatePrefetchTTFCPWithoutRevalidation"
On 2016/09/14 12:33:21, mattcary wrote:
> So do we know positively that revalidation actually happens as soon as we
cross
> this time? If not, then a more clear histogram name might be
> *WithinPrefetchReuse or *OutsidePrefetchReuse, or similar.

I assume that your concern is that being outside that time window does not mean
that revalidation actually happens, because there are resources that don't need
revalidation at all (or under a different time window).

In this case, you're right that my naming is not great.

"OutsidePrefetchReuse" may be better (as it parallels the constant name) but
also not great, because the prefetch can still be reused even outside of the
window (for resources that don't need revalidation, but even for all resources
since revalidation is cheaper than a full fetch).

What about Warm/Cold (i.e. NoStatePrefetchTTFCPWarm and
NoStatePrefetchTTFCPCold) or Fresh/Old?

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager_factory.h (right):

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager_factory.h:28: static
PrerenderManager* GetForBrowserContext(
On 2016/09/14 12:33:21, mattcary wrote:
> Why change this? Is it just cleanup or is there a functional reason?

It's mainly a cleanup. I can do this a separate CL if this is better, or even
work around it.

I did this because I added a new call to this function in a place where I have a
BrowserContext and not a Profile.

I could convert the BrowserContext to Profile instead (like many other callers
do), but it turns out that the function does not actually need a Profile and a
BrowserContext is enough. So I figured the cleaner way is to fix the function
and all the callers to use BrowserContext.

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_navigator.cc:388:
prerender::PrerenderManagerFactory::GetForBrowserContext(profile);
On 2016/09/14 12:33:21, mattcary wrote:
> Here and elsewhere, the profile is gotten from the BrowserContext, above, and
it
> seems like it would be cleaner to pass that in rather than the profile?

Profile is the BrowserContext (by inheritance).
FromBrowserContext() is just a static cast, and the two objects are literally
the same.
This property is not just an implementation detail, as Chrome assumes this
everywhere.

So I think it's fine to conflate the two here in this specific file, since we
need to have the profile for other calls in any case (i.e. line 383).


For other files though, I double checked and you're right, there are a few cases
where the conversion from BrowserContext to Profile is now useless, that I
missed in my first pass. Fixed now.

[mattcary, 2016-09-14 14:19:39]

lgtm

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:475: ?
"NoStatePrefetchTTFCPWithoutRevalidation"
> I assume that your concern is that being outside that time window does not
mean
> that revalidation actually happens, because there are resources that don't
need
> revalidation at all (or under a different time window).
> 
Yes, exactly.

> 
> What about Warm/Cold (i.e. NoStatePrefetchTTFCPWarm and
> NoStatePrefetchTTFCPCold) or Fresh/Old?

SGTM

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager_factory.h (right):

https://codereview.chromium.org/2321083002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager_factory.h:28: static
PrerenderManager* GetForBrowserContext(
On 2016/09/14 14:15:42, droger wrote:
> On 2016/09/14 12:33:21, mattcary wrote:
> > Why change this? Is it just cleanup or is there a functional reason?
> 
> It's mainly a cleanup. I can do this a separate CL if this is better, or even
> work around it.
> 
> I did this because I added a new call to this function in a place where I have
a
> BrowserContext and not a Profile.
> 
> I could convert the BrowserContext to Profile instead (like many other callers
> do), but it turns out that the function does not actually need a Profile and a
> BrowserContext is enough. So I figured the cleaner way is to fix the function
> and all the callers to use BrowserContext.

Ok, thanks for the context.

No need to put that in a separate CL from my perspective.

[commit-bot: I haz the power, 2016-09-14 14:27:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 14:28:00]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[droger, 2016-09-14 14:40:44]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 14:41:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 14:59:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 14:59:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[droger, 2016-09-15 09:58:12]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 09:58:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2016-09-15 09:58:33]

Patchset #4 (id:140001) has been deleted

[droger, 2016-09-15 10:06:02]

droger@chromium.org changed reviewers:
+ asvitkine@chromium.org, mmenke@chromium.org

[droger, 2016-09-15 10:06:02]

+mmenke for chrome/browser/prerender
+asvitkine for histograms.xml

Il will TBR the OWNERS for other changes (updating callsites of the
PrerenderManagerFactory).

[droger, 2016-09-15 10:10:02]

Description was changed from

==========
[NoStatePrefetch] Add performance histograms.

BUG=642371
==========

to

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:
- NoStatePrefetchAge: age of the prefetch when it is used
- NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
  - Cold/Warm/Reference: whether the prefetch is recent enough
    to skip validation (reference means no prefetch).
  - Cacheable/NoStore: whether the main resource had "no-store"
    headers.

BUG=642371
==========

[droger, 2016-09-15 10:12:32]

Description was changed from

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:
- NoStatePrefetchAge: age of the prefetch when it is used
- NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
  - Cold/Warm/Reference: whether the prefetch is recent enough
    to skip validation (reference means no prefetch).
  - Cacheable/NoStore: whether the main resource had "no-store"
    headers.

BUG=642371
==========

to

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:
- NoStatePrefetchAge: age of the prefetch when it is used
- NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
  - Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
  - Cacheable/NoStore: whether the main resource had "no-store"
    headers.

BUG=642371
==========

[droger, 2016-09-15 10:13:08]

Description was changed from

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:
- NoStatePrefetchAge: age of the prefetch when it is used
- NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
  - Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
  - Cacheable/NoStore: whether the main resource had "no-store"
    headers.

BUG=642371
==========

to

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store"
    headers.

BUG=642371
==========

[droger, 2016-09-15 10:13:35]

Description was changed from

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store"
    headers.

BUG=642371
==========

to

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store" headers.

BUG=642371
==========

[droger, 2016-09-15 10:14:15]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 10:14:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2016-09-15 10:15:45]

droger@chromium.org changed reviewers:
+ csharrison@chromium.org

[droger, 2016-09-15 10:15:46]

And +csharrison for chrome/browser/page_load_metrics

[commit-bot: I haz the power, 2016-09-15 10:51:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 10:51:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[droger, 2016-09-15 11:21:34]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 11:21:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 12:03:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 12:03:56]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Charlie Harrison, 2016-09-15 13:41:34]

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc:28:
prerender_manager_ = manager->AsWeakPtr();
Be mindful that we turn off page load metrics for prerenderer pages (see
PageLoadMetricsEmbedder::IsPrerendering and uses). Make sure that that won't
interfere with any logging that you want to do.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc:28:
prerender_manager_ = manager->AsWeakPtr();
I wonder if we can avoid setting this observer if there is no manager in a
create() static method or something? This would avoid allocation for non
prerenderer pages (or pages without a a prerender_manager).

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:31:
GURL url_;          // URL of the page being loaded.
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.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:33:
bool navigation_committed_;
It looks like this bool is only for a DCHECK. Is that really necessary?

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:34:
base::WeakPtr<prerender::PrerenderManager> prerender_manager_;
Why is a weak pointer necessary? The lifetime of the observer is scoped to a WCO
which is scoped to a WebContents.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:464: histogram_base_name =
"NoStatePrefetchTTFCPReference";
We prefer page load metrics observers to use histograms that follow the form

PageLoad.Clients.<client name>.<page load metric>

In this case we have a TTFCP metric, so the name would look like
PageLoad.Clients.NoStatePrefetch.NavigationStartToFirstContentfulPaint

This way you can use the metrics by just adding a histogram suffix to
histograms.xml

[droger, 2016-09-15 15:06:21]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

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

Thanks for the review.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc:28:
prerender_manager_ = manager->AsWeakPtr();
On 2016/09/15 13:41:33, Charlie Harrison wrote:
> I wonder if we can avoid setting this observer if there is no manager in a
> create() static method or something? This would avoid allocation for non
> prerenderer pages (or pages without a a prerender_manager).

As far as I know, this is only happening in unit tests. I see no harm in doing
it though.
Done.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc:28:
prerender_manager_ = manager->AsWeakPtr();
On 2016/09/15 13:41:33, Charlie Harrison wrote:
> Be mindful that we turn off page load metrics for prerenderer pages (see
> PageLoadMetricsEmbedder::IsPrerendering and uses). Make sure that that won't
> interfere with any logging that you want to do.

I know, and I actually don't want the metrics for the prerendered pages.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:31:
GURL url_;          // URL of the page being loaded.
On 2016/09/15 13:41:33, Charlie Harrison wrote:
> 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.

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?

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:33:
bool navigation_committed_;
On 2016/09/15 13:41:33, Charlie Harrison wrote:
> It looks like this bool is only for a DCHECK. Is that really necessary?

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.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:34:
base::WeakPtr<prerender::PrerenderManager> prerender_manager_;
On 2016/09/15 13:41:33, Charlie Harrison wrote:
> Why is a weak pointer necessary? The lifetime of the observer is scoped to a
WCO
> which is scoped to a WebContents.

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.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:464: histogram_base_name =
"NoStatePrefetchTTFCPReference";
On 2016/09/15 13:41:33, Charlie Harrison wrote:
> We prefer page load metrics observers to use histograms that follow the form
> 
> PageLoad.Clients.<client name>.<page load metric>
> 
> In this case we have a TTFCP metric, so the name would look like
> PageLoad.Clients.NoStatePrefetch.NavigationStartToFirstContentfulPaint
> 
> This way you can use the metrics by just adding a histogram suffix to
> histograms.xml

The fact that my metric is implemented by a PageLoadMetricsObserver is an
implementation detail for me.
This is mainly a prerender histogram, which has different formatting rules and
many other suffixes.

The final name of the histogram will be something like:
Prerender.omnibox.NoStatePrefetchTTFCPReferenceNoStore
which is already long.

Under your convention it becomes:
PageLoad.Clients.Prerender.omnibox.NoStatePrefetchReferenceNoStore.NavigationStartToFirstContentfulPaint

Which is extra super long.
I'm also not sure these kind of histograms would have any value for people
looking at these PageLoad.Clients histograms.

I would argue that we should keep the name as is.

[commit-bot: I haz the power, 2016-09-15 15:06:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2016-09-15 15:17:18]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 15:17:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2016-09-15 15:17:39]

Patchset #4 (id:160001) has been deleted

[droger, 2016-09-15 15:18:31]

Patchset #4 (id:180001) has been deleted

[Charlie Harrison, 2016-09-15 15:22:20]

Thanks! Just a few more questions.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:31:
GURL url_;          // URL of the page being loaded.
On 2016/09/15 15:06:34, droger wrote:
> On 2016/09/15 13:41:33, Charlie Harrison wrote:
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:33:
bool navigation_committed_;
On 2016/09/15 15:06:34, droger wrote:
> On 2016/09/15 13:41:33, Charlie Harrison wrote:
> > It looks like this bool is only for a DCHECK. Is that really necessary?
> 
> 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.

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.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:464: histogram_base_name =
"NoStatePrefetchTTFCPReference";
On 2016/09/15 15:06:34, droger wrote:
> On 2016/09/15 13:41:33, Charlie Harrison wrote:
> > We prefer page load metrics observers to use histograms that follow the form
> > 
> > PageLoad.Clients.<client name>.<page load metric>
> > 
> > In this case we have a TTFCP metric, so the name would look like
> > PageLoad.Clients.NoStatePrefetch.NavigationStartToFirstContentfulPaint
> > 
> > This way you can use the metrics by just adding a histogram suffix to
> > histograms.xml
> 
> The fact that my metric is implemented by a PageLoadMetricsObserver is an
> implementation detail for me.
> This is mainly a prerender histogram, which has different formatting rules and
> many other suffixes.
> 
> The final name of the histogram will be something like:
> Prerender.omnibox.NoStatePrefetchTTFCPReferenceNoStore
> which is already long.
> 
> Under your convention it becomes:
>
PageLoad.Clients.Prerender.omnibox.NoStatePrefetchReferenceNoStore.NavigationStartToFirstContentfulPaint
> 
> Which is extra super long.
> I'm also not sure these kind of histograms would have any value for people
> looking at these PageLoad.Clients histograms.
> 
> I would argue that we should keep the name as is.

This is reasonable to me.

[commit-bot: I haz the power, 2016-09-15 16:12:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 16:12:34]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-09-15 16:53:29]

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:52: void
RecordHistogramTime(std::string histogram_name,
Pass by const std::string&

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:484: time, 100);
We don't recommend using 100 bucket histograms unless you have evidence that 50
bucket isn't enough. 100 bucket histograms end up using much more data than 50
as usually more unique buckets are used and there's less aggregation.

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45956: +<histogram
name="Prerender.NoStatePrefetchTTFCPWarmCacheable" units="ms">
I suggest using the histogram_suffixes construct to make the XML a bit more
concise. See the top of the file for an example.

Also suggest adding an intermediate . as a delimiter to make these a bit more
readable, e.g.:

rerender.NoStatePrefetchTTFCP.WarmCacheable

[mmenke, 2016-09-15 17:20:44]

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc:59:
DCHECK(navigation_committed_);
include base/logging.h

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:48:
prerender::PrerenderManager* prerender_manager_;
const?

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:570: base::TimeDelta time) {
Would it make more sense to pass along the WebContents instead?  Relying one
prerender of a URL seems like a bad idea.  Also worth noting that offline
prerenders may be able to violate that, so you end up with the wrong prerender.

[droger, 2016-09-16 12:21:09]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 12:21:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2016-09-16 12:21:50]

Thanks all for the reviews.

Almost everything done.
I have a few questions and comments inline.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:31:
GURL url_;          // URL of the page being loaded.
On 2016/09/15 15:22:19, Charlie Harrison wrote:
> On 2016/09/15 15:06:34, droger wrote:
> > On 2016/09/15 13:41:33, Charlie Harrison wrote:
> > > 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.
> > 
> > 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?
> 
> 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.
> 

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.

> 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.

Done.

https://codereview.chromium.org/2321083002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:33:
bool navigation_committed_;
On 2016/09/15 15:22:19, Charlie Harrison wrote:
> On 2016/09/15 15:06:34, droger wrote:
> > On 2016/09/15 13:41:33, Charlie Harrison wrote:
> > > It looks like this bool is only for a DCHECK. Is that really necessary?
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:48:
prerender::PrerenderManager* prerender_manager_;
On 2016/09/15 17:20:44, mmenke wrote:
> const?

It cannot be const, since I call a non-const function on the prerender manager
(which cannot be made const).

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:52: void
RecordHistogramTime(std::string histogram_name,
On 2016/09/15 16:53:28, Alexei Svitkine (very slow) wrote:
> Pass by const std::string&

Good catch. This mistake comes from a copy-paste, so I'm fixing the code I
copied from too.

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:484: time, 100);
On 2016/09/15 16:53:28, Alexei Svitkine (very slow) wrote:
> We don't recommend using 100 bucket histograms unless you have evidence that
50
> bucket isn't enough. 100 bucket histograms end up using much more data than 50
> as usually more unique buckets are used and there's less aggregation.

Done.

I did this because all other timing histograms in this file also use 100 buckets
(see line 238 above). Should I change them all to 50?

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:570: base::TimeDelta time) {
On 2016/09/15 17:20:44, mmenke wrote:
> Would it make more sense to pass along the WebContents instead?  Relying one
> prerender of a URL seems like a bad idea.  Also worth noting that offline
> prerenders may be able to violate that, so you end up with the wrong
prerender.

Sorry, I don't understand the concern.

Note that I don't need any WebContents here.

What this function does, is look if that URL was prefetched recently (note that
prefetch is different from prerender, in particular it does not matter if the
WebContents that was used for prefetch is still alive), and if so records how
long ago.

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45956: +<histogram
name="Prerender.NoStatePrefetchTTFCPWarmCacheable" units="ms">
On 2016/09/15 16:53:28, Alexei Svitkine (very slow) wrote:
> I suggest using the histogram_suffixes construct to make the XML a bit more
> concise. See the top of the file for an example.
> 
> Also suggest adding an intermediate . as a delimiter to make these a bit more
> readable, e.g.:
> 
> rerender.NoStatePrefetchTTFCP.WarmCacheable

I added the dot separator.

However, I don't know if the suffix thing is possible, since these histograms
already have a suffix.
Can we do nested <suffix> constructs?
Or should I do second suffix construct, and list all the combinations from the
first suffix construct in the <affected-histograms>?

[commit-bot: I haz the power, 2016-09-16 13:15:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 13:15:47]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-09-16 14:16:21]

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:484: time, 100);
On 2016/09/16 12:21:49, droger wrote:
> On 2016/09/15 16:53:28, Alexei Svitkine (very slow) wrote:
> > We don't recommend using 100 bucket histograms unless you have evidence that
> 50
> > bucket isn't enough. 100 bucket histograms end up using much more data than
50
> > as usually more unique buckets are used and there's less aggregation.
> 
> Done.
> 
> I did this because all other timing histograms in this file also use 100
buckets
> (see line 238 above). Should I change them all to 50?
> 

Changing the other histograms' buckets is a breaking change and requires
renaming them. So it's probably OK to leave them as-is but use 50 for all new
ones.

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45956: +<histogram
name="Prerender.NoStatePrefetchTTFCPWarmCacheable" units="ms">
On 2016/09/16 12:21:50, droger wrote:
> On 2016/09/15 16:53:28, Alexei Svitkine (very slow) wrote:
> > I suggest using the histogram_suffixes construct to make the XML a bit more
> > concise. See the top of the file for an example.
> > 
> > Also suggest adding an intermediate . as a delimiter to make these a bit
more
> > readable, e.g.:
> > 
> > rerender.NoStatePrefetchTTFCP.WarmCacheable
> 
> I added the dot separator.
> 
> However, I don't know if the suffix thing is possible, since these histograms
> already have a suffix.
> Can we do nested <suffix> constructs?
> Or should I do second suffix construct, and list all the combinations from the
> first suffix construct in the <affected-histograms>?

You can do multiple sets of suffixes and affected histograms can list suffixed
entries that were created by a different suffixes definition.

[droger, 2016-09-16 14:51:59]

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45956: +<histogram
name="Prerender.NoStatePrefetchTTFCPWarmCacheable" units="ms">
On 2016/09/16 14:16:21, Alexei Svitkine (very slow) wrote:
> On 2016/09/16 12:21:50, droger wrote:
> > On 2016/09/15 16:53:28, Alexei Svitkine (very slow) wrote:
> > > I suggest using the histogram_suffixes construct to make the XML a bit
more
> > > concise. See the top of the file for an example.
> > > 
> > > Also suggest adding an intermediate . as a delimiter to make these a bit
> more
> > > readable, e.g.:
> > > 
> > > rerender.NoStatePrefetchTTFCP.WarmCacheable
> > 
> > I added the dot separator.
> > 
> > However, I don't know if the suffix thing is possible, since these
histograms
> > already have a suffix.
> > Can we do nested <suffix> constructs?
> > Or should I do second suffix construct, and list all the combinations from
the
> > first suffix construct in the <affected-histograms>?
> 
> You can do multiple sets of suffixes and affected histograms can list suffixed
> entries that were created by a different suffixes definition.

Done.
Does this break if the suffix definition are re-ordered in the future?

[Alexei Svitkine (slow), 2016-09-16 14:59:37]

lgtm

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2321083002/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45956: +<histogram
name="Prerender.NoStatePrefetchTTFCPWarmCacheable" units="ms">
On 2016/09/16 14:51:58, droger wrote:
> On 2016/09/16 14:16:21, Alexei Svitkine (very slow) wrote:
> > On 2016/09/16 12:21:50, droger wrote:
> > > On 2016/09/15 16:53:28, Alexei Svitkine (very slow) wrote:
> > > > I suggest using the histogram_suffixes construct to make the XML a bit
> more
> > > > concise. See the top of the file for an example.
> > > > 
> > > > Also suggest adding an intermediate . as a delimiter to make these a bit
> > more
> > > > readable, e.g.:
> > > > 
> > > > rerender.NoStatePrefetchTTFCP.WarmCacheable
> > > 
> > > I added the dot separator.
> > > 
> > > However, I don't know if the suffix thing is possible, since these
> histograms
> > > already have a suffix.
> > > Can we do nested <suffix> constructs?
> > > Or should I do second suffix construct, and list all the combinations from
> the
> > > first suffix construct in the <affected-histograms>?
> > 
> > You can do multiple sets of suffixes and affected histograms can list
suffixed
> > entries that were created by a different suffixes definition.
> 
> Done.
> Does this break if the suffix definition are re-ordered in the future?

Nope - re-ordering suffixes is fine.

[mmenke, 2016-09-16 16:53:49]

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:48:
prerender::PrerenderManager* prerender_manager_;
On 2016/09/16 12:21:49, droger wrote:
> On 2016/09/15 17:20:44, mmenke wrote:
> > const?
> 
> It cannot be const, since I call a non-const function on the prerender manager
> (which cannot be made const).

I actually meant const after the asterisk (The address pointed at can't change).

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:570: base::TimeDelta time) {
Is this called on prefetches themselves?  That is, if we prefetch a URL twice in
a row (The second one after the old one is destroyed), will the second one
result in calling this method for the abbreviated prefetched page?

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:584:
histograms_->RecordFirstContentfulPaint(origin, is_no_store, time,
What do you plan to do with these numbers?  If we're just going to compare them
to PLT of random non-prefetched URLs, that probably won't work well - there's a
strong bias in the type of pages that are prerendered (landing pages, mostly).

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.h:224: base::TimeDelta time);
Oh, this does something completely different from what I assumed it did.  Could
we make it clearer that this is for recording the time of a non-prefetched page,
after it was previously prefetched, and rename the method as well to make that
clearer?

[droger, 2016-09-19 09:55:23]

Thanks!

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:48:
prerender::PrerenderManager* prerender_manager_;
On 2016/09/16 16:53:48, mmenke wrote:
> On 2016/09/16 12:21:49, droger wrote:
> > On 2016/09/15 17:20:44, mmenke wrote:
> > > const?
> > 
> > It cannot be const, since I call a non-const function on the prerender
manager
> > (which cannot be made const).
> 
> I actually meant const after the asterisk (The address pointed at can't
change).

Done.

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:570: base::TimeDelta time) {
On 2016/09/16 16:53:48, mmenke wrote:
> Is this called on prefetches themselves?  That is, if we prefetch a URL twice
in
> a row (The second one after the old one is destroyed), will the second one
> result in calling this method for the abbreviated prefetched page?

No, this is not called on the prefetch themselves.

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:584:
histograms_->RecordFirstContentfulPaint(origin, is_no_store, time,
On 2016/09/16 16:53:48, mmenke wrote:
> What do you plan to do with these numbers?  If we're just going to compare
them
> to PLT of random non-prefetched URLs, that probably won't work well - there's
a
> strong bias in the type of pages that are prerendered (landing pages, mostly).

Very good point.

I know we have plans to do UMA experiments where we don't actually prefetch
pages but record the ones that would have been prefetched (similar to the old
experiments for prerendering).

Doing this should enable comparisons between prefetched page and
"would-have-been-prefetched" page.

However, your concern is still valid. In particular it's not immediately clear
that the "Reference" histograms (recorded for pages that are not prefetched)
will ever be useful.

I can probably remove this particular histogram if this is a problem.

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.h:224: base::TimeDelta time);
On 2016/09/16 16:53:48, mmenke wrote:
> Oh, this does something completely different from what I assumed it did. 
Could
> we make it clearer that this is for recording the time of a non-prefetched
page,
> after it was previously prefetched, and rename the method as well to make that
> clearer?

Note that prefetched pages are not rendered at all, so the "first contentful
paint" does not mean anything for them.

Added a comment.

[Bryan McQuade, 2016-09-19 14:11:17]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Bryan McQuade, 2016-09-19 14:11:19]

Thanks, this looks good! One suggestion: could we add a test in
page_load_metrics_browsertest.cc that loads a test page with the appropriate
cache control header, and verify that this histogram gets logged? Feel free to
add a new html test file (and headers file) under the page_load_metrics test
directory for this.

https://codereview.chromium.org/2321083002/diff/300001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2321083002/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc:16:
page_load_metrics::PageLoadTracker* tracker,
PageLoadTracker isn't a class we want observers to interact with. It's a private
class subject to change / go away / etc. Exposing it constrains our ability to
factor the code in the future. Can you try to find another way to do this? Maybe
we can do this in a helper in page_load_metrics_initialize.cc? Or we can do what
csharrison suggested and add a CreateOrNull() method that returns an instance or
a null object if there's no PrerenderManager available. I think I prefer the
latter.

[mmenke, 2016-09-19 15:43:46]

prerender/ LGTM

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:570: base::TimeDelta time) {
On 2016/09/19 09:55:23, droger wrote:
> On 2016/09/16 16:53:48, mmenke wrote:
> > Is this called on prefetches themselves?  That is, if we prefetch a URL
twice
> in
> > a row (The second one after the old one is destroyed), will the second one
> > result in calling this method for the abbreviated prefetched page?
> 
> No, this is not called on the prefetch themselves.

Why isn't it?  Prefetches don't have first contentful paints?

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:584:
histograms_->RecordFirstContentfulPaint(origin, is_no_store, time,
On 2016/09/19 09:55:23, droger wrote:
> On 2016/09/16 16:53:48, mmenke wrote:
> > What do you plan to do with these numbers?  If we're just going to compare
> them
> > to PLT of random non-prefetched URLs, that probably won't work well -
there's
> a
> > strong bias in the type of pages that are prerendered (landing pages,
mostly).
> 
> Very good point.
> 
> I know we have plans to do UMA experiments where we don't actually prefetch
> pages but record the ones that would have been prefetched (similar to the old
> experiments for prerendering).
> 
> Doing this should enable comparisons between prefetched page and
> "would-have-been-prefetched" page.
> 
> However, your concern is still valid. In particular it's not immediately clear
> that the "Reference" histograms (recorded for pages that are not prefetched)
> will ever be useful.
> 
> I can probably remove this particular histogram if this is a problem.

I'm happy to defer to you on whether we should add this histogram, I just want
you to be aware that you can't really compare it to other ContentfulPaint
histograms.  This bias may be worth mentioning on the histogram description, at
least.

We've previously addressed this by making a group that "pretends" to prerender
pages, but doesn't really, and then comparing it with this.  In the prerender
case, we had trouble with cancellation reasons not being accounted for, but if
we did something similar for prefetch, I don't think we'd have similar issues,
since we're including prefetches that are cancelled early in prefetches. 
Anyhow, just throwing the idea out there.  I don't think we'd want to keep that
group around indefinitely, if we add it.

[droger, 2016-09-19 15:52:56]

Thanks.

Still working on the test, and will do another pass on histogram descriptions.

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:570: base::TimeDelta time) {
On 2016/09/19 15:43:46, mmenke wrote:
> On 2016/09/19 09:55:23, droger wrote:
> > On 2016/09/16 16:53:48, mmenke wrote:
> > > Is this called on prefetches themselves?  That is, if we prefetch a URL
> twice
> > in
> > > a row (The second one after the old one is destroyed), will the second one
> > > result in calling this method for the abbreviated prefetched page?
> > 
> > No, this is not called on the prefetch themselves.
> 
> Why isn't it?  Prefetches don't have first contentful paints?

For two reasons:
- prefetches are never rendered, and thus never painted
- the caller (PageLoadMetricsObserver) is disabled for prerender/prefetch loads.

https://codereview.chromium.org/2321083002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:584:
histograms_->RecordFirstContentfulPaint(origin, is_no_store, time,
On 2016/09/19 15:43:46, mmenke wrote:
> We've previously addressed this by making a group that "pretends" to prerender
> pages, but doesn't really, and then comparing it with this.

Yes, as far as I know we have the same plan for prefetch.

https://codereview.chromium.org/2321083002/diff/300001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2321083002/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc:16:
page_load_metrics::PageLoadTracker* tracker,
On 2016/09/19 14:11:18, Bryan McQuade wrote:
> PageLoadTracker isn't a class we want observers to interact with. It's a
private
> class subject to change / go away / etc. Exposing it constrains our ability to
> factor the code in the future. Can you try to find another way to do this?
Maybe
> we can do this in a helper in page_load_metrics_initialize.cc? Or we can do
what
> csharrison suggested and add a CreateOrNull() method that returns an instance
or
> a null object if there's no PrerenderManager available. I think I prefer the
> latter.

Done.

[droger, 2016-09-19 16:28:02]

bmcquade: added the browser test, please take a look.

[droger, 2016-09-19 16:28:14]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 16:28:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 16:30:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 16:30:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Bryan McQuade, 2016-09-19 16:35:03]

Thanks very much for the tests!

LGTM once comments are addressed.

Thanks again!

https://codereview.chromium.org/2321083002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2321083002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:435:
IN_PROC_BROWSER_TEST_F(MetricsWebContentsObserverBrowserTest,
i just landed a change that renamed these tests to PageLoadMetricsBrowserTest -
you'll need to rebase and then rename your tests to match, thanks!

https://codereview.chromium.org/2321083002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:443:
histogram_tester_.ExpectTotalCount(
for completeness, can we also verify that the count for the no-store variant is
zero here?

https://codereview.chromium.org/2321083002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:457:
histogram_tester_.ExpectTotalCount(
likewise, let's verify that the non-no-store variant has a count of zero here.

https://codereview.chromium.org/2321083002/diff/340001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.h (right):

https://codereview.chromium.org/2321083002/diff/340001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.h:137: static std::string
GetFirstContentfulPaintHistogrameName(
GetFirstContentfulPaintHistogrameName -> GetFirstContentfulPaintHistogramName
(drop 'e' at end of Histogram).

[Charlie Harrison, 2016-09-19 16:40:06]

LGTM w/ nit

https://codereview.chromium.org/2321083002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2321083002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:34:
// page_load_metrics::PageLoadMetricsObserver overrides:
nit: remove "overrides"

[droger, 2016-09-20 11:56:25]

Description was changed from

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store" headers.

BUG=642371
==========

to

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store" headers.

BUG=642371
TBR=jochen
==========

[droger, 2016-09-20 11:56:26]

droger@chromium.org changed reviewers:
+ jochen@chromium.org

[droger, 2016-09-20 11:58:33]

TBR jochen for updated callsites of PrerenderManagerFactory:

chrome/browser/android/offline_pages/prerender_adapter.cc
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc
chrome/browser/android/tab_android.cc
chrome/browser/android/tab_web_contents_delegate_android.cc
chrome/browser/browsing_data/browsing_data_remover.cc
chrome/browser/extensions/activity_log/activity_log.cc
chrome/browser/extensions/activity_log/activity_log_browsertest.cc
chrome/browser/extensions/activity_log/activity_log_unittest.cc
chrome/browser/history/history_tab_helper.cc
chrome/browser/predictors/autocomplete_action_predictor.cc
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
chrome/browser/ui/browser_navigator.cc
chrome/browser/ui/search/instant_search_prerenderer.cc
chrome/browser/ui/search/instant_search_prerenderer_unittest.cc

[droger, 2016-09-20 11:59:09]

The CQ bit was checked by droger@chromium.org

[droger, 2016-09-20 11:59:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org, asvitkine@chromium.org, mmenke@chromium.org,
csharrison@chromium.org, bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2321083002/#ps360001
(title: "Review comments")

[commit-bot: I haz the power, 2016-09-20 11:59:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 13:03:55]

Description was changed from

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store" headers.

BUG=642371
TBR=jochen
==========

to

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store" headers.

BUG=642371
TBR=jochen
==========

[commit-bot: I haz the power, 2016-09-20 13:03:57]

Committed patchset #12 (id:360001)

[commit-bot: I haz the power, 2016-09-20 13:05:34]

Description was changed from

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store" headers.

BUG=642371
TBR=jochen
==========

to

==========
[NoStatePrefetch] Add performance histograms.

This CL adds histograms to record performance of NoStatePrefetch.

The histograms added are:

NoStatePrefetchAge: age of the prefetch when it is used

NoStatePrefetchTTFCP: Time To First Contentful Paint, with variations:
- Cold/Warm/Reference:
    * Warm: Prefetch is recent enough to skip revalidation
    * Cold: Prefetch is old and does not skip revalidation
    * Reference: there was no prefetch
- Cacheable/NoStore: whether the main resource had "no-store" headers.

BUG=642371
TBR=jochen

Committed: https://crrev.com/c154315d263486c4ecd1cc9b1f73e82efeb2468a
Cr-Commit-Position: refs/heads/master@{#419733}
==========

[commit-bot: I haz the power, 2016-09-20 13:05:38]

Patchset 12 (id:??) landed as
https://crrev.com/c154315d263486c4ecd1cc9b1f73e82efeb2468a
Cr-Commit-Position: refs/heads/master@{#419733}