[soft navs] Simplify the FrameRenderDataUpdate mojom struct [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
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Annie Sullivan (Gerrit)
Annie Sullivan voted Code-Review+1![]( "Open in Gerrit")
- Johannes Henkel
- Michal Mocny
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 2 comments![]( "Open in Gerrit")
I love the direction of just sending the data and allowing the browser-side to slice using timestamps. This has been the long-arch direction of these metrics: report data and move more of the algo completely to browser.
However-- could you take it a step further and actually compare to the First Input or Scroll timestamp? (see below)
Instead, this change adds the after_input_or_scroll bool to theI notice that the `PaintTiming` part of mojo message already seems to report the `first_input_or_scroll_notified_timestamp;` and each shift reports its own timestamp...
Could we check if we have the FIoS time and compare timestamps?
Related details
- 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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
if (!has_seen_input_or_scroll_) {Also I see that we already guard on this here, which might have been set earlier but before the CLS values are accumulated.
I.e. I think there's a chance we have a shift and interaction buffered and sent together and we might skip accumulating it.
This isn't a big deal and would be rare, but I point it out because you might FIX this issue and see (expected) shifts in the field data as a result.
I suggest instead of comparing to a bool here, you compare actual timestamps.
Johannes Henkel (Gerrit)
Johannes Henkel added 2 comments![]( "Open in Gerrit")
Instead, this change adds the after_input_or_scroll bool to theI notice that the `PaintTiming` part of mojo message already seems to report the `first_input_or_scroll_notified_timestamp;` and each shift reports its own timestamp...
Could we check if we have the FIoS time and compare timestamps?
I don't want to do this for now, but added a comment.
Thanks a lot and please let me know if it's ok with you to get this submitted as is.
Also I see that we already guard on this here, which might have been set earlier but before the CLS values are accumulated.
I.e. I think there's a chance we have a shift and interaction buffered and sent together and we might skip accumulating it.
This isn't a big deal and would be rare, but I point it out because you might FIX this issue and see (expected) shifts in the field data as a result.
I suggest instead of comparing to a bool here, you compare actual timestamps.
This is a good point. I added a comment for now, I think for now this part here
isn't related to soft navigations and would add risk to the cl, also because
the timestamps I'd need to use aren't base::TimeTicks right now, they're converted several times, so this would require quite a bit more plumbing / refactoring.
Related details
- Michal Mocny
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 voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
4 comments
Instead, this change adds the after_input_or_scroll bool to theJohannes HenkelI notice that the `PaintTiming` part of mojo message already seems to report the `first_input_or_scroll_notified_timestamp;` and each shift reports its own timestamp...
Could we check if we have the FIoS time and compare timestamps?
I don't want to do this for now, but added a comment.
Thanks a lot and please let me know if it's ok with you to get this submitted as is.
Acknowledged this is fine for now and the mojo structs can iterate without backwards compat, so as long as its slightly better than before...
But we discussed yesterday leaning more into timestamp based segmentation so maybe just keep track of this in the design doc (or maybe you have already)
// potentially depends on timing behavior between the frames; therefore thisDo you think its only between frames or could it also be a single frame when the metrics sender buffers and sends these things together (unlikely I guess)
// layout shift timestmp with the earliest timestamp from any inputs orPlease fix this WARNING reported by Spellchecker: "timestmp" is a possible misspelling of "timestamp".
To bypass Spellchecker, ad...
"timestmp" is a possible misspelling of "timestamp".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
struct FrameRenderDataUpdate {Not for this patch, but, since this is just a wrapper around a single value, we could update `UpdateTiming` call to just pass the array, similar to new_features and resource timing updates:
- PageLoadTiming page_load_timing,
- FrameMetadata frame_metadata,
- array<blink.mojom.UseCounterFeature> new_features,
- array<ResourceDataUpdate> resources,
- FrameRenderDataUpdate render_data,
- CpuTiming cpu_load_timing,
- InputTiming input_timing_delta,
- SubresourceLoadMetrics? subresource_load_metrics,
- SoftNavigationMetrics soft_navigation_metrics);
(...we will also want an array of soft-navs and icp/lcp things)
Another alternative to to merge all these arrays into some sort of common wrapper but I dont know if thats useful.
Related details
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 3 comments![]( "Open in Gerrit")
Thanks a ton, both of you. Will try to submit this (after ipc review) and I will need to follow up on looking into canary data to make sure I'm not messing up the production CLS reporting.
// potentially depends on timing behavior between the frames; therefore thisDo you think its only between frames or could it also be a single frame when the metrics sender buffers and sends these things together (unlikely I guess)
For the single frame, the comparison is made in the layout shift tracker against this boolean field (see the link). It has an interesting comment there... but I think the gist is no, the timing behavior of the buffering between renderer and browser as is doesn't and shouldn't affect the outcome of that comparison.
// layout shift timestmp with the earliest timestamp from any inputs orPlease fix this WARNING reported by Spellchecker: "timestmp" is a possible misspelling of "timestamp".
To bypass Spellchecker, ad...
"timestmp" is a possible misspelling of "timestamp".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mea...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mea...@chromium.org
Reviewer source(s):
mea...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Mustafa Emre Acer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mustafa Emre Acer
| Code-Review | +1 |
1 comment
Mojo lgtm. Sorry for the delay!
Related details
- Johannes Henkel
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
1 comment
Thanks a lot, will try to submit this one.
Related details
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![]( "Open in Gerrit")
Change information
[soft navs] Simplify the FrameRenderDataUpdate mojom struct
Currently, we send two aggregated values, layout_shift_delta and
layout_shift_delta_before_input_or_scroll.
If a soft navigation were to occur in the middle of the array of the
|new_layout_shifts|, then we'd somehow need to split these
accumulations, or send multiple FrameRenderDataUpdate instances.
Instead, this change adds the after_input_or_scroll bool to the
LayoutShift struct, and computes the deltas on the browser side, when
they're used. Even though it's slightly more verbose there (loops), it
may actually be easier to understand, especially once we attribute some
of the shifts to before and after a soft navigation. This type of
comparison is possible for UKM, by comparing
LayoutShift.layout_shift_time (the field in the mojom record) with the
commit time of the soft navigation.
https://chromium-review.googlesource.com/c/chromium/src/+/7500735 is
preparing to have the commit time in the PageTimingMetricsSender.
An equivalent comparison can be made with web apis in Javascript, by
comparing the commitTime of the soft-navigation performance entry (which
we don't expose yet) with the startTime of the layout-shift performance
entry, or currently it's done by looking at the navigationId.
page_load_metrics.mojom: Remove the two fields which are the sum of the individual scores in the array, and add the bool to each array entry instead so these sums can be recomputed (including for slices of the array).
page_timing_metrics_sender.cc: Propagate the bool.
page_load_metrics_update_dispatcher.cc: Compute the sums as needed.
Remainder of the change does the obvious things; for the tests, I think they're now slightly more meaningful / detailed because previously, in some cases they actually had test data that was impossible - as in, the fields that are supposed a sum of the scores of the entries were set to something different.
Bug: 480135950
- M chrome/browser/page_load_metrics/observers/core/amp_page_load_metrics_observer.cc
- M chrome/browser/page_load_metrics/observers/core/amp_page_load_metrics_observer_unittest.cc
- M chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer_unittest.cc
- M chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc
- M components/page_load_metrics/browser/layout_shift_normalization_unittest.cc
- M components/page_load_metrics/browser/page_load_metrics_test_waiter.cc
- M components/page_load_metrics/browser/page_load_metrics_update_dispatcher.cc
- M components/page_load_metrics/common/page_load_metrics.mojom
- M components/page_load_metrics/renderer/fake_page_timing_sender.cc
- M components/page_load_metrics/renderer/page_timing_metrics_sender.cc
- M components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
Code-Review: +1 by Annie Sullivan, +1 by Mustafa Emre Acer, +1 by Michal Mocny
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |