[soft navs] split soft lcp mojom from soft nav mojom [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 added 6 comments![]( "Open in Gerrit")
Some initial comments but i'll take one more stab.
This is the right direction for sure!
SoftNavigationMetrics mojom message, and instead sends the Soft LCP
separately. I've tried to keep this as mechanical and simple asWe don't have to, but, do you think the same message would be good to use for hard-LCP, or too risky that these have unique mechanics?
Today we have the same problem with hard-LCP sending. We don't have to solve both here, but just design-wise we use the same INP and the same CLS messages for hard and soft... can/should we do that for LCP?
waiter->AddSoftNavigationImageLCPExpectation(1);This is a soft-LCP test and will eventually get the soft-lcp from the page and from ukm it looks like-- so does it not need the waiter to wait?
(I dont undestand how this part works, sorry)
mojom::LargestContentfulPaintTimingPtr,Should this be a vector? I know that today it isnt-- today we overwrite the global value on renderer side before sending, and/or "force flush" to ensure we can send a stream of these.
but if we want to support stream buffering, I think we can just send the full list?
---
Or maybe you want to keep this patch 1:1 with existing and change in the future?
mojom::LargestContentfulPaintTimingPtr(std::in_place),Aside: I know what `std::in_place` does, but man does it suck that this is the idiomatic way to initialize a default value in a wrapper type.
return !timing.count;FWIW, I think that IsEmpty is only needed when we send a structure with each update, so its in like an "empty state", I think.
But, when you have vector<> of items, you can just check if its empty (for LCP this would work), and perhaps there is an optional<> concept for mojom args as well?
(These IsEmpty() checks smell terrible to me somehow)
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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 5 comments![]( "Open in Gerrit")
Thank you!
SoftNavigationMetrics mojom message, and instead sends the Soft LCP
separately. I've tried to keep this as mechanical and simple asWe don't have to, but, do you think the same message would be good to use for hard-LCP, or too risky that these have unique mechanics?
Today we have the same problem with hard-LCP sending. We don't have to solve both here, but just design-wise we use the same INP and the same CLS messages for hard and soft... can/should we do that for LCP?
Not sure I understand 100%, but yes, soft lcp and hard lcp use the same messages.
blink::LargestContentfulPaintDetailsForReporting and mojom::LargestContentfulPaintTiming. This isn't changing. Hard LCP is a field of blink::WebPerformanceMetricsForReporting and mojom::PaintTiming.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_performance_metrics_for_reporting.h;l=30?
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/common/page_load_metrics.mojom;l=36?
waiter->AddSoftNavigationImageLCPExpectation(1);This is a soft-LCP test and will eventually get the soft-lcp from the page and from ukm it looks like-- so does it not need the waiter to wait?
(I dont undestand how this part works, sorry)
Yeah, I removed the SoftNavigation*LCPExpectation stuff, because there isn't a notification on the browser side for LCP like there is for the soft navs. And I think for now, there shouldn't be, because we'll use the pattern to ask the delegate for the LCP. Note that there is also no notification for soft lcp and soft inp.
If I wanted to keep these expectations, I'd have to set things up so that the waiter receives events for soft lcp. It's possible, e.g. by putting stuff into the metrics render frame observer, but this seems quite artificial since the normal page load metrics observers won't be doing that. Not waiting for the soft lcp is ok because the soft nav will only be recognized if there was a paint, and the ukm part of the test will receive that either way.
mojom::LargestContentfulPaintTimingPtr,Should this be a vector? I know that today it isnt-- today we overwrite the global value on renderer side before sending, and/or "force flush" to ensure we can send a stream of these.
but if we want to support stream buffering, I think we can just send the full list?
---
Or maybe you want to keep this patch 1:1 with existing and change in the future?
Yeah, would like to do the switch to vectors in a futurue cl, since there'll be more interesting logic in the browser to digest the vector also.
return !timing.count;FWIW, I think that IsEmpty is only needed when we send a structure with each update, so its in like an "empty state", I think.
But, when you have vector<> of items, you can just check if its empty (for LCP this would work), and perhaps there is an optional<> concept for mojom args as well?
(These IsEmpty() checks smell terrible to me somehow)
Exactly. This will go away, because a vector is a generalization of optional. I didn't want to do the vector change right away though, since if it was a vector I'd have to change the logic in page_load_metrics_update_dispatcher etc., and I don't want to do that now.
Related details
- Annie Sullivan
- Michal Mocny
- 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. |
Michal Mocny (Gerrit)
Michal Mocny added 4 comments![]( "Open in Gerrit")
SoftNavigationMetrics mojom message, and instead sends the Soft LCP
separately. I've tried to keep this as mechanical and simple asJohannes HenkelWe don't have to, but, do you think the same message would be good to use for hard-LCP, or too risky that these have unique mechanics?
Today we have the same problem with hard-LCP sending. We don't have to solve both here, but just design-wise we use the same INP and the same CLS messages for hard and soft... can/should we do that for LCP?
Not sure I understand 100%, but yes, soft lcp and hard lcp use the same messages.
blink::LargestContentfulPaintDetailsForReporting and mojom::LargestContentfulPaintTiming. This isn't changing. Hard LCP is a field of blink::WebPerformanceMetricsForReporting and mojom::PaintTiming.https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_performance_metrics_for_reporting.h;l=30?
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/common/page_load_metrics.mojom;l=36?
We have `page_load_timing.paint_timing.largest_contentful_paint` and then we have a distinct `soft_navigation_metrics.largest_contentful_paint`, each as separate "singletons" with each `UpdateTiming` call.
Now you are splitting out the LCP message as its own arg, but I think that the hard-nav LCP is still embedded in the PageLoadMetrics -> PaintTiming.
But I guess if we can use the same structure (we already do), then we can send with the same pipe?
waiter->AddSoftNavigationImageLCPExpectation(1);Johannes HenkelThis is a soft-LCP test and will eventually get the soft-lcp from the page and from ukm it looks like-- so does it not need the waiter to wait?
(I dont undestand how this part works, sorry)
Yeah, I removed the SoftNavigation*LCPExpectation stuff, because there isn't a notification on the browser side for LCP like there is for the soft navs. And I think for now, there shouldn't be, because we'll use the pattern to ask the delegate for the LCP. Note that there is also no notification for soft lcp and soft inp.
If I wanted to keep these expectations, I'd have to set things up so that the waiter receives events for soft lcp. It's possible, e.g. by putting stuff into the metrics render frame observer, but this seems quite artificial since the normal page load metrics observers won't be doing that. Not waiting for the soft lcp is ok because the soft nav will only be recognized if there was a paint, and the ukm part of the test will receive that either way.
I think I have to defer to @sull...@chromium.org or just trust you and stamp on the design of this part!
Sorry-- I hope to get more insights here some day.
mojom::LargestContentfulPaintTimingPtr,Johannes HenkelShould this be a vector? I know that today it isnt-- today we overwrite the global value on renderer side before sending, and/or "force flush" to ensure we can send a stream of these.
but if we want to support stream buffering, I think we can just send the full list?
---
Or maybe you want to keep this patch 1:1 with existing and change in the future?
Yeah, would like to do the switch to vectors in a futurue cl, since there'll be more interesting logic in the browser to digest the vector also.
Acknowledged
return !timing.count;Johannes HenkelFWIW, I think that IsEmpty is only needed when we send a structure with each update, so its in like an "empty state", I think.
But, when you have vector<> of items, you can just check if its empty (for LCP this would work), and perhaps there is an optional<> concept for mojom args as well?
(These IsEmpty() checks smell terrible to me somehow)
Exactly. This will go away, because a vector is a generalization of optional. I didn't want to do the vector change right away though, since if it was a vector I'd have to change the logic in page_load_metrics_update_dispatcher etc., and I don't want to do that now.
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 3 comments![]( "Open in Gerrit")
Thanks a lot for your thoughts and checking this out. I scribbled some more, hopefully not too much. :-)
SoftNavigationMetrics mojom message, and instead sends the Soft LCP
separately. I've tried to keep this as mechanical and simple asJohannes HenkelWe don't have to, but, do you think the same message would be good to use for hard-LCP, or too risky that these have unique mechanics?
Today we have the same problem with hard-LCP sending. We don't have to solve both here, but just design-wise we use the same INP and the same CLS messages for hard and soft... can/should we do that for LCP?
Michal MocnyNot sure I understand 100%, but yes, soft lcp and hard lcp use the same messages.
blink::LargestContentfulPaintDetailsForReporting and mojom::LargestContentfulPaintTiming. This isn't changing. Hard LCP is a field of blink::WebPerformanceMetricsForReporting and mojom::PaintTiming.https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_performance_metrics_for_reporting.h;l=30?
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/common/page_load_metrics.mojom;l=36?
We have `page_load_timing.paint_timing.largest_contentful_paint` and then we have a distinct `soft_navigation_metrics.largest_contentful_paint`, each as separate "singletons" with each `UpdateTiming` call.
Now you are splitting out the LCP message as its own arg, but I think that the hard-nav LCP is still embedded in the PageLoadMetrics -> PaintTiming.
But I guess if we can use the same structure (we already do), then we can send with the same pipe?
I don't think hard LCP has a correctness problem with respect to UKM reporting, so I'd like to leave it as is if at all possible. I think it's OK that it's part of the timing message and that it gets filled in in the middle of MetricsRenderFrameObserver::GetTiming.
But soft lcp, we have inverted the direction of the messages in the renderer, so that now each soft lcp arrives to the page timing metrics sender in a forward direction. So, naturally, these should go into a vector (in a future cl), and then get sent whenever the sender sends.
More thoughts about the direction for future cls:
Each entry into the soft lcp vector should be tagged with the soft navigation that it belongs to. In https://chromium-review.googlesource.com/c/chromium/src/+/7500735, I started preparing for that, by adding the soft navigation count to the soft navigation context. This will make it easy to save that count into a field of LargestContentfulPaintTiming (and the equivalent ForReporting thing in blink) which will be 0 for hard navs, and have the count otherwise.
So then, they can be in a vector, in the UpdateTiming call.
The soft navigations need to also be in a vector, in case two of them arrive while the thing hasn't been sent yet. The page timing metrics sender should have very little logic, not even checks - just queue and send. The browser should check, because the renderer can't be trusted.
On the browser side, I want to put something similar to the SoftNavigationTracker in https://chromium-review.googlesource.com/c/chromium/src/+/7558920/3/components/page_load_metrics/browser/soft_navigation_tracking.h (but slightly simpler), which will be owned by the page load metrics update dispatcher.
Open question is where to make the timings relative. At the moment it's still the metrics render frame observer, and I'll try to get away with that for starters, since it doesn't affect correctness.
Once correctness gets better, I'd like to revive / expand the soft navigation metrics browsertest, which only covers soft lcp; it should cover cls and inp as well, of course.
PS: https://chromium-review.googlesource.com/c/chromium/src/+/7239589 landed yesterday, which changed the page_load_metrics.mojom's UpdateTiming thing. Merge conflicts were tedious to resolve but also fairly easy, because this change is quite modest in how much of the logic it changes.
waiter->AddSoftNavigationImageLCPExpectation(1);Johannes HenkelThis is a soft-LCP test and will eventually get the soft-lcp from the page and from ukm it looks like-- so does it not need the waiter to wait?
(I dont undestand how this part works, sorry)
Michal MocnyYeah, I removed the SoftNavigation*LCPExpectation stuff, because there isn't a notification on the browser side for LCP like there is for the soft navs. And I think for now, there shouldn't be, because we'll use the pattern to ask the delegate for the LCP. Note that there is also no notification for soft lcp and soft inp.
If I wanted to keep these expectations, I'd have to set things up so that the waiter receives events for soft lcp. It's possible, e.g. by putting stuff into the metrics render frame observer, but this seems quite artificial since the normal page load metrics observers won't be doing that. Not waiting for the soft lcp is ok because the soft nav will only be recognized if there was a paint, and the ukm part of the test will receive that either way.
I think I have to defer to @sull...@chromium.org or just trust you and stamp on the design of this part!
Sorry-- I hope to get more insights here some day.
Maybe to say it better, the test needs to wait before it clicks, and we're using that waiter mechanism, which ensures that we've seen a soft navigation coming from the metrics render frame observer. At that point, the soft navigation for sure has also manifested in the soft navigation heuristics. Since that requires paint, we know that this will also generate a soft lcp.
I think an alternative would be to do away with the waiter, and use a javascript snippet that uses the PerformanceObserver to ensure that we've emitted to the performance timeline. Perhaps we could wait for a specific soft lcp candidate in that way. We may need to do such a thing for the soft cls and soft inp coverage, which is still disabled. But, for the purpose of this change, it's not something I want to mess with, it looks like it passes just fine with what I've got.
Related details
- Annie Sullivan
- Michal Mocny
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 4 comments![]( "Open in Gerrit")
I double checked / reminded myself that we have decent test coverage for the soft lcp, in our integration test, and the removal of the soft lcp from the test waiter does not affect that.
// Soft navigation LCP.
auto source_id_to_lcp = GetSoftNavigationMetrics(
ukm_recorder(),
ukm::builders::SoftNavigation::kPaintTiming_LargestContentfulPaintName);
EXPECT_EQ(source_id_to_lcp.size(), expected_soft_nav_count);This fetches the soft LCP of the soft navigations, from UKM.
// If the SoftNavigationHeuristics flag is enabled, we verify exact values
// in UKM against the web exposed values.
if (soft_nav_heuristics_enabled) {
EXPECT_EQ(soft_nav_lcp_list.size(), expected_soft_nav_count);
for (uint32_t i = 0; i < soft_nav_lcp_list.size(); ++i) {
SCOPED_TRACE(base::StringPrintf("soft_nav_lcp_list[%d]", i));
double expected_lcp = soft_nav_lcp[i] + soft_nav_start_times[i];
EXPECT_NEAR(soft_nav_lcp_list[i].GetDouble(), expected_lcp, 6);
}
}This compares the web-exposed soft LCP values and the UKM soft LCP values.
VerifySoftNavIdsAndSoftLcpStartTimes(
soft_nav_lcp_list, /*expected_soft_nav_count=*/2);This is where we verify that we did in fact record the soft LCP, including in UKM.