Based on email discussions, we want omnibox breakout metrics, and we want to understand the impact of prerender on improving page load time in particular.

We have almost everything we need for this already - we just need to plumb an indicator that a given load is a prerender through to observers.

To do this, we can pass a boolean is_prerender to the constructor of PageLoadMetricsObservers that want to track both prerenders and non-prerenders.

We can do this in PageLoadMetricsEmbedder::RegisterObservers, where we already detect whether a given load is a prerender.

Then, when logging a metric for a prerender in the observer, we
1. test whether this is_prerender boolean is set to true (we'll want to keep this bool as a member variable in our observer)
2. if so, test whether PageLoadExtraInfo::started_in_foreground is false
3. if so, we use PageLoadExtraInfo::first_foreground_time as the first visible time for the navigation

For these new prerender metrics, we can use a name like:
Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint

this metric should log all matching omnibox navs, not just prerenders. it should use navigation_start for loads where started_in_foreground is true, and first_foreground_time for navs where started_in_foreground is false.

We should also have a breakout:
Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint.Prerender
which measures the above, but for prerenders only.

This lets us understand aggregate user perceived load performance of all omnibox loads, and also gives us the breakout for only those loads where a prerender was used.

I'd also like to add a
Omnibox.SuggestionUsed.Search.NavigationToFirstForeground.Prerender
which will measure the amount of loading time saved thanks to prerender.

Let's also shorten the histogram names generally to use Omnibox.SuggestionUsed rather than the full PageLoad.Clients prefix - while I like using this prefix, the histogram names became so insanely long as to be pretty inconvenient.

lpy, mpearson, how does this all sound?

> we can pass a boolean is_prerender to the constructor of
> PageLoadMetricsObservers that want to track both prerenders and
> non-prerenders.

[snip]

> Then, when logging a metric for a prerender in the observer, we 
>
> 1. test whether this is_prerender boolean is set to true (we'll want to keep
> this bool as a member variable in our observer)

Is my understanding correct that you suggest to attach is_prerender flag to the
observer (which is attached to a WebContents)?

So if prerender was requested, (created a renderer, did fancy things, consumed
CPU) and then was killed (because failed to commit or due to other
Prerender.FinalStatus reasons), the FCP would be recorded without attribution to
Prerender?

In that case is_prerender would be true but we'd never have a first_foreground_time. We can log the frequency of those cases as well so we know how often we're prerendering and discarding for omnibox (perhaps we just count these, since timing metrics are less interesting here?)

So, to clarify, we'd only log a prerender metric if we have a first_foreground_time, which indicates that the prerender was used.

> mpearson, how does this all sound?

Sounds fine to me, but note that I would like separate timing histograms for searches from the omnibox (type GENERATED) and for URLs from the omnibox (type TYPED), per my original e-mail.

Yep, agree - we will provide that breakout as well.

sgtm

Quick question, does this also mean I should create an entry in histograms.xml like

<histogram name="PageLoadTiming.Timing.ForegroundToFirstContentfulPaint" units="ms">
</histogram>

Good question. We used to add these but stopped doing that & switched to only adding histograms we log data for. So you can create a
<histogram name="Omnibox. ... .ForegroundToFirstContentfulPaint" units="ms">
</histogram>

entry directly since this one doesn't have a base or parent histogram to reference.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71bcf50b0c34114c514e431af16527792c55006a

commit 71bcf50b0c34114c514e431af16527792c55006a
Author: lpy <lpy@chromium.org>
Date: Tue Apr 04 03:29:05 2017

[Page Load Metrics] Add page load metrics for omnibox.

Add metrics to track time to first contentful/meaningful paint from pages
where users select suggestion from omnibox.

BUG= 702440 

Review-Url: https://codereview.chromium.org/2744983002
Cr-Commit-Position: refs/heads/master@{#461621}

[modify] https://crrev.com/71bcf50b0c34114c514e431af16527792c55006a/chrome/browser/BUILD.gn
[add] https://crrev.com/71bcf50b0c34114c514e431af16527792c55006a/chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc
[add] https://crrev.com/71bcf50b0c34114c514e431af16527792c55006a/chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h
[modify] https://crrev.com/71bcf50b0c34114c514e431af16527792c55006a/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/71bcf50b0c34114c514e431af16527792c55006a/tools/metrics/histograms/histograms.xml

Can we cherry-pick to M58? Is there anything beyond that left to be done for this issue?

I added a merge-request label.

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

merge is approved to m58, was it merged?

Cherrypick is in review - https://codereview.chromium.org/2802833002 - once lpy confirms that it builds cleanly we'll land.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02b1616fb612bfde61b74086016a66230fb41cea

commit 02b1616fb612bfde61b74086016a66230fb41cea
Author: bmcquade <bmcquade@chromium.org>
Date: Thu Apr 06 16:53:36 2017

Add owners for new omnibox metrics.

These were mistakenly left out of https://codereview.chromium.org/2744983002

Note that, without an owner, metrics do not show up in the UMA dash by
default, so it's important that we have owners for all metrics that we
intend to use. lpy and mpearson, I'm happy to add myself as a third owner
on these if you prefer.

BUG= 702440 

Review-Url: https://codereview.chromium.org/2802863005
Cr-Commit-Position: refs/heads/master@{#462512}

[modify] https://crrev.com/02b1616fb612bfde61b74086016a66230fb41cea/tools/metrics/histograms/histograms.xml

Your change is approved for M58. Please merge ASAP so that it will be picked up for next Beta Release.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4981296103dd498f1dd18329d39a58006caa9394

commit 4981296103dd498f1dd18329d39a58006caa9394
Author: lpy <lpy@chromium.org>
Date: Thu Apr 06 23:06:38 2017

[Page Load Metrics] Add page load metrics for omnibox.

Add metrics to track time to first contentful/meaningful paint from pages
where users select suggestion from omnibox.

BUG= 702440 
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2744983002
Cr-Commit-Position: refs/heads/master@{#461621}
(cherry picked from commit 71bcf50b0c34114c514e431af16527792c55006a)

Review-Url: https://codereview.chromium.org/2802833002
Cr-Commit-Position: refs/branch-heads/3029@{#617}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/4981296103dd498f1dd18329d39a58006caa9394/chrome/browser/BUILD.gn
[add] https://crrev.com/4981296103dd498f1dd18329d39a58006caa9394/chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc
[add] https://crrev.com/4981296103dd498f1dd18329d39a58006caa9394/chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h
[modify] https://crrev.com/4981296103dd498f1dd18329d39a58006caa9394/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/4981296103dd498f1dd18329d39a58006caa9394/tools/metrics/histograms/histograms.xml

I merged my patch to branch 3029, Bryan@, shall I merge yours?

histograms.xml changes are not snapshotted at a branch; the dashboard uses the most recent version available.  That is, the owners changes don't need to be merged.