Remove kInitialWebUISyncNavStartToCommit flag. [chromium/src : main]
0 views
Skip to first unread message
Fergal Daly (Gerrit)
Jun 12, 2026, 6:32:39 AM (14 days ago) Jun 12
to Fergal Daly, Jerome Jiang, Mirko Bonadei, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, devtools...@chromium.org, penghuan...@chromium.org, net-r...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, fgal...@chromium.org, jz...@chromium.org, oshima...@chromium.org, chrome-intell...@chromium.org, mar...@chromium.org, feature-me...@chromium.org, omnibox-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, creis...@chromium.org, navigation...@chromium.org
Fergal Daly abandoned this change.
Fergal Daly abandoned this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Jun 17, 2026, 1:26:56 AM (9 days ago) Jun 17
to Fergal Daly, Rakina Zata Amni, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Rakina Zata Amni
Fergal Daly added 2 comments![]( "Open in Gerrit")
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1574, Patchset 8 (Latest):
webui_contents->GetController().Reload(content::ReloadType::NORMAL,Fergal Daly . unresolved
Will this `Reload` immediately drive everything as far as `Commit`? Is there any need to wait for anything?
And then, should
Line 1591, Patchset 8 (Latest):
base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(Fergal Daly . unresolved
Maybe this task should be posted before calling `Reload` to ensure it hits before `DidCommit`.
Related details
Attention is currently required from:
- Rakina Zata Amni
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. |
Rakina Zata Amni (Gerrit)
Jun 17, 2026, 4:51:30 AM (9 days ago) Jun 17
to Fergal Daly, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly
Rakina Zata Amni added 2 comments![]( "Open in Gerrit")
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1574, Patchset 8 (Latest):
webui_contents->GetController().Reload(content::ReloadType::NORMAL,Fergal Daly . unresolved
Will this `Reload` immediately drive everything as far as `Commit`? Is there any need to wait for anything?
And then, should
Rakina Zata Amni
Yeah I don't think you need to wait, just make sure you got the action before DidCommit?
Line 1591, Patchset 8 (Latest):
base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(Fergal Daly . unresolved
Maybe this task should be posted before calling `Reload` to ensure it hits before `DidCommit`.
Attention is currently required from:
- Fergal Daly
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. |
Fergal Daly (Gerrit)
Jun 17, 2026, 6:15:26 AM (9 days ago) Jun 17
to Fergal Daly, Ben Hamilton, Rakina Zata Amni, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Ben Hamilton and Rakina Zata Amni
Fergal Daly added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1578, Patchset 8 (Parent):
if (rfh) {Fergal Daly . unresolved
This old test code is odd. Why would `rfh` be null and why would we want that to result in a pass? Same for handler below. It might not even call `BindInterface` every time it's run.
I added `ASSERT_TRUE`s to make sure it does.
Line 1591, Patchset 8 (Latest):
base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(Fergal Daly . unresolved
Rakina Zata AmniMaybe this task should be posted before calling `Reload` to ensure it hits before `DidCommit`.
Can you use `DidCommitNavigationInterceptor` instead?
Fergal Daly
Do we want to post a task from there? That will definitely be run *after* `DidCommit`. I think I don't properly understand what this is testing.
Related details
Attention is currently required from:
- Ben Hamilton
- Rakina Zata Amni
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. |
Fergal Daly (Gerrit)
Jun 18, 2026, 12:22:30 AM (8 days ago) Jun 18
to Fergal Daly, Russ Hamilton, Rakina Zata Amni, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Rakina Zata Amni and Russ Hamilton
Fergal Daly added 4 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 9 (Latest):
Fergal Daly . resolved
PTAL, this is ready for review.
Russ, please check WebUIToolbarWebViewRaceTest.BindInterfaceAfterCloseRace.
Thanks,
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1574, Patchset 8:
webui_contents->GetController().Reload(content::ReloadType::NORMAL,Fergal Daly . resolved
Rakina Zata AmniWill this `Reload` immediately drive everything as far as `Commit`? Is there any need to wait for anything?
And then, should
Yeah I don't think you need to wait, just make sure you got the action before DidCommit?
Fergal Daly
It seems like I need to wait or the bind fails (too early?).
Line 1578, Patchset 8 (Parent):
if (rfh) {Fergal Daly . resolved
This old test code is odd. Why would `rfh` be null and why would we want that to result in a pass? Same for handler below. It might not even call `BindInterface` every time it's run.
I added `ASSERT_TRUE`s to make sure it does.
Fergal Daly
Done
Line 1591, Patchset 8:
base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(Fergal Daly . resolved
Rakina Zata AmniMaybe this task should be posted before calling `Reload` to ensure it hits before `DidCommit`.
Fergal DalyCan you use `DidCommitNavigationInterceptor` instead?
Do we want to post a task from there? That will definitely be run *after* `DidCommit`. I think I don't properly understand what this is testing.
Attention is currently required from:
- Rakina Zata Amni
- Russ Hamilton
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Jun 18, 2026, 8:40:09 AM (8 days ago) Jun 18
to Fergal Daly, Rakina Zata Amni, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly and Rakina Zata Amni
Russ Hamilton added 2 comments![]( "Open in Gerrit")
File chrome/browser/profiles/profile_keyed_service_browsertest.cc
File-level comment, Patchset 11 (Latest):
Russ Hamilton . unresolved
I don't understand the purpose of these changes. Can you explain?
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1579, Patchset 11 (Latest):
ASSERT_TRUE(
base::test::RunUntil([&]() { return observer.ready_to_commit(); }));Russ Hamilton . unresolved
Wouldn't it be more reliable if we used a RunLoop here and ReadyToCommitNavigation called it's QuitClosure? Or do we need the RunUntilIdle behavior for some reason?
Related details
Attention is currently required from:
- Fergal Daly
- Rakina Zata Amni
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. |
Rakina Zata Amni (Gerrit)
Jun 18, 2026, 10:26:19 PM (7 days ago) Jun 18
to Fergal Daly, Russ Hamilton, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly
Rakina Zata Amni added 2 comments![]( "Open in Gerrit")
Commit Message
Line 16, Patchset 11 (Latest):
- Remove InitialWebUIPageLoadMetricsObserverTest.OnFailedProvisionalLoad since there are no provisional loads for initial WebUI navigations anymore.
- Remove a bunch of expected services from some ProfileKeyedServiceGuestBrowserTest tests. These services were previously activated by throttles but now there are no throttles.Rakina Zata Amni . unresolved
Why is this not a problem when we had kInitialWebUISyncNavStartToCommit?
File content/test/navigation_simulator_impl.cc
Line 450, Patchset 11 (Latest):
if (request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . unresolved
Do you actually need this addition (same below)?
Related details
Attention is currently required from:
- Fergal Daly
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. |
Fergal Daly (Gerrit)
Jun 19, 2026, 7:05:13 AM (7 days ago) Jun 19
to Fergal Daly, Russ Hamilton, Rakina Zata Amni, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Rakina Zata Amni and Russ Hamilton
Fergal Daly added 3 comments![]( "Open in Gerrit")
Commit Message
Line 16, Patchset 11:
- Remove InitialWebUIPageLoadMetricsObserverTest.OnFailedProvisionalLoad since there are no provisional loads for initial WebUI navigations anymore.
- Remove a bunch of expected services from some ProfileKeyedServiceGuestBrowserTest tests. These services were previously activated by throttles but now there are no throttles.Rakina Zata Amni . resolved
Why is this not a problem when we had kInitialWebUISyncNavStartToCommit?
Fergal Daly
This is now moved into https://crrev.com/c/7960658 but it seems to do something special with flags and ignores the field trial flags.
```
browser_tests --gtest_filter=ProfileKeyedServiceGuestBrowserTest.GuestProfileOTR_NeededServices --enable-features=InitialWebUISyncNavStartToCommit
```
fails currently even though InitialWebUISyncNavStartToCommit is set true in field trial config
File chrome/browser/profiles/profile_keyed_service_browsertest.cc
I don't understand the purpose of these changes. Can you explain?
Fergal Daly
I have moved this change (with a tweak) into https://crrev.com/c/7960658 to try to make this simpler.
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1579, Patchset 11:
ASSERT_TRUE(
base::test::RunUntil([&]() { return observer.ready_to_commit(); }));Russ Hamilton . resolved
Wouldn't it be more reliable if we used a RunLoop here and ReadyToCommitNavigation called it's QuitClosure? Or do we need the RunUntilIdle behavior for some reason?
Fergal Daly
It turns out the the run was essential because without the RFH is the old one.
I've rewritten the test now so that it's entirely synchronous and we explicitly get the pending RFH instead of whichever one happens to be the live one.
There is actually a RunLoop in WaiterHelper however it quits immediately.
Related details
Attention is currently required from:
- Rakina Zata Amni
- Russ Hamilton
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. |
Fergal Daly (Gerrit)
Jun 19, 2026, 8:18:46 AM (7 days ago) Jun 19
to Fergal Daly, Russ Hamilton, Rakina Zata Amni, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Rakina Zata Amni and Russ Hamilton
Fergal Daly added 1 comment![]( "Open in Gerrit")
File content/test/navigation_simulator_impl.cc
Line 450, Patchset 11:
if (request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . unresolved
Do you actually need this addition (same below)?
Fergal Daly
Without these, we run into
```
FATAL:content/browser/renderer_host/navigation_throttle_registry_impl.cc:284] Check failed: !navigation_request_->IsInitialWebUINavigation().
```
See https://crrev.com/c/7961878/1?tab=checks where I've removed them for the tests that fail.
Maybe the comment should say "so we need to not register".
Rakina Zata Amni (Gerrit)
Jun 22, 2026, 4:20:58 AM (4 days ago) Jun 22
to Fergal Daly, Russ Hamilton, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly and Russ Hamilton
Rakina Zata Amni voted and added 4 comments![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +1 |
Rakina Zata Amni . resolved
LGTM, thanks!
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1546, Patchset 14 (Latest):
raw_ptr<content::RenderFrameHost> rfh_;Rakina Zata Amni . unresolved
Can this be a RenderFrameHostWrapper
File content/test/navigation_simulator_impl.cc
Line 450, Patchset 11:
if (request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . unresolved
Fergal DalyDo you actually need this addition (same below)?
Without these, we run into
```
FATAL:content/browser/renderer_host/navigation_throttle_registry_impl.cc:284] Check failed: !navigation_request_->IsInitialWebUINavigation().
```
See https://crrev.com/c/7961878/1?tab=checks where I've removed them for the tests that fail.Maybe the comment should say "so we need to not register".
Rakina Zata Amni
Oh right this is for unit tests. SGTM to tweak the comment a bit
Line 1738, Patchset 14 (Latest):
request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . unresolved
Please fix this WARNING reported by autoreview issue finding: Consider updating the comment block above (around line 1733) to also mention initial WebUI navigations, similar to the comment you updated in `RegisterTestThrottle`.
Related details
Attention is currently required from:
- Fergal Daly
- Russ Hamilton
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. |
Russ Hamilton (Gerrit)
Jun 22, 2026, 10:13:16 AM (3 days ago) Jun 22
to Fergal Daly, Rakina Zata Amni, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly
Russ Hamilton added 1 comment![]( "Open in Gerrit")
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1563, Patchset 14 (Latest):
// Regression test for crbug.com/478033216. Tests that an attempt to bind to
// `TrackedElementHandler` while the window is being closed. This previously
// caused a crash..Russ Hamilton . unresolved
```
// Regression test for crbug.com/478033216. Tests that an attempt to bind to
// `TrackedElementHandler` while the window is being closed does not cause a
// crash.
```
Related details
Attention is currently required from:
- Fergal Daly
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. |
Fergal Daly (Gerrit)
Jun 23, 2026, 1:52:47 AM (3 days ago) Jun 23
to Fergal Daly, Rakina Zata Amni, Russ Hamilton, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly and Russ Hamilton
Fergal Daly added 4 comments![]( "Open in Gerrit")
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1546, Patchset 14:
raw_ptr<content::RenderFrameHost> rfh_;Rakina Zata Amni . unresolved
Can this be a RenderFrameHostWrapper
Fergal Daly
I tried but it requires making RFHW copy-assignable but the rfh id is const. I think I'm happier to leave it as is.
Line 1563, Patchset 14:
// Regression test for crbug.com/478033216. Tests that an attempt to bind to
// `TrackedElementHandler` while the window is being closed. This previously
// caused a crash..Russ Hamilton . resolved
```
// Regression test for crbug.com/478033216. Tests that an attempt to bind to
// `TrackedElementHandler` while the window is being closed does not cause a
// crash.
```
Fergal Daly
Done
File content/test/navigation_simulator_impl.cc
Line 450, Patchset 11:
if (request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . resolved
Fergal DalyDo you actually need this addition (same below)?
Rakina Zata AmniWithout these, we run into
```
FATAL:content/browser/renderer_host/navigation_throttle_registry_impl.cc:284] Check failed: !navigation_request_->IsInitialWebUINavigation().
```
See https://crrev.com/c/7961878/1?tab=checks where I've removed them for the tests that fail.Maybe the comment should say "so we need to not register".
Oh right this is for unit tests. SGTM to tweak the comment a bit
Fergal Daly
Done
Line 1738, Patchset 14:
request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . unresolved
Please fix this WARNING reported by autoreview issue finding: Consider updating the comment block above (around line 1733) to also mention initial WebUI navigations, similar to the comment you updated in `RegisterTestThrottle`.
Attention is currently required from:
- Fergal Daly
- Russ Hamilton
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. |
Rakina Zata Amni (Gerrit)
Jun 23, 2026, 2:36:56 AM (3 days ago) Jun 23
to Fergal Daly, Fergal Daly, Russ Hamilton, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly, Fergal Daly and Russ Hamilton
Rakina Zata Amni voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +1 |
2 comments
File chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
Line 1546, Patchset 14:
raw_ptr<content::RenderFrameHost> rfh_;Rakina Zata Amni . resolved
Fergal DalyCan this be a RenderFrameHostWrapper
I tried but it requires making RFHW copy-assignable but the rfh id is const. I think I'm happier to leave it as is.
Rakina Zata Amni
Acknowledged
File content/test/navigation_simulator_impl.cc
Line 1738, Patchset 14:
request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . unresolved
Fergal DalyPlease fix this WARNING reported by autoreview issue finding: Consider updating the comment block above (around line 1733) to also mention initial WebUI navigations, similar to the comment you updated in `RegisterTestThrottle`.
Where is the autoreview warning?
Rakina Zata Amni
Yeah it's kinda confusing, it's not actually visible to you, it's just visible to the reviewer when they do the review agent thing
Related details
Attention is currently required from:
- Fergal Daly
- Fergal Daly
- Russ Hamilton
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. |
Rakina Zata Amni (Gerrit)
Jun 23, 2026, 3:00:33 AM (3 days ago) Jun 23
to Fergal Daly, Fergal Daly, Russ Hamilton, Ryan Sultanem, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly, Fergal Daly and Russ Hamilton
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
File content/test/navigation_simulator_impl.cc
Line 1738, Patchset 14:
request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . resolved
Fergal DalyPlease fix this WARNING reported by autoreview issue finding: Consider updating the comment block above (around line 1733) to also mention initial WebUI navigations, similar to the comment you updated in `RegisterTestThrottle`.
Rakina Zata AmniWhere is the autoreview warning?
Yeah it's kinda confusing, it's not actually visible to you, it's just visible to the reviewer when they do the review agent thing
Rakina Zata Amni
Done
Related details
Attention is currently required from:
- Fergal Daly
- Fergal Daly
- Russ Hamilton
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Jun 23, 2026, 3:02:48 AM (3 days ago) Jun 23
to Fergal Daly, Ryan Sultanem, Rakina Zata Amni, Russ Hamilton, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly, Russ Hamilton and Ryan Sultanem
Fergal Daly added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 16 (Latest):
Fergal Daly . resolved
+rsult for chrome/browser/profiles/profile_keyed_service_browsertest.cc
File content/test/navigation_simulator_impl.cc
Line 1738, Patchset 14:
request_->IsPageActivation() || request_->IsInitialWebUINavigation()) {Rakina Zata Amni . resolved
Fergal DalyPlease fix this WARNING reported by autoreview issue finding: Consider updating the comment block above (around line 1733) to also mention initial WebUI navigations, similar to the comment you updated in `RegisterTestThrottle`.
Rakina Zata AmniWhere is the autoreview warning?
Yeah it's kinda confusing, it's not actually visible to you, it's just visible to the reviewer when they do the review agent thing
Attention is currently required from:
- Fergal Daly
- Russ Hamilton
- Ryan Sultanem
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Jun 23, 2026, 4:02:23 AM (3 days ago) Jun 23
to Fergal Daly, Fergal Daly, Rakina Zata Amni, Russ Hamilton, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly and Russ Hamilton
Ryan Sultanem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Sultanem
| Code-Review | +1 |
1 comment
Patchset-level comments
Ryan Sultanem . resolved
LGTM for `chrome/browser/profiles/profile_keyed_service_browsertest.cc`
Related details
Attention is currently required from:
- Fergal Daly
- Russ Hamilton
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Jun 23, 2026, 11:50:35 AM (2 days ago) Jun 23
to Fergal Daly, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly
Russ Hamilton voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- Fergal Daly
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Jun 23, 2026, 10:41:11 PM (2 days ago) Jun 23
to Fergal Daly, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Fergal Daly voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Jun 23, 2026, 10:44:12 PM (2 days ago) Jun 23
to Fergal Daly, Andrea Orru, Nicolás Peña, Marlon Facey, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Andrea Orru, Marlon Facey and Nicolás Peña
Fergal Daly added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 17 (Latest):
Fergal Daly . resolved
Adding owners for
- c/b/e/renderer_startup_helper_browsertest.cc
- c/b/p/o/initial_webui_page_load_metrics_observer_browsertest.cc
- c/b/u/v/l/content_setting_interactive_uitest.cc
Related details
Attention is currently required from:
- Andrea Orru
- Marlon Facey
- Nicolás Peña
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marlon Facey (Gerrit)
Jun 24, 2026, 10:20:38 AM (yesterday) Jun 24
to Fergal Daly, Andrea Orru, Nicolás Peña, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Andrea Orru, Fergal Daly and Nicolás Peña
Marlon Facey voted and added 1 comment![]( "Open in Gerrit")
Votes added by Marlon Facey
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Andrea Orru
- Fergal Daly
- Nicolás Peña
Nicolás Peña (Gerrit)
Jun 24, 2026, 11:10:10 AM (yesterday) Jun 24
to Fergal Daly, Marlon Facey, Andrea Orru, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Andrea Orru and Fergal Daly
Nicolás Peña added 3 comments![]( "Open in Gerrit")
File chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_browsertest.cc
File chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_unittest.cc
Line 252, Patchset 17 (Parent):
std::unique_ptr<NavigationSimulator> simulator =Nicolás Peña . unresolved
Why is this test being deleted?
Related details
Attention is currently required from:
- Andrea Orru
- Fergal Daly
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. |
Fergal Daly (Gerrit)
Jun 24, 2026, 9:48:32 PM (24 hours ago) Jun 24
to Fergal Daly, Marlon Facey, Andrea Orru, Nicolás Peña, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Andrea Orru and Nicolás Peña
Fergal Daly added 3 comments![]( "Open in Gerrit")
File chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_browsertest.cc
File chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_unittest.cc
Line 252, Patchset 17 (Parent):
std::unique_ptr<NavigationSimulator> simulator =Nicolás Peña . unresolved
Why is this test being deleted?
Fergal Daly
InitialWebUI navigation is a synchronous navigation, there is no provisional load anymore, the navigation commits immediately. I've added this to the commit message.
Related details
Attention is currently required from:
- Andrea Orru
- Nicolás Peña
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. |
Nicolás Peña (Gerrit)
11:19 AM (10 hours ago) 11:19 AM
to Fergal Daly, Marlon Facey, Andrea Orru, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Andrea Orru and Fergal Daly
Nicolás Peña voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nicolás Peña
| Code-Review | +1 |
1 comment
File chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_unittest.cc
Line 252, Patchset 17 (Parent):
std::unique_ptr<NavigationSimulator> simulator =Nicolás Peña . unresolved
Fergal DalyWhy is this test being deleted?
InitialWebUI navigation is a synchronous navigation, there is no provisional load anymore, the navigation commits immediately. I've added this to the commit message.
Nicolás Peña
So this means we can remove the metrics recording code as well as the histograms/ukm? Are you planning to do this in a follow-up?
Related details
Attention is currently required from:
- Andrea Orru
- Fergal Daly
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. |
Andrea Orru (Gerrit)
6:17 PM (3 hours ago) 6:17 PM
to Fergal Daly, Nicolás Peña, Marlon Facey, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Attention needed from Fergal Daly
Andrea Orru voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrea Orru
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Fergal Daly
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. |
Fergal Daly (Gerrit)
8:55 PM (1 hour ago) 8:55 PM
to Fergal Daly, Nicolás Peña, Marlon Facey, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Fergal Daly voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fergal Daly
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
File chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_unittest.cc
Line 252, Patchset 17 (Parent):
std::unique_ptr<NavigationSimulator> simulator =Nicolás Peña . resolved
Fergal DalyWhy is this test being deleted?
Nicolás PeñaInitialWebUI navigation is a synchronous navigation, there is no provisional load anymore, the navigation commits immediately. I've added this to the commit message.
So this means we can remove the metrics recording code as well as the histograms/ukm? Are you planning to do this in a follow-up?
Fergal Daly
There's still a navigation and many stages of it to be recorded, it just doesn't have the 2-phases commit of most other navigations. I'm not sure what can be deleted. I've filed https://crbug.com/528072972.
I'll submit this CL since it has a lot of OWNERS, so I'll resolve this comment for now.
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)
9:00 PM (26 minutes ago) 9:00 PM
to Fergal Daly, Andrea Orru, Nicolás Peña, Marlon Facey, Russ Hamilton, Ryan Sultanem, Fergal Daly, Rakina Zata Amni, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metrics...@chromium.org, speed-metr...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, jdonnel...@chromium.org, navigation...@chromium.org, omnibox-...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Remove kInitialWebUISyncNavStartToCommit flag.
The behaviour after removal should be as if it was enabled.
This also removes the `IsInitialWebUISyncNavigation` method and replaced
all callers with `IsInitialWebUINavigation`.
This got quite messy.
- Update NavigationSimulator to no register throttles for initial WebUI navigations
- Remove InitialWebUIPageLoadMetricsObserverTest.OnFailedProvisionalLoad since there are no provisional loads for initial WebUI navigations anymore.
- Rewrite WebUIToolbarWebViewRaceTest.BindInterfaceAfterCloseRace. Now it asserts that the reference is null and that the rfh etc are not null, confirming that the timing of the attempt to bind is correct.
- Remove InitialWebUIPageLoadMetricsObserverTest.OnFailedProvisionalLoad as
synchronous navs don't have provisional loads.
Bug: 522636876
Change-Id: I12ee1697f53435702f8a42f686e58ab55136b704
Reviewed-by: Ryan Sultanem <rs...@google.com>
Reviewed-by: Fergal Daly <fer...@chromium.org>
Reviewed-by: Russ Hamilton <beham...@google.com>
Reviewed-by: Marlon Facey <mfa...@chromium.org>
Commit-Queue: Fergal Daly <fer...@chromium.org>
Reviewed-by: Andrea Orru <andre...@chromium.org>
Reviewed-by: Rakina Zata Amni <rak...@chromium.org>
Reviewed-by: Nicolás Peña <n...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1652845}
Files:
- M chrome/browser/extensions/renderer_startup_helper_browsertest.cc
- M chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_browsertest.cc
- M chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_unittest.cc
- M chrome/browser/profiles/profile_keyed_service_browsertest.cc
- M chrome/browser/ui/views/location_bar/content_setting_interactive_uitest.cc
- M chrome/browser/ui/views/toolbar/webui_toolbar_interactive_uitest.cc
- M chrome/browser/ui/views/toolbar/webui_toolbar_web_view_browsertest.cc
- M chrome/browser/ui/views/toolbar/webui_toolbar_web_view_uitest.cc
- M chrome/browser/ui/waap/initial_webui_browsertest.cc
- M content/browser/gpu/initial_gpu_channel_browsertest.cc
- M content/browser/renderer_host/commit_deferring_condition_runner.cc
- M content/browser/renderer_host/navigation_request.cc
- M content/browser/renderer_host/navigation_request.h
- M content/browser/renderer_host/navigation_throttle_registry_impl.cc
- M content/browser/renderer_host/navigation_throttle_runner.cc
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/webui/initial_webui_browsertest.cc
- M content/browser/webui/initial_webui_navigation_url_loader.cc
- M content/public/browser/navigation_handle.h
- M content/public/common/content_features.cc
- M content/public/common/content_features.h
- M content/public/test/mock_navigation_handle.h
- M content/renderer/render_frame_impl.cc
- M content/test/navigation_simulator_impl.cc
- M testing/variations/fieldtrial_testing_config.json
Change size: L
Delta: 25 files changed, 104 insertions(+), 216 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Andrea Orru, +1 by Fergal Daly, +1 by Nicolás Peña, +1 by Russ Hamilton, +1 by Ryan Sultanem, +1 by Marlon Facey, +1 by Rakina Zata Amni
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages