[soft navs] Start separating soft navs and soft lcp updates [chromium/src : main]
Johannes Henkel (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Annie Sullivan
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 5 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
5 comments
Consequently, updating the soft lcp takes place inNit: some formatting changes needed in this commit message
if (new_soft_navigation_metrics.count <= soft_navigation_metrics_->count) {And just confirming-- this only happens today because we still send the WHOLE soft-nav message in order to update the LCP candidate, but we will re-architect the mojo pipe to basically be 1:1 with new updates for either of these?
// Notify the observers - including and in particular, this willNit: comments can span 80 char line width to reduce number of lines and make it a bit easier to read.
(I've found that git cl format will not merge comment lines if they all fit, but if you make one of the lines go beyond 80, then it will re-format perfectly)
// reset the CLS, INP, and LCP calculators, and remember the
// new soft navigation.Nit: "remember the new soft navigation" --> do we only use this in order to remember new_soft_navigation_metrics.count later?
I notice that above `observer->OnSoftNavigationUpdated` passes only the new soft-nav not the old one, so presumably the observers have their own copy?
Anyway, no concerns for this patch, I just wonder if we only need a simple "key" like count that helps us know that soft-nav actually changed vs LCP-update-only, and if all of that goes away after architecture change?
Related details
- Annie Sullivan
- Johannes Henkel
- Scott Haseley
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. |
Scott Haseley (Gerrit)
Scott Haseley added 3 comments![]( "Open in Gerrit")
soft_navigation_contentful_paint_candidate_.Text().Reset(I'm curious why this class doesn't have a `Reset()` method that just clears everything (or `.Clear()`). Might be worth cleaning up at some point...
// Notify the observers - including and in particular, this willNit: comments can span 80 char line width to reduce number of lines and make it a bit easier to read.
(I've found that git cl format will not merge comment lines if they all fit, but if you make one of the lines go beyond 80, then it will re-format perfectly)
Neat trick! I obsessively `gq` in vim.
// Now that the previous soft navigation is digested by the observers,ultra nit: consumed? I'm getting some bad imagery with digested 😄
Related details
- Annie Sullivan
- Johannes Henkel
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. |
Scott Haseley (Gerrit)
Scott Haseley voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
// Now that the previous soft navigation is digested by the observers,ultra nit: consumed? I'm getting some bad imagery with digested 😄
Actually laughed out loud!
Johannes Henkel (Gerrit)
Johannes Henkel added 7 comments![]( "Open in Gerrit")
Thanks a lot. Will try to get this one submitted.
Nit: some formatting changes needed in this commit message
Done
soft_navigation_contentful_paint_candidate_.Text().Reset(I'm curious why this class doesn't have a `Reset()` method that just clears everything (or `.Clear()`). Might be worth cleaning up at some point...
Yeah, good point. I'll attempt a follow-up.
if (new_soft_navigation_metrics.count <= soft_navigation_metrics_->count) {And just confirming-- this only happens today because we still send the WHOLE soft-nav message in order to update the LCP candidate, but we will re-architect the mojo pipe to basically be 1:1 with new updates for either of these?
Yeah.
// Notify the observers - including and in particular, this willNit: comments can span 80 char line width to reduce number of lines and make it a bit easier to read.
(I've found that git cl format will not merge comment lines if they all fit, but if you make one of the lines go beyond 80, then it will re-format perfectly)
Yup, thank you, done!
I'm in Emacs, so it's META-q for me. :-)
// Now that the previous soft navigation is digested by the observers,Michal Mocnyultra nit: consumed? I'm getting some bad imagery with digested 😄
Actually laughed out loud!
consumed it is :-)
// reset the CLS, INP, and LCP calculators, and remember the
// new soft navigation.Nit: "remember the new soft navigation" --> do we only use this in order to remember new_soft_navigation_metrics.count later?
I notice that above `observer->OnSoftNavigationUpdated` passes only the new soft-nav not the old one, so presumably the observers have their own copy?
Anyway, no concerns for this patch, I just wonder if we only need a simple "key" like count that helps us know that soft-nav actually changed vs LCP-update-only, and if all of that goes away after architecture change?
The observers have access to the previous soft navigation via the PageLoadMetricsObserverDelegate interface, which the PageLoadTracker implements (this is documented in the comment I added). So, they don't need to keep their own copy (they can if they really want to).
This is also why at the end of OnSoftNavigationChanged, the PageLoadTracker updates its copy of the soft nav, so that next time the notification is sent to the observers, the delegate knows the previous one.
Related details
- Annie Sullivan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: components/page_load_metrics/browser/page_load_tracker.cc
Insertions: 7, Deletions: 9.
The diff is too large to show. Please review the diff.
```
Change information
[soft navs] Start separating soft navs and soft lcp updates
This is another modest refactoring. In
PageLoadTracker::OnSoftNavigationChanged, we no longer access the lcp
data that's attached to the SoftNavigationMetrics mojom. Instead, we
only process the new SoftNavigation record itself. The new comments
explain how the method works.
Consequently, updating the soft lcp takes place in
PageLoadMetricsUpdateDispatcher::UpdateMetrics, and to keep the logic
the same as before, the soft lcp is always updated just after a new soft
navigation has been recognized. Note that the UpdateSoftNavigation call
in PageLoadMetricsUpdateDispatcher::UpdateMetrics triggers
PageLoadTracker::OnSoftNavigationChanged (which is a no-op if no new
soft navigation arrived).
- M components/page_load_metrics/browser/page_load_metrics_update_dispatcher.cc
- M components/page_load_metrics/browser/page_load_metrics_update_dispatcher.h
- M components/page_load_metrics/browser/page_load_tracker.cc
Code-Review: +1 by Michal Mocny, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |