[soft navs] Streamline soft navs integrationtest. [chromium/src : main]

0 views

Skip to first unread message

Scott Haseley (Gerrit)

unread,

May 19, 2026, 7:07:58 PM (8 hours ago) May 19

to Johannes Henkel, Annie Sullivan, Chromium LUCI CQ, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org

Attention needed from Annie Sullivan and Johannes Henkel

Scott Haseley voted and added 8 comments

Votes added by Scott Haseley

Code-Review

8 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Scott Haseley . resolved

LGTM w/ a few nits

File chrome/browser/page_load_metrics/integration_tests/soft_navigation_metrics_browsertest.cc

Line 265, Patchset 6 (Latest): EXPECT_TRUE(event_name != nullptr);

Scott Haseley . unresolved

nit: Maybe more idiomatic?

```suggestion
    EXPECT_TRUE(event_name);
```

Line 266, Patchset 6 (Latest):

    if (event_name == nullptr) {
      continue;
    }

Scott Haseley . unresolved

Can we ASSERT_TRUE() above and ditch this, or we can't because it's a non-test method?

Line 301, Patchset 6 (Latest): bool BlinkApiForSoftNavigationsIsEnabled() { return GetParam(); }

Scott Haseley . unresolved

nit: To match convention:

```suggestion
  bool IsBlinkApiForSoftNavigationsEnabled() { return GetParam(); }
```

Line 416, Patchset 6 (Latest): void GetPerformanceEntries(base::ListValue* entries) {

Scott Haseley . unresolved

nit: You might have had a reason for this, so feel free to ignore, but it might be slightly more readable if this (and below) returned a list?

Line 815, Patchset 6 (Latest): std::optional<int64_t> INP_worst_value = GetMetricFromUkmEntry(

Scott Haseley . unresolved

style nit: Here and elsewhere. I don't think we should upper-case this? Variables are all lower-case in chrome.

```suggestion
  std::optional<int64_t> inp_worst_value = GetMetricFromUkmEntry(
```

Line 830, Patchset 6 (Latest):

Scott Haseley . unresolved

ultra nit: either remove the empty line (830) or make it an empty `//` line since this comment applies directly to this block of code?

Line 1506, Patchset 6 (Latest): // FOr the second soft nav, a UKM CLS of 0 is recorded.

Scott Haseley . unresolved

```suggestion
  // For the second soft nav, a UKM CLS of 0 is recorded.
```

Open in Gerrit

Related details

Attention is currently required from:

Annie Sullivan
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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages