[soft navs] Add MergeAndClear to INP normalization abstraction in browser [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 and added 1 comment![]( "Open in Gerrit")
Votes added by Annie Sullivan
| Code-Review | +1 |
1 comment
This looks good to me but I'd like Michal to take a look too
Related details
- 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")
std::vector<mojom::UserInteractionLatencyPtr> worst_ten_latencies_ptrs;Nit: name this "new_interactions" or "other_interactions" or something, at first read I thought this was our cached internal worst_ten (and there is no reason to think the incoming list is necessarily ten)
worst_ten_latencies_ptrs.reserve(other->worst_ten_latencies_.size());I think you can just std::move the other worst_ten into this temporary, then save on reserving and cloning, or no?
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 4 comments![]( "Open in Gerrit")
std::vector<mojom::UserInteractionLatencyPtr> worst_ten_latencies_ptrs;Nit: name this "new_interactions" or "other_interactions" or something, at first read I thought this was our cached internal worst_ten (and there is no reason to think the incoming list is necessarily ten)
Done
worst_ten_latencies_ptrs.reserve(other->worst_ten_latencies_.size());I think you can just std::move the other worst_ten into this temporary, then save on reserving and cloning, or no?
Yeah I can do it if I change the type of worst_ten_latencies_ to hold pointers.
worst_ten_latencies_ptrs.reserve(other->worst_ten_latencies_.size());I think you can just std::move the other worst_ten into this temporary, then save on reserving and cloning, or no?
The reason for what I had was to deal with the difference between an array of value and an array of pointers. The value array had probably been made the type of the field for efficiency, but now that we want to support this operation, we may be better off with pointers, so I've switched it. Hope it's cool.
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 2 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
2 comments
worst_ten_latencies_ptrs.reserve(other->worst_ten_latencies_.size());Johannes HenkelI think you can just std::move the other worst_ten into this temporary, then save on reserving and cloning, or no?
Yeah I can do it if I change the type of worst_ten_latencies_ to hold pointers.
Huh, I guess that `mojom::UserInteractionLatency` doesn't support move semantics...
Looking at the auto generated code there's no explicit move support, but gemini suggests there should be a default copy constructor which move() will fallback to using. I would have hoped there would be a way to explicitly enable move() semantics on mojo structs, but I dont see mention in mojo docs.
It does seem like using `Ptr` types is the typical solution, which sucks, but seems like the best option.
Maybe Scott has insights / oppinions?
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
2 comments
Thanks a lot!
worst_ten_latencies_ptrs.reserve(other->worst_ten_latencies_.size());Johannes HenkelI think you can just std::move the other worst_ten into this temporary, then save on reserving and cloning, or no?
Michal MocnyYeah I can do it if I change the type of worst_ten_latencies_ to hold pointers.
Huh, I guess that `mojom::UserInteractionLatency` doesn't support move semantics...
Looking at the auto generated code there's no explicit move support, but gemini suggests there should be a default copy constructor which move() will fallback to using. I would have hoped there would be a way to explicitly enable move() semantics on mojo structs, but I dont see mention in mojo docs.
It does seem like using `Ptr` types is the typical solution, which sucks, but seems like the best option.
Maybe Scott has insights / oppinions?
I think it's fine with the Ptr types for the elements. What we get from the mojom message is Ptrs anyway due to the way the bindings work, e.g. if you look at the call sites for the AddNewUserInteractionLatencies in this class, the arguments all come in ans vectors of pointers of structs due to the bindings anyway. I think we're ok-ish.
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] Add MergeAndClear to INP normalization abstraction in browser
ResponsivenessMetricsNormalization::MergeAndClear merges the data
from a second normalization object into the first one, and clears
the second. This allows us to start a second normalization object
when a provisional soft navigation (URL change caused by an interaction)
is detected, and then either continue the separation between the two
objects upon confirmation, or undo them by invoking MergeAndClear.
Bug: 480135950
- M components/page_load_metrics/browser/responsiveness_metrics_normalization.cc
- M components/page_load_metrics/browser/responsiveness_metrics_normalization.h
- M components/page_load_metrics/browser/responsiveness_metrics_normalization_unittest.cc
Code-Review: +1 by Michal Mocny, +1 by Annie Sullivan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |