[actor] Ensure new tabs are loaded when created using TabManagementTool [chromium/src : main]

[Kevin Graney (Gerrit)]

Attention needed from Kene Okoye

Kevin Graney added 1 comment

File chrome/browser/actor/tools/history_tool.cc

Line 114, Patchset 12 (Latest): std::optional<ObservationDelayController::PageStabilityConfig>
page_stability_config) {

Kevin Graney . unresolved

Can you change `Tool::GetObservationDelayer` to take a `PageStabilityConfig` (and not a `std::optional`)? Working back up the stack to where the optional is set might be a useful exercise. I think with certain features set the `value()` calls you're adding will cause crashes?
https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/actor/actor_utils.cc;l=11-19;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f

Open in Gerrit

Related details

Attention is currently required from:

• Kene Okoye

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ic8e3edf83b458b1ad871f15724da7bb90b74bcd5

Gerrit-Change-Number: 7004069

Gerrit-PatchSet: 12

Gerrit-Owner: Kene Okoye <ke...@google.com>

Gerrit-Reviewer: David Bokan <bo...@chromium.org>

Gerrit-Reviewer: Kene Okoye <ke...@google.com>

Gerrit-Reviewer: Kevin Graney <k...@google.com>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Christine Ying <chr...@google.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kene Okoye <ke...@google.com>

Gerrit-Comment-Date: Fri, 10 Oct 2025 16:38:18 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Kene Okoye (Gerrit)]

Attention needed from Kevin Graney

Kene Okoye added 1 comment

File chrome/browser/actor/tools/history_tool.cc

Line 114, Patchset 12: std::optional<ObservationDelayController::PageStabilityConfig>
page_stability_config) {

Kevin Graney . resolved

Can you change `Tool::GetObservationDelayer` to take a `PageStabilityConfig` (and not a `std::optional`)? Working back up the stack to where the optional is set might be a useful exercise. I think with certain features set the `value()` calls you're adding will cause crashes?
https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/actor/actor_utils.cc;l=11-19;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f

Kene Okoye

Done. I had to revise the `ObservationDelayController::PageStabilityConfig` struct to have an additional .enabled field set to false by default to indicate whether page stability monitoring is enabled (which is used instead of returning `std::nullopt` in `ToolRequest::GetObservationPageStabilityConfig()`).

Open in Gerrit

Related details

Attention is currently required from:

• Kevin Graney

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ic8e3edf83b458b1ad871f15724da7bb90b74bcd5

Gerrit-Change-Number: 7004069

Gerrit-PatchSet: 13

Gerrit-Owner: Kene Okoye <ke...@google.com>

Gerrit-Reviewer: David Bokan <bo...@chromium.org>

Gerrit-Reviewer: Kene Okoye <ke...@google.com>

Gerrit-Reviewer: Kevin Graney <k...@google.com>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Christine Ying <chr...@google.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kevin Graney <k...@google.com>

Gerrit-Comment-Date: Fri, 10 Oct 2025 19:36:11 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Kevin Graney <k...@google.com>

[Kevin Graney (Gerrit)]

Attention needed from Kene Okoye

Kevin Graney added 2 comments

File chrome/browser/actor/tools/history_tool_request.cc

Line 52, Patchset 14 (Latest): if (UseGeneralPageStabilityNavigationTools()) {
config.enabled = true;
}

Kevin Graney . unresolved

With an enabled/disabled bit in `PageStabilityConfig` now, should this check happen during the construction of that object rather than everywhere that constructs one?

File chrome/browser/actor/tools/observation_delay_controller.h

Line 42, Patchset 14 (Latest): // Whether to enable page stability monitoring at all.
bool enabled = false;

Kevin Graney . unresolved

I'll look more closely at this change next week, but are you sure you need/want to do this? What does this get you over having a `std::optional<ObservationDelayController>` that's `nullopt` when page stability is disabled?

Is it not enough to just return an `ObservationDelayController` from the `TabManagementTool` and make the change to `ObservationDelayController::Wait()`.

Open in Gerrit

Related details

Attention is currently required from:

• Kene Okoye

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ic8e3edf83b458b1ad871f15724da7bb90b74bcd5

Gerrit-Change-Number: 7004069

Gerrit-PatchSet: 14

Gerrit-Owner: Kene Okoye <ke...@google.com>

Gerrit-Reviewer: David Bokan <bo...@chromium.org>

Gerrit-Reviewer: Kene Okoye <ke...@google.com>

Gerrit-Reviewer: Kevin Graney <k...@google.com>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Christine Ying <chr...@google.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kene Okoye <ke...@google.com>

Gerrit-Comment-Date: Fri, 10 Oct 2025 20:40:31 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No