[soft navs] Simplify UKM_PLMO::RecordSoftNavigationMetrics(). [chromium/src : main]
0 views
Skip to first unread message
Johannes Henkel (Gerrit)
Apr 28, 2026, 2:08:12 PM (yesterday) Apr 28
to Scott Haseley, Annie Sullivan, Chromium LUCI CQ, chromium...@chromium.org, bmcquad...@chromium.org, core-web-vita...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org
Attention needed from Annie Sullivan and Scott Haseley
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Annie Sullivan
- Scott Haseley
Submit Requirements:
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)
Apr 28, 2026, 2:13:32 PM (yesterday) Apr 28
to Johannes Henkel, Scott Haseley, Chromium LUCI CQ, chromium...@chromium.org, bmcquad...@chromium.org, core-web-vita...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org
Attention needed from Johannes Henkel and Scott Haseley
Annie Sullivan voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Johannes Henkel
- Scott Haseley
Submit Requirements:
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)
Apr 28, 2026, 2:14:12 PM (yesterday) Apr 28
to Scott Haseley, Annie Sullivan, Chromium LUCI CQ, chromium...@chromium.org, bmcquad...@chromium.org, core-web-vita...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org
Johannes Henkel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
1 comment
Patchset-level comments
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Apr 28, 2026, 2:17:34 PM (yesterday) Apr 28
to Johannes Henkel, Annie Sullivan, Chromium LUCI CQ, chromium...@chromium.org, bmcquad...@chromium.org, core-web-vita...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org
Attention needed from Johannes Henkel
Scott Haseley voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
3 comments
Patchset-level comments
Scott Haseley . resolved
LGTM
Commit Message
Line 15, Patchset 2 (Latest):
Change-Id: Id906e036760e2584493d53cd4f6dfabe3ab4a972Scott Haseley . unresolved
nit: is there an existing bug id for tracking?
File chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer.h
Line 380, Patchset 2 (Latest):
int64_t soft_navigation_count_ = 0;Scott Haseley . unresolved
nit: should this be uint64_t since negative values don't make sense? I guess int64_t matches the ukm builder though, so maybe not (or use base::saturated_cast<> to convert)?
Related details
Attention is currently required from:
- Johannes Henkel
Submit Requirements:
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)
Apr 28, 2026, 2:45:10 PM (yesterday) Apr 28
to Scott Haseley, Annie Sullivan, Chromium LUCI CQ, chromium...@chromium.org, bmcquad...@chromium.org, core-web-vita...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org
Johannes Henkel voted and added 3 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
3 comments
Patchset-level comments
Commit Message
nit: is there an existing bug id for tracking?
Johannes Henkel
I picked the one about prerender and bfcache, since it's sorta preparation for that.
File chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer.h
nit: should this be uint64_t since negative values don't make sense? I guess int64_t matches the ukm builder though, so maybe not (or use base::saturated_cast<> to convert)?
Johannes Henkel
Yeah, maybe keeping it simple is best, for consistency with the ukm builder. However I added a couple of CHECKs that it's not negative.
Related details
Attention set is empty
Submit Requirements:
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)
Apr 28, 2026, 5:11:44 PM (23 hours ago) Apr 28
to Johannes Henkel, Scott Haseley, Annie Sullivan, chromium...@chromium.org, bmcquad...@chromium.org, core-web-vita...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer.cc
Insertions: 5, Deletions: 3.
@@ -880,6 +880,7 @@
}
void UkmPageLoadMetricsObserver::OnSoftNavigation() {
+ CHECK_GE(soft_navigation_count_, 0);
soft_navigation_count_++;
// When the 1st soft navigation comes in, we record the
// soft_navigation_interval_responsiveness_metrics_normalization_ as INP
@@ -1226,15 +1227,16 @@
}
void UkmPageLoadMetricsObserver::RecordLastSoftNavigation() {
- ukm::builders::PageLoad builder(GetDelegate().GetPageUkmSourceId());
-
- builder.SetSoftNavigationCount(soft_navigation_count_);
+ CHECK_GE(soft_navigation_count_, 0);
// Record last soft navigation metrics. The smallest count that would be set
// for an actual soft navigation metric is 1.
if (soft_navigation_count_) {
RecordSoftNavigationMetrics();
}
+
+ ukm::builders::PageLoad builder(GetDelegate().GetPageUkmSourceId());
+ builder.SetSoftNavigationCount(soft_navigation_count_);
builder.Record(ukm::UkmRecorder::Get());
// Record soft navigation count histogram to UMA.
```
Change information
Commit message:
[soft navs] Simplify UKM_PLMO::RecordSoftNavigationMetrics().
Instead of passing the mojom record into the method, use a field to
count the soft navs (like we do for the other observers, bfcache and
prerender) and only access the mojom record inside
RecordSoftNavigationMetrics. This makes things more consistent with the
usage in the bfcache and prerender ukm observers.
Bug: 496664486
Change-Id: Id906e036760e2584493d53cd4f6dfabe3ab4a972
Commit-Queue: Johannes Henkel <joha...@chromium.org>
Reviewed-by: Scott Haseley <shas...@chromium.org>
Reviewed-by: Annie Sullivan <sull...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1622021}
Files:
- M chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer.cc
- M chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer.h
Change size: M
Delta: 2 files changed, 22 insertions(+), 29 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Annie Sullivan, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages