[soft navs] Pure refactor - extract SoftNavigationMetricsObserver [chromium/src : main]
Johannes Henkel (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Annie Sullivan
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Annie Sullivan (Gerrit)
Annie Sullivan added 2 comments![]( "Open in Gerrit")
CalculateLCPEntropyBucket is now duplicated, may need to move this somewhere else to make this cl nice.Maybe `ContentfulPaintTimingInfo`?
}One thing I'm not clear on is the lifetime of this observer and how it relates to BackForwardCachePageLoadMetricsObserver registered in RegisterCommonObservers above, and PrefetchPageLoadMetricsObserver above.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 3 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +1 |
3 comments
CalculateLCPEntropyBucket is now duplicated, may need to move this somewhere else to make this cl nice.Johannes HenkelMaybe `ContentfulPaintTimingInfo`?
Done
One thing I'm not clear on is the lifetime of this observer and how it relates to BackForwardCachePageLoadMetricsObserver registered in RegisterCommonObservers above, and PrefetchPageLoadMetricsObserver above.
Lifetime: It appears the observers are owned by the PageLoadTracker (in the observers_ field there), and they get destroyed when the tracker stops tracking. Which happens in void MetricsWebContentsObserver::DidFinishNavigation, in specific conditions. E.g., if the page is put into the bfcache then it looks that the tracker remains alive, presumably so that it can track some more later.
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/metrics_web_contents_observer.cc;l=654;bpv=1 - For the interesting conditions, I'm looking for the StopTracking calls in MetricsWebContentsObserver::DidFinishNavigation, since StopTracking will do observers_.clear(); and the only other way to delete an observer is when it returns STOP_OBSERVING from one of its methods - this happens in InvokeAndPruneObservers (https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/page_load_tracker.cc;l=833;bpv=1;bpt=1).
I'm sort of thinking that these observers should be pretty light in logic, and just be about sending stuff to ukm.
Perhaps one way to organize the code may be to make it so that each of these observers sends metrics for only a single ukm event, e.g., the new observer would only send data for the SoftNavigation UKM event. This is not the case in my changelist currently - it's also sending stuff for the PageLoad UKM event (the INP / CLS before soft navigation). Could change that.
WDYT?
Related details
- Annie Sullivan
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Annie Sullivan (Gerrit)
Annie Sullivan added 1 comment![]( "Open in Gerrit")
Johannes HenkelOne thing I'm not clear on is the lifetime of this observer and how it relates to BackForwardCachePageLoadMetricsObserver registered in RegisterCommonObservers above, and PrefetchPageLoadMetricsObserver above.
Lifetime: It appears the observers are owned by the PageLoadTracker (in the observers_ field there), and they get destroyed when the tracker stops tracking. Which happens in void MetricsWebContentsObserver::DidFinishNavigation, in specific conditions. E.g., if the page is put into the bfcache then it looks that the tracker remains alive, presumably so that it can track some more later.
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/metrics_web_contents_observer.cc;l=654;bpv=1 - For the interesting conditions, I'm looking for the StopTracking calls in MetricsWebContentsObserver::DidFinishNavigation, since StopTracking will do observers_.clear(); and the only other way to delete an observer is when it returns STOP_OBSERVING from one of its methods - this happens in InvokeAndPruneObservers (https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/page_load_tracker.cc;l=833;bpv=1;bpt=1).
I'm sort of thinking that these observers should be pretty light in logic, and just be about sending stuff to ukm.
Perhaps one way to organize the code may be to make it so that each of these observers sends metrics for only a single ukm event, e.g., the new observer would only send data for the SoftNavigation UKM event. This is not the case in my changelist currently - it's also sending stuff for the PageLoad UKM event (the INP / CLS before soft navigation). Could change that.
WDYT?
Yeah, I definitely think having PageLoad before* events in the PageLoad observer, bfcache before* events in the bfcache observer, and SoftNavigation events in this observer is the way to go.
I also think that currently, it's not a lot of duplicated code to write it that way.
But when we look at all the timing issues I documented, fixing them all will require some code, and ideally that code runs at the PageLoadTracker level but maybe lives in its own class. This is why I'm hoping we can sketch out a bit around what the ideal solution looks like so we can iterate towards it.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |