[OmniboxPopup] Add startup FP/FCP measurement [chromium/src : main]
Ming-Ying Chung (Gerrit)
Message from Ming-Ying Chung![]( "Open in Gerrit")
Set Ready For Review
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Ming-Ying Chung
- Paul Adedeji
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Adedeji (Gerrit)
Paul Adedeji added 3 comments![]( "Open in Gerrit")
omnibox* changes lgtm % nit + optional
base::TimeDelta delta =
presentation_timestamp - popup_create_start_time;
base::UmaHistogramTimes("Omnibox.Views.PopupFirstPaint", delta);
// Record identically named metrics to allow comparison with WebUI
// version.
TopChromeWebUIMetricsObserver::RecordFirstPaintMetrics(
"OmniboxPopup", delta);nit: Consider removing comment here or rewording e.g. "Record metrics via `TopChromeWebUIMetricsObserver` to allow comparison between WebUI and Views Omnibox versions."
The way this comment is written, makes me expect to see multiple metrics being recorded directly here (as opposed to `TopChromeWebUIMetricsObserver` doing the identical recording).
// Check both legacy and new metrics.optional: Consider replacing with `Views and WebUI metrics` OR `legacy (Views) and new (WebUI) metrics`, to clarify what legacy and new mean.
Related details
- Ming-Ying Chung
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Adedeji (Gerrit)
Paul Adedeji voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ming-Ying Chung
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
// Check both legacy and new metrics.optional: Consider replacing with `Views and WebUI metrics` OR `legacy (Views) and new (WebUI) metrics`, to clarify what legacy and new mean.
Could you also help confirm if `omnibox::kWebUIOmniboxPopup` is the right flag to enable the WebUI version? I tried adding browsertest for it but it seems to be very flaky.
Related details
- Paul Adedeji
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 1 comment![]( "Open in Gerrit")
High level QQ - how does this differ from the non-tab page load metrics observer (which I think also switches on the top-chrome pseudo tld)
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/page_load_metrics_initialize.cc;l=104;drc=af6927a3ff251d63093086932feb8fce181e9b91
Currently it looks like its only enabled for the omnibox but presumably this will grow to cover all top-chrome?
Related details
- Ming-Ying Chung
- Paul Adedeji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Adedeji (Gerrit)
Paul Adedeji added 1 comment![]( "Open in Gerrit")
// Check both legacy and new metrics.Ming-Ying Chungoptional: Consider replacing with `Views and WebUI metrics` OR `legacy (Views) and new (WebUI) metrics`, to clarify what legacy and new mean.
Could you also help confirm if `omnibox::kWebUIOmniboxPopup` is the right flag to enable the WebUI version? I tried adding browsertest for it but it seems to be very flaky.
`omnibox::kWebUIOmniboxPopup` is the correct flag, are the flakes you're referring to the one in the latest failed run - https://chromium-review.googlesource.com/c/7650781?checksPatchset=3&tab=checks?
Related details
- Ming-Ying Chung
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
High level QQ - how does this differ from the non-tab page load metrics observer (which I think also switches on the top-chrome pseudo tld)
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/page_load_metrics_initialize.cc;l=104;drc=af6927a3ff251d63093086932feb8fce181e9b91Currently it looks like its only enabled for the omnibox but presumably this will grow to cover all top-chrome?
1. non-tab page load metrics observer doesn't log FP
2. this new observer attempts to support both webui and native view (via static helper).
Other than the above, this observer does duplicate the same `GetBackgroundTime()` logic from non-tab page load metrics observer.
Related details
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
Ming-Ying ChungHigh level QQ - how does this differ from the non-tab page load metrics observer (which I think also switches on the top-chrome pseudo tld)
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/page_load_metrics_initialize.cc;l=104;drc=af6927a3ff251d63093086932feb8fce181e9b91Currently it looks like its only enabled for the omnibox but presumably this will grow to cover all top-chrome?
1. non-tab page load metrics observer doesn't log FP
2. this new observer attempts to support both webui and native view (via static helper).Other than the above, this observer does duplicate the same `GetBackgroundTime()` logic from non-tab page load metrics observer.
Currently it looks like its only enabled for the omnibox but presumably this will grow to cover all top-chrome?
Yes that's the plan.
I have also considered reusing `non_tab_webui_page_load_metrics_observer.cc` but the metrics it logs have `webui` in its name.
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tom Lukaszewicz
| Code-Review | +1 |
2 comments
lgtm
class TopChromeWebUIMetricsObserverCould we add additional detail to the comment here on how this differs from the NonTabWebUIMetricsObserver
Related details
- Ming-Ying Chung
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 3 comments![]( "Open in Gerrit")
Could we add additional detail to the comment here on how this differs from the NonTabWebUIMetricsObserver
Done
base::TimeDelta delta =
presentation_timestamp - popup_create_start_time;
base::UmaHistogramTimes("Omnibox.Views.PopupFirstPaint", delta);
// Record identically named metrics to allow comparison with WebUI
// version.
TopChromeWebUIMetricsObserver::RecordFirstPaintMetrics(
"OmniboxPopup", delta);nit: Consider removing comment here or rewording e.g. "Record metrics via `TopChromeWebUIMetricsObserver` to allow comparison between WebUI and Views Omnibox versions."
The way this comment is written, makes me expect to see multiple metrics being recorded directly here (as opposed to `TopChromeWebUIMetricsObserver` doing the identical recording).
Done
Ming-Ying Chungoptional: Consider replacing with `Views and WebUI metrics` OR `legacy (Views) and new (WebUI) metrics`, to clarify what legacy and new mean.
Paul AdedejiCould you also help confirm if `omnibox::kWebUIOmniboxPopup` is the right flag to enable the WebUI version? I tried adding browsertest for it but it seems to be very flaky.
`omnibox::kWebUIOmniboxPopup` is the correct flag, are the flakes you're referring to the one in the latest failed run - https://chromium-review.googlesource.com/c/7650781?checksPatchset=3&tab=checks?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Message from Ming-Ying Chung![]( "Open in Gerrit")
- Ming-Ying Chung
- Paul Adedeji
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
void TopChromeWebUIMetricsObserver::OnFirstPaintInPage(
const page_load_metrics::mojom::PageLoadTiming& timing) {
CHECK(timing.paint_timing->first_paint.has_value());
// Time from navigation to FP. They can be very large for preloaded WebUIs
// because the FP is not recorded until the WebUI is actually shown.
base::TimeDelta first_paint = timing.paint_timing->first_paint.value();
PAGE_LOAD_HISTOGRAM(GetMetricName(webui_name_, "NavigationToFirstPaint"),
first_paint);
// Time from request to FP. This metric disregards time spent in the
// background, which is non-zero when the WebUI is preloaded.
base::TimeDelta background_time = GetBackgroundTime(GetDelegate());
PAGE_LOAD_SHORT_HISTOGRAM(GetMetricName(webui_name_, "RequestToFirstPaint"),
first_paint - background_time);
}@paula...@google.com I am struggling to figure out why FirstPaint is triggered for webui omnibox popup but not FirstContentfulPaint. Maybe your knowledge can help:
- Is WebUI OmniboxPopup preloaded in background?
- If so,
- is it set to visible only when user interacts with the search bar?
- any of the preloaded UI being displayed before user interaction?
Related details
- Paul Adedeji
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |