Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744983002:
[Page Load Metrics] Add page load metrics for omnibox. (Closed)
Description[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}
Committed: https://chromium.googlesource.com/chromium/src/+/71bcf50b0c34114c514e431af16527792c55006a
Patch Set 1 #
Total comments: 12
Patch Set 2 : update #
Total comments: 8
Patch Set 3 : Addressed comments #Patch Set 4 : update metrics #
Total comments: 4
Patch Set 5 : Address comments #
Total comments: 10
Patch Set 6 : Addressed comments #Patch Set 7 : update #
Total comments: 4
Patch Set 8 : removed unnecessary override and check first_foreground_time #
Total comments: 20
Patch Set 9 : update histogram names #
Total comments: 4
Patch Set 10 : Addressed comments #
Total comments: 31
Patch Set 11 : addressed comments #
Total comments: 4
Patch Set 12 : Addressed comments #
Messages
Total messages: 103 (58 generated)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lpy@chromium.org changed reviewers: + bmcquade@chromium.org, mpearson@chromium.org
PTAL.
Thanks lpy for putting this change together! https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:17: return CONTINUE_OBSERVING; I think we probably also want something like: return (transition_type_ & ui::PAGE_TRANSITION_FROM_ADDRESS_BAR != 0) ? CONTINUE_OBSERVING : STOP_OBSERVING; Mark, should we only track metrics for cases where this qualifier is set? I think I see cases in the code where PAGE_TRANSITION_TYPED is used that seem to be non-omnibox contexts, so it seems like that might not be sufficient to restrict to typed omnibox URLs, and we may want this extra qualifier as well. https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:20: void OmniboxSuggestedUsedMetricsObserver::OnFirstMeaningfulPaint( this is great, thanks! let's also log FCP since it's our current best non experimental page load metric. https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:23: if (ui::PageTransitionCoreTypeIs(transition_type_, our default behavior has been to only log metrics if the event was reached in the foreground. if we log metrics where the page spends some time in the background, we tend to get very strange outliers as the paint is disabled when pages are in the background. you can accomplish logging only in foreground in 2 different ways (your choice): 1. return STOP_OBSERVING from OnStart if started_in_foreground is true, and return STOP_OBSERVING from OnHidden 2. use the WasStartedInForegroundOptionalEventInForeground helper method https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:26: "PageLoad.Clients.Omnibox.SuggestedUsed.Search.Experimental." re: histogram naming, mark, our convention for page load metrics specific to a chrome team is to prefix them with PageLoad.Clients, so in this case PageLoad.Clients.Omnibox. Having them in the PageLoad namespace makes it a bit clearer that these are core page load metrics. That said, Omnibox already has histograms under Omnibox.SuggestionUsed, so if that's a better place for the team to have these, it's fine to put them there. Mark, what's your naming preference? One other possible naming variation to consider: our convention re: transition types has been to suffix the histogram with LoadType.<Type> so for example ...NavigationToFirstMeaningfulPaint.LoadType.Typed ...NavigationToFirstMeaningfulPaint.LoadType.Generated Are we ok with using this naming convention here instead? I'm also fine with the naming as proposed in this patch if there's a strong preference for it instead. https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.h (right): https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.h:25: ui::PageTransition transition_type_; let's set this to a default value - for historical reasons it seems that PAGE_TRANSITION_LINK is the default value, so let's use that here.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
I made an update, ptal. https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:17: return CONTINUE_OBSERVING; On 2017/03/13 13:07:42, Bryan McQuade wrote: > I think we probably also want something like: > > return (transition_type_ & ui::PAGE_TRANSITION_FROM_ADDRESS_BAR != 0) ? > CONTINUE_OBSERVING : STOP_OBSERVING; > > Mark, should we only track metrics for cases where this qualifier is set? I > think I see cases in the code where PAGE_TRANSITION_TYPED is used that seem to > be non-omnibox contexts, so it seems like that might not be sufficient to > restrict to typed omnibox URLs, and we may want this extra qualifier as well. Changed it for now and wait for further feedback from Mark. https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:20: void OmniboxSuggestedUsedMetricsObserver::OnFirstMeaningfulPaint( On 2017/03/13 13:07:42, Bryan McQuade wrote: > this is great, thanks! let's also log FCP since it's our current best non > experimental page load metric. Done. https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:23: if (ui::PageTransitionCoreTypeIs(transition_type_, On 2017/03/13 13:07:42, Bryan McQuade wrote: > our default behavior has been to only log metrics if the event was reached in > the foreground. if we log metrics where the page spends some time in the > background, we tend to get very strange outliers as the paint is disabled when > pages are in the background. you can accomplish logging only in foreground in 2 > different ways (your choice): > 1. return STOP_OBSERVING from OnStart if started_in_foreground is true, and > return STOP_OBSERVING from OnHidden > 2. use the WasStartedInForegroundOptionalEventInForeground helper method Done. https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:26: "PageLoad.Clients.Omnibox.SuggestedUsed.Search.Experimental." On 2017/03/13 13:07:42, Bryan McQuade wrote: > re: histogram naming, mark, our convention for page load metrics specific to a > chrome team is to prefix them with PageLoad.Clients, so in this case > PageLoad.Clients.Omnibox. Having them in the PageLoad namespace makes it a bit > clearer that these are core page load metrics. > > That said, Omnibox already has histograms under Omnibox.SuggestionUsed, so if > that's a better place for the team to have these, it's fine to put them there. > > Mark, what's your naming preference? > > One other possible naming variation to consider: our convention re: transition > types has been to suffix the histogram with LoadType.<Type> so for example > > ...NavigationToFirstMeaningfulPaint.LoadType.Typed > ...NavigationToFirstMeaningfulPaint.LoadType.Generated > > Are we ok with using this naming convention here instead? I'm also fine with the > naming as proposed in this patch if there's a strong preference for it instead. Wait for Mark's reply. https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.h (right): https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.h:25: ui::PageTransition transition_type_; On 2017/03/13 13:07:42, Bryan McQuade wrote: > let's set this to a default value - for historical reasons it seems that > PAGE_TRANSITION_LINK is the default value, so let's use that here. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@Mark, gentle ping for review.
https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:17: return CONTINUE_OBSERVING; On 2017/03/13 17:24:08, lpy wrote: > On 2017/03/13 13:07:42, Bryan McQuade wrote: > > I think we probably also want something like: > > > > return (transition_type_ & ui::PAGE_TRANSITION_FROM_ADDRESS_BAR != 0) ? > > CONTINUE_OBSERVING : STOP_OBSERVING; > > > > Mark, should we only track metrics for cases where this qualifier is set? I > > think I see cases in the code where PAGE_TRANSITION_TYPED is used that seem to > > be non-omnibox contexts, so it seems like that might not be sufficient to > > restrict to typed omnibox URLs, and we may want this extra qualifier as well. > > Changed it for now and wait for further feedback from Mark. I'm not sure if this qualifier is necessary. I've never used it before. (Indeed, I didn't even know we had a flag like that.) It seems sensible though. Can you confirm with someone on the navigation team that this flag is passed correctly (e.g., kept through redirects and whatever else you need)? https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:26: "PageLoad.Clients.Omnibox.SuggestedUsed.Search.Experimental." On 2017/03/13 17:24:08, lpy wrote: > On 2017/03/13 13:07:42, Bryan McQuade wrote: > > re: histogram naming, mark, our convention for page load metrics specific to a > > chrome team is to prefix them with PageLoad.Clients, so in this case > > PageLoad.Clients.Omnibox. Having them in the PageLoad namespace makes it a bit > > clearer that these are core page load metrics. > > > > That said, Omnibox already has histograms under Omnibox.SuggestionUsed, so if > > that's a better place for the team to have these, it's fine to put them there. > > > > Mark, what's your naming preference? I don't have a strong preference. If you prefer your method, go with it. I like how in either case, typing "Omnibox.Sugg" in the dashboard will bring up the metric. > > One other possible naming variation to consider: our convention re: transition > > types has been to suffix the histogram with LoadType.<Type> so for example > > > > ...NavigationToFirstMeaningfulPaint.LoadType.Typed > > ...NavigationToFirstMeaningfulPaint.LoadType.Generated > > > > Are we ok with using this naming convention here instead? I would prefer the Search vs. URL name, as many of the people who look up the Omnibox.SuggestionUsed metrics aren't familiar with the transition type names. Also, note that the histograms here you named "SuggestedUsed", not "SuggestionUsed". Please correct that, thanks. > > I'm also fine with > the > > naming as proposed in this patch if there's a strong preference for it > instead. > > Wait for Mark's reply. https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:17: bool started_in_foreground) { How does |started_in_foreground| work with the omnibox? The omnibox is always in the foreground. Is this always true? Or does this distinguish between, say, middle-clicking the omnibox, which causes the clicked suggestion to open the background and regular suggestions? https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.h (right): https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.h:5: #ifndef CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_OMNIBOX_SUGGESTED_USED_PAGE_LOAD_METRICS_OBSERVER_H_ nit: prefer "omnibox suggestion used" (not "suggested used"--I'm not sure what that means)
On 2017/03/14 18:36:49, Mark P wrote: > https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... > File > chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2744983002/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:17: > return CONTINUE_OBSERVING; > On 2017/03/13 17:24:08, lpy wrote: > > On 2017/03/13 13:07:42, Bryan McQuade wrote: > > > I think we probably also want something like: > > > > > > return (transition_type_ & ui::PAGE_TRANSITION_FROM_ADDRESS_BAR != 0) ? > > > CONTINUE_OBSERVING : STOP_OBSERVING; > > > > > > Mark, should we only track metrics for cases where this qualifier is set? I > > > think I see cases in the code where PAGE_TRANSITION_TYPED is used that seem > to > > > be non-omnibox contexts, so it seems like that might not be sufficient to > > > restrict to typed omnibox URLs, and we may want this extra qualifier as > well. > > > > Changed it for now and wait for further feedback from Mark. > > I'm not sure if this qualifier is necessary. I've never used it before. > (Indeed, I didn't even know we had a flag like that.) It seems sensible though. > Can you confirm with someone on the navigation team that this flag is passed > correctly (e.g., kept through redirects and whatever else you need)? @Bryan, whom would you suggest confirming with?
This seems good, thanks! Let's inquire about the PAGE_TRANSITION_FROM_ADDRESS_BAR just to be sure that will cover all intended cases. I'm not totally sure who to ask - navigation team would mean nasko to start I think. https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:17: bool started_in_foreground) { On 2017/03/14 at 18:36:47, Mark P wrote: > How does |started_in_foreground| work with the omnibox? The omnibox is always in the foreground. Is this always true? Or does this distinguish between, say, middle-clicking the omnibox, which causes the clicked suggestion to open the background and regular suggestions? Right, I expect that a middle click on the omnibox would cause started_in_foreground to be false. https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:43: "PageLoad.Clients.Omnibox.SuggestedUsed.Search." these names seem fine to me, just changing 'SuggestedUsed' to 'SuggestionUsed' as mark requested. https://codereview.chromium.org/2744983002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:119757: + name="PageLoad.PaintTiming.NavigationToFirstContentfulPaint"/> the histograms you've declared in code are missing the 'PaintTiming' part of the name - you'd need to add that in for the suffixes to work let's also add an entry for first meaningful paint here
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.cc:43: "PageLoad.Clients.Omnibox.SuggestedUsed.Search." On 2017/03/15 01:44:25, Bryan McQuade wrote: > these names seem fine to me, just changing 'SuggestedUsed' to 'SuggestionUsed' > as mark requested. Done. https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.h (right): https://codereview.chromium.org/2744983002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggested_used_page_load_metrics_observer.h:5: #ifndef CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_OMNIBOX_SUGGESTED_USED_PAGE_LOAD_METRICS_OBSERVER_H_ On 2017/03/14 18:36:47, Mark P wrote: > nit: prefer "omnibox suggestion used" (not "suggested used"--I'm not sure what > that means) Done. https://codereview.chromium.org/2744983002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:119757: + name="PageLoad.PaintTiming.NavigationToFirstContentfulPaint"/> On 2017/03/15 01:44:25, Bryan McQuade wrote: > the histograms you've declared in code are missing the 'PaintTiming' part of the > name - you'd need to add that in for the suffixes to work > > let's also add an entry for first meaningful paint here Done.
Ideally, the first use case for these metrics is to evaluate the benefit from the current Chrome prerender compared to doing nothing at all, and the benefit from the proposed NoState prefetch compared to those. Can you please confirm that this metric will measure those things correctly, i.e., measure the time from when the user hits enter in the omnibox / clicks on an omnibox suggestion to the time the first contentful page is displayed? The timer shouldn't start from the beginning of prefetch or preload, just from when the user says "navigate me now". thanks, mark
On 2017/03/15 at 20:09:37, mpearson wrote: > Ideally, the first use case for these metrics is to evaluate the benefit from the current Chrome prerender compared to doing nothing at all, and the benefit from the proposed NoState prefetch compared to those. > > Can you please confirm that this metric will measure those things correctly, i.e., measure the time from when the user hits enter in the omnibox / clicks on an omnibox suggestion to the time the first contentful page is displayed? The timer shouldn't start from the beginning of prefetch or preload, just from when the user says "navigate me now". > > thanks, > mark Thanks Mark for sharing planned use. Prerenders are not tracked by default; there's another small change we'd need to make to allow for that. So I am glad you mentioned this! Is it useful to you to have a further breakout for just the subset of navigations from the omnibox that use a prerender? If that's useful, it should be straightforward for us to add.
On 2017/03/15 at 20:16:08, Bryan McQuade wrote: > On 2017/03/15 at 20:09:37, mpearson wrote: > > Ideally, the first use case for these metrics is to evaluate the benefit from the current Chrome prerender compared to doing nothing at all, and the benefit from the proposed NoState prefetch compared to those. > > > > Can you please confirm that this metric will measure those things correctly, i.e., measure the time from when the user hits enter in the omnibox / clicks on an omnibox suggestion to the time the first contentful page is displayed? The timer shouldn't start from the beginning of prefetch or preload, just from when the user says "navigate me now". > > > > thanks, > > mark > > Thanks Mark for sharing planned use. Prerenders are not tracked by default; there's another small change we'd need to make to allow for that. So I am glad you mentioned this! > > Is it useful to you to have a further breakout for just the subset of navigations from the omnibox that use a prerender? If that's useful, it should be straightforward for us to add. RE: the PAGE_TRANSITION_FROM_ADDRESS_BAR, it was added here: https://codereview.chromium.org/7624031 - we still need to dig in and understand if this is reliably set in all cases.
On 2017/03/15 20:16:08, Bryan McQuade wrote: > On 2017/03/15 at 20:09:37, mpearson wrote: > > Ideally, the first use case for these metrics is to evaluate the benefit from > the current Chrome prerender compared to doing nothing at all, and the benefit > from the proposed NoState prefetch compared to those. > > > > Can you please confirm that this metric will measure those things correctly, > i.e., measure the time from when the user hits enter in the omnibox / clicks on > an omnibox suggestion to the time the first contentful page is displayed? The > timer shouldn't start from the beginning of prefetch or preload, just from when > the user says "navigate me now". > > > > thanks, > > mark > > Thanks Mark for sharing planned use. Prerenders are not tracked by default; > there's another small change we'd need to make to allow for that. So I am glad > you mentioned this! > > Is it useful to you to have a further breakout for just the subset of > navigations from the omnibox that use a prerender? If that's useful, it should > be straightforward for us to add. Sure, that'd be useful. It would both provide a second estimate of how often prerender / preload / whatever triggers (by comparing the total counts) and also provide some idea of how much time prerender saves when it triggers. (For the latter, we'd compare this breakout against the overall breakout. Oh, that's not quite a right comparison point because it includes the prerender items in the overall load times. Maybe we should also have a breakout for cases that did not use any of those prerender/preload things. That's a better comparison point. Admittedly, it's still not perfect, as the types of URL and queries on which prerender or preload triggers might be systematically different than other queries, but at least it's closer than comparing the prerender breakout with the overall histogram.) thanks for asking, mark
On 2017/03/15 at 20:26:52, mpearson wrote: > On 2017/03/15 20:16:08, Bryan McQuade wrote: > > On 2017/03/15 at 20:09:37, mpearson wrote: > > > Ideally, the first use case for these metrics is to evaluate the benefit from > > the current Chrome prerender compared to doing nothing at all, and the benefit > > from the proposed NoState prefetch compared to those. > > > > > > Can you please confirm that this metric will measure those things correctly, > > i.e., measure the time from when the user hits enter in the omnibox / clicks on > > an omnibox suggestion to the time the first contentful page is displayed? The > > timer shouldn't start from the beginning of prefetch or preload, just from when > > the user says "navigate me now". > > > > > > thanks, > > > mark > > > > Thanks Mark for sharing planned use. Prerenders are not tracked by default; > > there's another small change we'd need to make to allow for that. So I am glad > > you mentioned this! > > > > Is it useful to you to have a further breakout for just the subset of > > navigations from the omnibox that use a prerender? If that's useful, it should > > be straightforward for us to add. > > Sure, that'd be useful. It would both provide a second estimate of how often prerender / preload / whatever triggers (by comparing the total counts) and also provide some idea of how much time prerender saves when it triggers. > > (For the latter, we'd compare this breakout against the overall breakout. Oh, that's not quite a right comparison point because it includes the prerender items in the overall load times. Maybe we should also have a breakout for cases that did not use any of those prerender/preload things. That's a better comparison point. Admittedly, it's still not perfect, as the types of URL and queries on which prerender or preload triggers might be systematically different than other queries, but at least it's closer than comparing the prerender breakout with the overall histogram.) > > thanks for asking, > mark Great. lpy@, integrating with prerender changes things a little bit - I'll start an email thread w/ some details on that. Looks like FROM_ADDRESS_BAR is set in the places we expect. For example, when initiating a prerender: https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte... As well as in various omnibox code, such as (among other places): https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_...
Can you please create a bug for this work? The main reason I ask is that there's a reasonable chance that Darin and Rahul may ask to merge this into M-58 to help evaluate the effect of prerender and its proposed shutdown. Having a bug would make that easier. Of course, I should ask: do you foresee any problem with being able to merge this into M-58 if requested? Does it touch code that is likely to have been changed since the branch point / use new APIs / whatever? thanks, mark
Description was changed from ========== [Page Load Metrics] Add page load metrics for omnibox experiment. Add metrics to track time to first meaningful paint from pages where users select suggestion from omnibox. ========== to ========== [Page Load Metrics] Add page load metrics for omnibox experiment. Add metrics to track time to first meaningful paint from pages where users select suggestion from omnibox. BUG=702440 ==========
On 2017/03/16 23:43:15, Mark P wrote: > Can you please create a bug for this work? > > The main reason I ask is that there's a reasonable chance that Darin and Rahul > may ask to merge this into M-58 to help evaluate the effect of prerender and its > proposed shutdown. Having a bug would make that easier. Done, https://bugs.chromium.org/p/chromium/issues/detail?id=702440 > Of course, I should ask: do you foresee any problem with being able to merge > this into M-58 if requested? Does it touch code that is likely to have been > changed since the branch point / use new APIs / whatever? I am working on a prototype to migrate page load metrics system to use mojo, but it will only change the pipeline behind the scene, it will not change any API that currently page load metrics observers are depending on. This is the only change to the page load metrics system I am currently aware of. Bryan, any thing else?
> > Of course, I should ask: do you foresee any problem with being able to merge > > this into M-58 if requested? Does it touch code that is likely to have been > > changed since the branch point / use new APIs / whatever? > > I am working on a prototype to migrate page load metrics system to use mojo, but > it will only change the pipeline behind the scene, it will not change any API > that currently page load metrics observers are depending on. > > This is the only change to the page load metrics system I am currently aware of. > Bryan, any thing else? Note that the prototype is still at an early stage, it's currently not for review/commit.
Description was changed from ========== [Page Load Metrics] Add page load metrics for omnibox experiment. Add metrics to track time to first meaningful paint from pages where users select suggestion from omnibox. BUG=702440 ========== to ========== [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 ==========
Description was changed from ========== [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 ========== to ========== [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 ==========
> Great. lpy@, integrating with prerender changes things a little bit - I'll start > an email thread w/ some details on that. > > Looks like FROM_ADDRESS_BAR is set in the places we expect. For example, when > initiating a prerender: > https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte... > > As well as in various omnibox code, such as (among other places): > https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_... According to navigation folks in the email thread, FROM_ADDRESS_BAR is what we want.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
I updated the patch, ptal.
thanks! a couple things, but this looks very good overall. https://codereview.chromium.org/2744983002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: PAGE_LOAD_HISTOGRAM(kPrerenderSearchFirstContentfulPaint, delta); in addition to these let's log the user perceived FCP, which is: std::max(0, timing.first_contentful_paint.value() - info.first_foreground_time.value()); https://codereview.chromium.org/2744983002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h (right): https://codereview.chromium.org/2744983002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h:34: bool is_prerender_; nit: since this is only set in the constructor, you can declare it as 'const bool is_prerender_;'
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> in addition to these let's log the user perceived FCP, which is: > > std::max(0, timing.first_contentful_paint.value() - > info.first_foreground_time.value()); Does this also mean first_meaningful_paint can also happen before first_foreground_time? https://codereview.chromium.org/2744983002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: PAGE_LOAD_HISTOGRAM(kPrerenderSearchFirstContentfulPaint, delta); On 2017/03/22 19:43:37, Bryan McQuade wrote: > in addition to these let's log the user perceived FCP, which is: > > std::max(0, timing.first_contentful_paint.value() - > info.first_foreground_time.value()); Done. https://codereview.chromium.org/2744983002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h (right): https://codereview.chromium.org/2744983002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h:34: bool is_prerender_; On 2017/03/22 19:43:37, Bryan McQuade wrote: > nit: since this is only set in the constructor, you can declare it as 'const > bool is_prerender_;' Done.
Thanks! Just some minimal restructuring, but otherwise this looks good. I think we're about ready to commit. https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:51: return started_in_foreground ? CONTINUE_OBSERVING : STOP_OBSERVING; ah, subtlety here - since we are now tracking both prerenders and non prerenders, we can no longer stop observing when started_in_foreground is false, as that's going to be the case for prerenders. https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:75: if (is_prerender_) { given the started_in_foreground change above, let's add: if (is_prerender_ && !started_in_foreground) return; https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:77: delta -= info.first_foreground_time.value(); i find the logic here a little confusing. let's keep the true and perceived FCP values separate. let's rename what you have as 'delta' to 'fcp' at the top of the function, and then add this: if (info.first_foreground_time) { base::TimeDelta perceived_fcp = std::max(base::TimeDelta(), fcp - info.first_foreground_time.value()); PAGE_LOAD_HISTOGRAM(kPrerenderSearchNavigationToFirstForeground, info.first_foreground_time.value()); PAGE_LOAD_HISTOGRAM(kPrerenderSearchFirstContentfulPaint, perceived_fcp); } https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:93: if (ui::PageTransitionCoreTypeIs(transition_type_, the remaining logic should be wrapped in if (started_in_foreground) perhaps the flow could be: if (started_in_foreground) { // log the regular foreground metrics } else if (is_prerender_ && info.first_foreground_time) { // log prerender metrics } https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:102: void OmniboxSuggestionUsedMetricsObserver::OnFirstMeaningfulPaint( let's make the same changes here
https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:75: if (is_prerender_) { On 2017/03/24 16:12:04, Bryan McQuade wrote: > given the started_in_foreground change above, let's add: > > if (is_prerender_ && !started_in_foreground) > return; I am a little confused here, my dump question is, if info.started_in_foreground is false, and is_prerender_ is true, this is the case we should log the metrics for prerender, so if we return we will miss metrics for prerender, is that correct?
On 2017/03/24 at 18:52:51, lpy wrote: > https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:75: if (is_prerender_) { > On 2017/03/24 16:12:04, Bryan McQuade wrote: > > given the started_in_foreground change above, let's add: > > > > if (is_prerender_ && !started_in_foreground) > > return; > > I am a little confused here, my dump question is, if info.started_in_foreground is false, and is_prerender_ is true, this is the case we should log the metrics for prerender, so if we return we will miss metrics for prerender, is that correct? I'm sorry, my suggestion here is wrong. Please disregard, thanks!
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the feedback. I have a dumb question, can first meaningful paint happen before first foreground? https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:51: return started_in_foreground ? CONTINUE_OBSERVING : STOP_OBSERVING; On 2017/03/24 16:12:04, Bryan McQuade wrote: > ah, subtlety here - since we are now tracking both prerenders and non > prerenders, we can no longer stop observing when started_in_foreground is false, > as that's going to be the case for prerenders. Done. https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:77: delta -= info.first_foreground_time.value(); On 2017/03/24 16:12:04, Bryan McQuade wrote: > i find the logic here a little confusing. let's keep the true and perceived FCP > values separate. let's rename what you have as 'delta' to 'fcp' at the top of > the function, and then add this: > > if (info.first_foreground_time) { > base::TimeDelta perceived_fcp = std::max(base::TimeDelta(), fcp - > info.first_foreground_time.value()); > PAGE_LOAD_HISTOGRAM(kPrerenderSearchNavigationToFirstForeground, > info.first_foreground_time.value()); > PAGE_LOAD_HISTOGRAM(kPrerenderSearchFirstContentfulPaint, perceived_fcp); > } Done. https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:93: if (ui::PageTransitionCoreTypeIs(transition_type_, On 2017/03/24 16:12:04, Bryan McQuade wrote: > the remaining logic should be wrapped in if (started_in_foreground) > > perhaps the flow could be: > > if (started_in_foreground) { > // log the regular foreground metrics > } else if (is_prerender_ && info.first_foreground_time) { > // log prerender metrics > } Done. https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:102: void OmniboxSuggestionUsedMetricsObserver::OnFirstMeaningfulPaint( On 2017/03/24 16:12:04, Bryan McQuade wrote: > let's make the same changes here Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/24 at 20:51:49, lpy wrote: > Thanks for the feedback. I have a dumb question, can first meaningful paint happen before first foreground? > > https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:51: return started_in_foreground ? CONTINUE_OBSERVING : STOP_OBSERVING; > On 2017/03/24 16:12:04, Bryan McQuade wrote: > > ah, subtlety here - since we are now tracking both prerenders and non > > prerenders, we can no longer stop observing when started_in_foreground is false, > > as that's going to be the case for prerenders. > > Done. > > https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:77: delta -= info.first_foreground_time.value(); > On 2017/03/24 16:12:04, Bryan McQuade wrote: > > i find the logic here a little confusing. let's keep the true and perceived FCP > > values separate. let's rename what you have as 'delta' to 'fcp' at the top of > > the function, and then add this: > > > > if (info.first_foreground_time) { > > base::TimeDelta perceived_fcp = std::max(base::TimeDelta(), fcp - > > info.first_foreground_time.value()); > > PAGE_LOAD_HISTOGRAM(kPrerenderSearchNavigationToFirstForeground, > > info.first_foreground_time.value()); > > PAGE_LOAD_HISTOGRAM(kPrerenderSearchFirstContentfulPaint, perceived_fcp); > > } > > Done. > > https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:93: if (ui::PageTransitionCoreTypeIs(transition_type_, > On 2017/03/24 16:12:04, Bryan McQuade wrote: > > the remaining logic should be wrapped in if (started_in_foreground) > > > > perhaps the flow could be: > > > > if (started_in_foreground) { > > // log the regular foreground metrics > > } else if (is_prerender_ && info.first_foreground_time) { > > // log prerender metrics > > } > > Done. > > https://codereview.chromium.org/2744983002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:102: void OmniboxSuggestionUsedMetricsObserver::OnFirstMeaningfulPaint( > On 2017/03/24 16:12:04, Bryan McQuade wrote: > > let's make the same changes here > > Done. Good question. The paint subsystem / rendering pipeline is disabled while a page is not in the foreground, so a page may be ready to paint while in the background, but won't actually paint until foregrounded. So, we decided it's not very interesting to measure first paint times for pages that have been backgrounded, since we're really measuring time to foreground after being ready to paint.
this looks really good, thanks! just a few small changes & i think we are good to land it. https://codereview.chromium.org/2744983002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:49: return CONTINUE_OBSERVING; this is the default implementation so you no longer have to implement OnStart https://codereview.chromium.org/2744983002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:81: } else if (is_prerender_) { even though pages are not supposed to paint while in the background, it's probably good for us to also check that first_foreground_time is set in this clause, just to be safe.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, ptal. https://codereview.chromium.org/2744983002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:49: return CONTINUE_OBSERVING; On 2017/03/24 22:53:04, Bryan McQuade wrote: > this is the default implementation so you no longer have to implement OnStart Done. https://codereview.chromium.org/2744983002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:81: } else if (is_prerender_) { On 2017/03/24 22:53:04, Bryan McQuade wrote: > even though pages are not supposed to paint while in the background, it's > probably good for us to also check that first_foreground_time is set in this > clause, just to be safe. Done.
bmcquade@chromium.org changed reviewers: + mpearson@chromium.orgtha - mpearson@chromium.org
thanks! looks like we need to fix a few histogram names, but code structure looks good now. https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:14: "Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint"; the non prerender histograms should actually be NavigationToFirstContentfulPaint https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:16: "Omnibox.SuggestionUsed.URL.ForegroundToFirstContentfulPaint"; same https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:23: "Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint"; these strings need to be updated to FMP https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:25: "Omnibox.SuggestionUsed.URL.ForegroundToFirstContentfulPaint"; FMP https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:27: "Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint.Prerender"; same https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:29: "Omnibox.SuggestionUsed.URL.ForegroundToFirstContentfulPaint.Prerender"; same https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44499: + name="Omnibox.SuggestionUsed.Search.Experimental.ForegroundToFirstMeaningfulPaint" navigationtofirst... https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44518: + name="Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint" navigation https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44556: + name="Omnibox.SuggestionUsed.URL.Experimental.ForegroundToFirstMeaningfulPaint" should be NavigationToFirst... https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44574: +<histogram name="Omnibox.SuggestionUsed.URL.ForegroundToFirstContentfulPaint" navigation
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:14: "Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint"; On 2017/03/24 23:13:00, Bryan McQuade wrote: > the non prerender histograms should actually be NavigationToFirstContentfulPaint Done. https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:16: "Omnibox.SuggestionUsed.URL.ForegroundToFirstContentfulPaint"; On 2017/03/24 23:13:00, Bryan McQuade wrote: > same Done. https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:23: "Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint"; On 2017/03/24 23:13:01, Bryan McQuade wrote: > these strings need to be updated to FMP Done. https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:25: "Omnibox.SuggestionUsed.URL.ForegroundToFirstContentfulPaint"; On 2017/03/24 23:13:00, Bryan McQuade wrote: > FMP Done. https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:27: "Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint.Prerender"; On 2017/03/24 23:13:01, Bryan McQuade wrote: > same Done. https://codereview.chromium.org/2744983002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:29: "Omnibox.SuggestionUsed.URL.ForegroundToFirstContentfulPaint.Prerender"; On 2017/03/24 23:13:00, Bryan McQuade wrote: > same Done. https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44499: + name="Omnibox.SuggestionUsed.Search.Experimental.ForegroundToFirstMeaningfulPaint" On 2017/03/24 23:13:01, Bryan McQuade wrote: > navigationtofirst... Done. https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44518: + name="Omnibox.SuggestionUsed.Search.ForegroundToFirstContentfulPaint" On 2017/03/24 23:13:01, Bryan McQuade wrote: > navigation Done. https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44556: + name="Omnibox.SuggestionUsed.URL.Experimental.ForegroundToFirstMeaningfulPaint" On 2017/03/24 23:13:01, Bryan McQuade wrote: > should be NavigationToFirst... Done. https://codereview.chromium.org/2744983002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44574: +<histogram name="Omnibox.SuggestionUsed.URL.ForegroundToFirstContentfulPaint" On 2017/03/24 23:13:01, Bryan McQuade wrote: > navigation Done.
LGTM once these last 2 comments are addressed, thanks! let's keep an eye on these metrics for the next few days & make sure they look sane on canary. https://codereview.chromium.org/2744983002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:23: "Omnibox.SuggestionUsed.Search.NavigationToFirstMeaningfulPaint"; i like working 'Experimental' into these to reflect that this metric is still under development. You have 'Experimental' in the histograms.xml versions. let's update these to match. https://codereview.chromium.org/2744983002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:103: } else if (is_prerender_) { let's update this to also check that first_foreground_time is set
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I think I somehow overwrote the changes I made to correct fmp histogram names while copying and pasting from fcp. :( https://codereview.chromium.org/2744983002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:23: "Omnibox.SuggestionUsed.Search.NavigationToFirstMeaningfulPaint"; On 2017/03/24 23:37:57, Bryan McQuade wrote: > i like working 'Experimental' into these to reflect that this metric is still > under development. You have 'Experimental' in the histograms.xml versions. let's > update these to match. Done. https://codereview.chromium.org/2744983002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:103: } else if (is_prerender_) { On 2017/03/24 23:37:57, Bryan McQuade wrote: > let's update this to also check that first_foreground_time is set Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/2744983002/#ps180001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lpy@chromium.org changed reviewers: + jochen@chromium.org, mpearson@chromium.org - mpearson@chromium.orgtha
+ jochen@ for histograms.xml owner review.
bmcquade@chromium.org changed reviewers: - jochen@chromium.org
actually mpearson is a histograms.xml owner so can review. mark, can you take a final review to see if this looks sane? we ended up not hooking into the omnibox directly, but instead used the time that a prerender becomes visible as its start time.
Mark@, gentle ping for review.
Sorry for the delay. Yes, everything looks sane. I have some comments, most of which can be answered with better comments, and some suggestions on histograms.xml too. --mark https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:37: "Omnibox.SuggestionUsed.URL.NavigationToFirstForeground.Prerender"; Is it expected that search and URL navigations to first foreground for non-prerender is 0, or at least at least really tiny and cannot be measured using this framework? https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: } else if (is_prerender_ && info.first_foreground_time) { Are all prerenders started in the background, even if the things that spawned them is in the foreground? Please comment. ditto below https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: } else if (is_prerender_ && info.first_foreground_time) { Why do you need to condition on info.first_foreground_time being there? Under what conditions is it missing? (The documentation in the .h for the structure does not say.) If this question causes you to add comments here, then add them below as well. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:78: std::max(base::TimeDelta(), fcp - info.first_foreground_time.value()); optional: This only makes sense to me if first_contentful_paint and first_foreground_time (both timedeltas) are both relative to the same baseline. E.g., time navigation started. chrome/common/page_load_metrics/page_load_timing.h doesn't say anything about the baseline for first_* values. (The info structure chrome/browser/page_load_metrics/page_load_metrics_observer.h does talk about baselines.) The comments in the timing structure kind of imply that the that all the first_ fields and the ones near it are all relative to the same baseline and all the parse_* fields are relative to a different baseline. It might help (outside this changelist), if my interpretation is correct, to make the comments in that file more explicit. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:82: PAGE_LOAD_HISTOGRAM(kPrerenderSearchNavigationToFirstForeground, Is it safe to assume (as this code does) that all pages that are foregrounded observe a first contentful paint? After all, you're only emitting the first foreground time on pages that do. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h (right): https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h:15: explicit OmniboxSuggestionUsedMetricsObserver(bool is_prerender); Please comment. It took me a lot of code browsing to convince myself that this is whether a particular page load came from prerender. My first glance at the call to the constructor for this class, which uses IsPrerendering(), made me think it's a question of whether the browser can be prerendering, not whether this particular page load is. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44502: + Metrics of the time from page first showing up at foreground to first minor rephrasings through out, let me know what you think: Measures the time from the page first appearing in the foreground to its first meaningful paint. Only recorded on navigations that used a prerender that was to a search query suggestion selected from the omnibox. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44512: + Metrics of the time from navigation to first meaningful paint from pages perhaps (if this correct? I don't understand the original description means by saying "from navigation") Measures the time from the user selecting a suggestion in the omnibox to the resulting page's first meaningful paint. Only recorded for search query suggestion selected from the omnibox. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44521: + Metrics of the time from page first showing up at foreground to first Please use the top comment above (possibly with your revisions), then making the analogous description here. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44531: + Metrics of the time from navigation to first contentful paint from pages Please use the second comment above (possibly with your revisions), then making the analogous description here. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44540: + Metrics of the time from page being loaded in prerender to page first Again, minor edits suggested: Measures the time from a page being loaded in prerender to it first showing up at foreground. Only recorded on navigations that used a prerender that was to a search query suggestion selected from the omnibox. (second sentence is the same as suggested in the first description above) https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44555: +<histogram Below: analogous descriptions above, just with URL (capitalized) instead of "search query"
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Thanks for the feedback, I made some update. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:37: "Omnibox.SuggestionUsed.URL.NavigationToFirstForeground.Prerender"; On 2017/03/29 23:17:07, Mark P wrote: > Is it expected that search and URL navigations to first foreground for > non-prerender is 0, or at least at least really tiny and cannot be measured > using this framework? If I understand correctly, non-prerender means we don't have is_prerender is false, we don't log to UMA. And I understand correctly, yes, it should be 0 because it's on foreground when navigation starts. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: } else if (is_prerender_ && info.first_foreground_time) { On 2017/03/29 23:17:07, Mark P wrote: > Why do you need to condition on info.first_foreground_time being there? Under > what conditions is it missing? > (The documentation in the .h for the structure does not say.) > If this question causes you to add comments here, then add them below as well. If I understand correctly, since pages are not supposed to paint while in the background, when `OnFirstContentfulPaint` gets called, it means pages are on foreground, thus first_foreground_time should exist, the reason we add this, quote from Bryan: > even though pages are not supposed to paint while in the background, it's > probably good for us to also check that first_foreground_time is set in this > clause, just to be safe. I add a comment to explain. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: } else if (is_prerender_ && info.first_foreground_time) { On 2017/03/29 23:17:07, Mark P wrote: > Are all prerenders started in the background, even if the things that spawned > them is in the foreground? > Please comment. > ditto below Bryan@, any idea about this question? https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:78: std::max(base::TimeDelta(), fcp - info.first_foreground_time.value()); On 2017/03/29 23:17:07, Mark P wrote: > optional: > This only makes sense to me if first_contentful_paint and first_foreground_time > (both timedeltas) are both relative to the same baseline. E.g., time navigation > started. > chrome/common/page_load_metrics/page_load_timing.h doesn't say anything about > the baseline for first_* values. (The info structure > chrome/browser/page_load_metrics/page_load_metrics_observer.h does talk about > baselines.) The comments in the timing structure kind of imply that the that > all the first_ fields and the ones near it are all relative to the same baseline > and all the parse_* fields are relative to a different baseline. It might help > (outside this changelist), if my interpretation is correct, to make the comments > in that file more explicit. Yes, they are both based on navigation start. For first_foreground_time, please see: https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... I will add some comments to the file in a separate patch. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:82: PAGE_LOAD_HISTOGRAM(kPrerenderSearchNavigationToFirstForeground, On 2017/03/29 23:17:07, Mark P wrote: > Is it safe to assume (as this code does) that all pages that are foregrounded > observe a first contentful paint? After all, you're only emitting the first > foreground time on pages that do. If I understand correctly, if we don't observe a first contentful pain, we don't even trigger this function. https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h (right): https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h:15: explicit OmniboxSuggestionUsedMetricsObserver(bool is_prerender); On 2017/03/29 23:17:07, Mark P wrote: > Please comment. It took me a lot of code browsing to convince myself that this > is whether a particular page load came from prerender. My first glance at the > call to the constructor for this class, which uses IsPrerendering(), made me > think it's a question of whether the browser can be prerendering, not whether > this particular page load is. Done. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44502: + Metrics of the time from page first showing up at foreground to first On 2017/03/29 23:17:07, Mark P wrote: > minor rephrasings through out, let me know what you think: > Measures the time from the page first appearing in the foreground to its first > meaningful paint. Only recorded on navigations that used a prerender that was to > a search query suggestion selected from the omnibox. Done. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44512: + Metrics of the time from navigation to first meaningful paint from pages On 2017/03/29 23:17:07, Mark P wrote: > perhaps (if this correct? I don't understand the original description means by > saying "from navigation") > Measures the time from the user selecting a suggestion in the omnibox to the > resulting page's first meaningful paint. Only recorded for search query > suggestion selected from the omnibox. It means from navigation starts. I rephrase it a little bit. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44521: + Metrics of the time from page first showing up at foreground to first On 2017/03/29 23:17:07, Mark P wrote: > Please use the top comment above (possibly with your revisions), then making the > analogous description here. Done. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44531: + Metrics of the time from navigation to first contentful paint from pages On 2017/03/29 23:17:07, Mark P wrote: > Please use the second comment above (possibly with your revisions), then making > the analogous description here. Done. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44540: + Metrics of the time from page being loaded in prerender to page first On 2017/03/29 23:17:07, Mark P wrote: > Again, minor edits suggested: > Measures the time from a page being loaded in prerender to it first showing up > at foreground. Only recorded on navigations that used a prerender that was to a > search query suggestion selected from the omnibox. > (second sentence is the same as suggested in the first description above) Done. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44555: +<histogram On 2017/03/29 23:17:07, Mark P wrote: > Below: > analogous descriptions above, just with URL (capitalized) instead of "search > query" Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:37: "Omnibox.SuggestionUsed.URL.NavigationToFirstForeground.Prerender"; On 2017/03/31 at 20:42:03, lpy wrote: > On 2017/03/29 23:17:07, Mark P wrote: > > Is it expected that search and URL navigations to first foreground for > > non-prerender is 0, or at least at least really tiny and cannot be measured > > using this framework? > > If I understand correctly, non-prerender means we don't have is_prerender is false, we don't log to UMA. > > And I understand correctly, yes, it should be 0 because it's on foreground when navigation starts. Navigation time is the time we start the prerender (as opposed to the time the user first sees the tab), whereas 'first foreground' would be the time that the prerender is swapped in and used. So this is essentially the amount of time we saved the user in predicting that a result might be needed. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: } else if (is_prerender_ && info.first_foreground_time) { On 2017/03/31 at 20:42:03, lpy wrote: > On 2017/03/29 23:17:07, Mark P wrote: > > Are all prerenders started in the background, even if the things that spawned > > them is in the foreground? > > Please comment. > > ditto below > > Bryan@, any idea about this question? Yes, this is my understanding - the WebContents hosting a prerender is 'hidden' until the prerender is used. We used to have a DCHECK to verify this, but it got lost in a refactor. You could ask pasko@ if you want to be certain of this. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:82: PAGE_LOAD_HISTOGRAM(kPrerenderSearchNavigationToFirstForeground, On 2017/03/31 at 20:42:03, lpy wrote: > On 2017/03/29 23:17:07, Mark P wrote: > > Is it safe to assume (as this code does) that all pages that are foregrounded > > observe a first contentful paint? After all, you're only emitting the first > > foreground time on pages that do. > > If I understand correctly, if we don't observe a first contentful pain, we don't even trigger this function. > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... Not all pages that are foregrounded will observe a contentful paint. For example a navigation to a completely blank page will never encounter an FCP. These cases are rare and it's not clear that it's interesting to track them so we ignore those pages. The last event in the pipeline that's guaranteed to happen for all pages is the first layout, so if we do want a metric that we know gets logged for all pages, even empty ones, we could hook OnFirstLayout. However, first layout is not as well connected to the user experience as painting, which is why we have tended to focus more on the paint metrics despite the fact that some pages may never report contentful paints (e.g. blank pages, or pages with no text, images, svg content, etc). https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44512: + Metrics of the time from navigation to first meaningful paint from pages On 2017/03/31 at 20:42:03, lpy wrote: > On 2017/03/29 23:17:07, Mark P wrote: > > perhaps (if this correct? I don't understand the original description means by > > saying "from navigation") > > Measures the time from the user selecting a suggestion in the omnibox to the > > resulting page's first meaningful paint. Only recorded for search query > > suggestion selected from the omnibox. > > It means from navigation starts. I rephrase it a little bit. Right, for better or for worse, page load metrics (both chrome's page load metrics infra, as well as public metrics APIs exposed to web developers) are baselined at 'navigation start' which is more or less the time that the navigation was initiated. In the case of prerender, this can be long before the user perceived time when they initiated the navigation. Since so much of the existing page load metrics infra baselines at navigation_start, we continue to use 'NavigationTo...' as the baseline except for cases where a prerender is involved, since in those cases, the user perceived delay starts at first foreground. Hope that helps to give a little more explanation for the subtleties in baseline times between these histograms.
lgtm modulo minor changes requested below --mark https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:37: "Omnibox.SuggestionUsed.URL.NavigationToFirstForeground.Prerender"; On 2017/04/03 19:34:34, Bryan McQuade wrote: > On 2017/03/31 at 20:42:03, lpy wrote: > > On 2017/03/29 23:17:07, Mark P wrote: > > > Is it expected that search and URL navigations to first foreground for > > > non-prerender is 0, or at least at least really tiny and cannot be measured > > > using this framework? > > > > If I understand correctly, non-prerender means we don't have is_prerender is > false, we don't log to UMA. > > > > And I understand correctly, yes, it should be 0 because it's on foreground > when navigation starts. > > Navigation time is the time we start the prerender (as opposed to the time the > user first sees the tab), whereas 'first foreground' would be the time that the > prerender is swapped in and used. So this is essentially the amount of time we > saved the user in predicting that a result might be needed. Acknowledged. https://codereview.chromium.org/2744983002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:82: PAGE_LOAD_HISTOGRAM(kPrerenderSearchNavigationToFirstForeground, On 2017/04/03 19:34:34, Bryan McQuade wrote: > On 2017/03/31 at 20:42:03, lpy wrote: > > On 2017/03/29 23:17:07, Mark P wrote: > > > Is it safe to assume (as this code does) that all pages that are > foregrounded > > > observe a first contentful paint? After all, you're only emitting the first > > > foreground time on pages that do. > > > > If I understand correctly, if we don't observe a first contentful pain, we > don't even trigger this function. > > > > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... > > Not all pages that are foregrounded will observe a contentful paint. For example > a navigation to a completely blank page will never encounter an FCP. These cases > are rare and it's not clear that it's interesting to track them so we ignore > those pages. The last event in the pipeline that's guaranteed to happen for all > pages is the first layout, so if we do want a metric that we know gets logged > for all pages, even empty ones, we could hook OnFirstLayout. However, first > layout is not as well connected to the user experience as painting, which is why > we have tended to focus more on the paint metrics despite the fact that some > pages may never report contentful paints (e.g. blank pages, or pages with no > text, images, svg content, etc). Thanks for the explanation. I don't think new comments are needed here, but it does influence the histograms.xml comments. I'll add a note there. https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44512: + Metrics of the time from navigation to first meaningful paint from pages On 2017/04/03 19:34:34, Bryan McQuade wrote: > On 2017/03/31 at 20:42:03, lpy wrote: > > On 2017/03/29 23:17:07, Mark P wrote: > > > perhaps (if this correct? I don't understand the original description means > by > > > saying "from navigation") > > > Measures the time from the user selecting a suggestion in the omnibox to the > > > resulting page's first meaningful paint. Only recorded for search query > > > suggestion selected from the omnibox. > > > > It means from navigation starts. I rephrase it a little bit. > > Right, for better or for worse, page load metrics (both chrome's page load > metrics infra, as well as public metrics APIs exposed to web developers) are > baselined at 'navigation start' which is more or less the time that the > navigation was initiated. In the case of prerender, this can be long before the > user perceived time when they initiated the navigation. Since so much of the > existing page load metrics infra baselines at navigation_start, we continue to > use 'NavigationTo...' as the baseline except for cases where a prerender is > involved, since in those cases, the user perceived delay starts at first > foreground. Hope that helps to give a little more explanation for the subtleties > in baseline times between these histograms. Thanks for the explanation. Now I know to mainly look at ForegroundTo for my purposes. https://codereview.chromium.org/2744983002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: // Since a page is not supposed to paint in the background, nit: it's odd to have this comment here and indented as such. Perhaps at line 76, instead do return; } // comment here if (is_prerender_ && info.first_foreground_time) { ... This also reduced indenting below. Also, minor nit: I think there's supposed to be a period after "be set", and then "we" in the next part should be capitalized as a new sentence. https://codereview.chromium.org/2744983002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44762: + that was to a search query suggestion selected from the omnibox. Can you please add that this is only recorded on pages that experience a first contentful paint? ditto on other ToFirstForeground metric.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the feedback. https://codereview.chromium.org/2744983002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2744983002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc:76: // Since a page is not supposed to paint in the background, On 2017/04/03 23:19:15, Mark P wrote: > nit: it's odd to have this comment here and indented as such. > > Perhaps at line 76, instead do > return; > } > // comment here > if (is_prerender_ && info.first_foreground_time) { > ... > > This also reduced indenting below. > > Also, minor nit: I think there's supposed to be a period after "be set", and > then "we" in the next part should be capitalized as a new sentence. Done. https://codereview.chromium.org/2744983002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744983002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44762: + that was to a search query suggestion selected from the omnibox. On 2017/04/03 23:19:15, Mark P wrote: > Can you please add that this is only recorded on pages that experience a first > contentful paint? > ditto on other ToFirstForeground metric. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2744983002/#ps220001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1491276193248170,
"parent_rev": "e2f3b5f19932e3726ee9d3ff19419587f067fd88", "commit_rev":
"71bcf50b0c34114c514e431af16527792c55006a"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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} Committed: https://chromium.googlesource.com/chromium/src/+/71bcf50b0c34114c514e431af165... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/71bcf50b0c34114c514e431af165...
Message was sent while issue was closed.
I just noticed that we didn't specify histogram owners for any of the new histograms in histograms.xml, which means none of these new histograms show up in the UMA dash by default. Mark, who is the right owner for these? Is that you? On Mon, Apr 3, 2017 at 11:30 PM commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Committed patchset #12 (id:220001) as > > https://chromium.googlesource.com/chromium/src/+/71bcf50b0c34114c514e431af165... > > https://codereview.chromium.org/2744983002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
As I answered on the code review, I'm happy being an owner of these. I feel like I understand them well at this point. --mark On Wed, Apr 5, 2017 at 7:48 PM, Bryan McQuade <bmcquade@chromium.org> wrote: > I just noticed that we didn't specify histogram owners for any of the new > histograms in histograms.xml, which means none of these new histograms show > up in the UMA dash by default. Mark, who is the right owner for these? Is > that you? > > On Mon, Apr 3, 2017 at 11:30 PM commit-bot@chromium.org via > codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> > wrote: > >> Committed patchset #12 (id:220001) as >> https://chromium.googlesource.com/chromium/src/+/ >> 71bcf50b0c34114c514e431af16527792c55006a >> >> https://codereview.chromium.org/2744983002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
