Record startup FCP/LCP via a WebContentsObserver LCP callback [chromium/src : main]
Chris Davis (Gerrit)
Chris Davis added 4 comments![]( "Open in Gerrit")
void DidFirstVisuallyNonEmptyPaint() override;Annie SullivanAll of this makes me wonder why `WebContentsObserver` doesn't just emit FCP and LCP?
It seems it already has FCP with `WebContentsObserver::OnFirstContentfulPaintInPrimaryMainFrame()`, so it's just missing LCP. I think it makes sense to add it => @sull...@chromium.org?
Gabriel CharetteIt definitely makes sense to add FCP.
For LCP, there are a series of candidates as larger and larger elements paint. The algorithm terminates at first input or scroll, but if the user doesn't interact you need to wait until the page unloads to get the value. I'm not sure how that would work here?
Chris DavisAs long as the retained candidate happened before the FirstWebContents was deprioritized (backgrounded/etc.), it should be fine?
Done
void RecordFirstWebContentsFirstContentfulPaint(int64_t navigation_id,Chris DavisI'd say we should rename this since it's called by multiple navigations and not just the first.
How about: OnFCPForPotentialFirstWebContents() ? And then we allow it being called multiple times, documenting that it only proceeds with the first match to the FirstWebContents, as defined by RecordFirstWebContentsMainNavigationStart().
But I'm also not a fan of this always-on API for something we need to record once, hence my above meta comment about adding support to the WebContentsObserver.
Done
// paint. |navigation_id| must match the navigation registered byChris DavisDoes it? I was under the impression this would take calls from each LCP until the one that matches the FirstWebContents? i.e. if another web contents finishes its LCP first, we still want to record the FirstWebContents' LCP, assuming it remained foreground the entire time (e.g. two foreground window scenario)
Done
class StartupMetricUtilsTest : public testing::Test {Chris DavisThis makes me realize, we already have chrome/browser/metrics/startup_metrics_browsertest.cc (and I forgot to mention in the previous CL). I think we should just add the new metrics to that one? I like an integration test here to confirm that it's not just that calling the methods works but that everything is hooked up in order to get there in real startups.
Done
Related details
- Annie Sullivan
- Gabriel Charette
- Ramin Halavati
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Gabriel Charette added 3 comments![]( "Open in Gerrit")
Quick pass as I won't have time to review this much code today.
I'm surprised this needs to be that large? Can we make it less verbose? Does it need to plumb through so many files?
g_startup_temperature = UNDETERMINED_STARTUP_TEMPERATURE;seems okay, but why is this related?
constexpr char kFirstRunSentinelHistogram[] = "FirstRun.Sentinel.Created";Let's track the new metrics in chrome/browser/metrics/startup_metrics_browsertest.cc.
With that, I don't feel we need this test framework, it's doing very little (confirms that calling the thing does the thing...). What we really care about is whether the full integration registers the metrics.
Related details
- Annie Sullivan
- Chris Davis
- Ramin Halavati
- Shunya Shishido
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. |
Fergal Daly (Gerrit)
Fergal Daly added 1 comment![]( "Open in Gerrit")
What's the status of this change. I would really like to be able to use this metric but the current issues make it problematic.
Related details
- Annie Sullivan
- Chris Davis
- Ramin Halavati
- Shunya Shishido
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. |
Chris Davis (Gerrit)
Chris Davis added 3 comments![]( "Open in Gerrit")
What's the status of this change. I would really like to be able to use this metric but the current issues make it problematic.
I'm currently out of office until 7/6 and will work more on this then
g_startup_temperature = UNDETERMINED_STARTUP_TEMPERATURE;seems okay, but why is this related?
This was needed to unblock unit tests issues. With the move away from the additional unit tests I add to startup_metrics_browsertest this is no longer needed.
constexpr char kFirstRunSentinelHistogram[] = "FirstRun.Sentinel.Created";Let's track the new metrics in chrome/browser/metrics/startup_metrics_browsertest.cc.
With that, I don't feel we need this test framework, it's doing very little (confirms that calling the thing does the thing...). What we really care about is whether the full integration registers the metrics.
Ok, let me create new unit tests there. I think the existing tests nav to about:blank so I will need to add a data url hello world for the FCP/LCP tests.
Related details
- Annie Sullivan
- Fergal Daly
- Gabriel Charette
- Ramin Halavati
- Shunya Shishido
Ramin Halavati (Gerrit)
Ramin Halavati added 1 comment![]( "Open in Gerrit")
I don't own any files in this change, please add me back to reviewers if I can help.
Related details
- Annie Sullivan
- Chris Davis
- Fergal Daly
- Shunya Shishido
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. |
Chris Davis (Gerrit)
Chris Davis added 2 comments![]( "Open in Gerrit")
g_startup_temperature = UNDETERMINED_STARTUP_TEMPERATURE;Chris Davisseems okay, but why is this related?
This was needed to unblock unit tests issues. With the move away from the additional unit tests I add to startup_metrics_browsertest this is no longer needed.
Done
constexpr char kFirstRunSentinelHistogram[] = "FirstRun.Sentinel.Created";Chris DavisLet's track the new metrics in chrome/browser/metrics/startup_metrics_browsertest.cc.
With that, I don't feel we need this test framework, it's doing very little (confirms that calling the thing does the thing...). What we really care about is whether the full integration registers the metrics.
Ok, let me create new unit tests there. I think the existing tests nav to about:blank so I will need to add a data url hello world for the FCP/LCP tests.
- Annie Sullivan
- Fergal Daly
- Gabriel Charette
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Gabriel Charette added 1 comment![]( "Open in Gerrit")
bool finish_reason_recorded_ = false;Do we still track page loads that were abandoned/deprioritized between first-paint and FCP/LCP?
Related details
- Annie Sullivan
- Chris Davis
- Fergal Daly
- Shunya Shishido
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. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
Do we still track page loads that were abandoned/deprioritized between first-paint and FCP/LCP?
No — once we reach first-non-empty-paint we commit to kDone and only keep observing the same WebContents for FCP/LCP. If it's abandoned/deprioritized in that window (new nav, hidden, or destroyed) we stop observing immediately and record only the FCP/LCP that already arrived; the finish reason stays kDone (unchanged from before this CL). Added a comment documenting this.
Related details
- Annie Sullivan
- Fergal Daly
- Gabriel Charette
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |