LibWebView: Run browser-UI traversals as queued history operations - #11101
Draft
shannonbooth wants to merge 3 commits into
Draft
LibWebView: Run browser-UI traversals as queued history operations#11101shannonbooth wants to merge 3 commits into
shannonbooth wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
💤 Files with no reviewable changes (9)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughHistory traversal APIs now complete asynchronously through queued operations. Legacy page, client, IPC, and precheck interfaces were removed. Modified arrow keys no longer trigger history traversal from Sequence Diagram(s)sequenceDiagram
participant ViewImplementation
participant CanonicalTraversable
participant HistoryOperationQueue
ViewImplementation->>CanonicalTraversable: request history traversal
CanonicalTraversable->>HistoryOperationQueue: enqueue browser history operation
HistoryOperationQueue-->>CanonicalTraversable: complete traversal operation
CanonicalTraversable-->>ViewImplementation: invoke completion callback
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Libraries/LibWebView/CanonicalTraversable.cpp (1)
810-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
traversal_requires_process_replacementfor the process-swap prediction.
traversal_requires_process_replacementat line 759 already wraps this exact call.ViewImplementation::start_requested_history_traversaluses it. Calling it here keeps one definition of the rule.♻️ Proposed refactor
- auto will_replace_web_content_process = SiteIsolationManager::the().navigation_requires_process_swap(view->url(), target.target_top_level_entry->url); + auto will_replace_web_content_process = traversal_requires_process_replacement(target, view->url());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWebView/CanonicalTraversable.cpp` at line 810, In the process-swap prediction within the relevant history traversal flow, replace the direct SiteIsolationManager::the().navigation_requires_process_swap call with the existing traversal_requires_process_replacement helper. Reuse that helper consistently with ViewImplementation::start_requested_history_traversal and remove the duplicate rule invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Libraries/LibWebView/CanonicalTraversable.cpp`:
- Line 810: In the process-swap prediction within the relevant history traversal
flow, replace the direct
SiteIsolationManager::the().navigation_requires_process_swap call with the
existing traversal_requires_process_replacement helper. Reuse that helper
consistently with ViewImplementation::start_requested_history_traversal and
remove the duplicate rule invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c1f8c7c-c4a7-4a02-860f-1498e3b1fb07
📒 Files selected for processing (21)
Libraries/LibWeb/HTML/HistoryOperation.hLibraries/LibWeb/Page/EventHandler.cppLibraries/LibWeb/Page/Page.cppLibraries/LibWeb/Page/Page.hLibraries/LibWebView/Application.cppLibraries/LibWebView/CanonicalTraversable.cppLibraries/LibWebView/CanonicalTraversable.hLibraries/LibWebView/ViewImplementation.cppLibraries/LibWebView/ViewImplementation.hLibraries/LibWebView/WebContentClient.cppLibraries/LibWebView/WebContentClient.hServices/WebContent/PageClient.cppServices/WebContent/PageClient.hServices/WebContent/WebContentClient.ipcTests/LibWeb/CMakeLists.txtTests/LibWeb/TestPage.cppTests/LibWebView/CMakeLists.txtTests/LibWebView/TestBrowserHistoryTraversal.cppTests/LibWebView/test-webdriver-session-history.pyUI/Qt/Menu.cppUI/Qt/WebContentView.cpp
💤 Files with no reviewable changes (9)
- Services/WebContent/PageClient.h
- Libraries/LibWeb/HTML/HistoryOperation.h
- Tests/LibWeb/CMakeLists.txt
- Libraries/LibWeb/Page/Page.cpp
- Services/WebContent/WebContentClient.ipc
- Libraries/LibWebView/WebContentClient.h
- Libraries/LibWeb/Page/Page.h
- Services/WebContent/PageClient.cpp
- Tests/LibWeb/TestPage.cpp
shannonbooth
force-pushed
the
site-isolation-part-20
branch
from
August 11, 2026 19:18
3c1faa9 to
c00eed4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Services/WebContent/WebDriverConnection.cpp`:
- Around line 626-630: Update the validation before
request_webdriver_press_history_traversal_key in the current WebDriver command
handler to accept only delta values of -1 or 1; return InvalidArgument for 0 and
values whose absolute value exceeds 1. Add endpoint tests covering zero and
out-of-range positive and negative values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3021142d-e762-487e-a14c-799308642368
📒 Files selected for processing (13)
Libraries/LibWeb/WebDriver/Client.cppLibraries/LibWeb/WebDriver/Client.hLibraries/LibWebView/WebContentClient.cppLibraries/LibWebView/WebContentClient.hServices/WebContent/PageClient.cppServices/WebContent/PageClient.hServices/WebContent/WebContentClient.ipcServices/WebContent/WebDriverClient.ipcServices/WebContent/WebDriverConnection.cppServices/WebContent/WebDriverConnection.hServices/WebDriver/Client.cppServices/WebDriver/Client.hTests/LibWebView/test-webdriver-session-history.py
🚧 Files skipped from review as they are similar to previous changes (3)
- Libraries/LibWebView/WebContentClient.h
- Services/WebContent/WebContentClient.ipc
- Services/WebContent/PageClient.h
shannonbooth
marked this pull request as draft
August 11, 2026 19:24
shannonbooth
force-pushed
the
site-isolation-part-20
branch
3 times, most recently
from
August 11, 2026 20:37
cf31cbf to
d07cef9
Compare
shannonbooth
marked this pull request as ready for review
August 11, 2026 20:49
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
Previously, browser-UI history traversal was split between CanonicalTraversable and ViewImplementation. CanonicalTraversable computed a HistoryTraversalDecision, then ViewImplementation interpreted it and queued the operation back onto the same traversable. The decision also carried UI updates and the traversal result. The result could be reported either synchronously or through the cancellation callback. This made WebDriver construct the same completion twice. Now, CanonicalTraversable starts the traversal as soon as it has computed the decision and reports a single completion. It invokes view methods directly for UI updates. New traversal flows no longer require additional decision fields or switch cases. The fallback path now uses the shared UI-process traversal loader. This also ensures that, when Back wins a cross-site race, a provisional WebContent process is replaced instead of being reused for the load.
Previously, browser traversal targets were resolved before the traversal entered the queue. If a second Forward was pressed while the first traversal was pending, it saw the session history from before that traversal and selected the same entry again. Now, the traversal is queued first. Its target is resolved when its queue slot starts, after earlier traversals have updated the session history. Each request sees the results of the requests ahead of it. This matches the specification's queued traversal steps. To-step traversals use the same path.
WebContent's EventHandler turned alt+arrow into a history traversal through a dedicated IPC message with its own precheck enum, a leftover from when the page and the browser chrome lived in one process. The requesting page can also host only a subframe of its traversable, which is why that message could not reuse the typed history-operation request. Match the back and forward keys in ViewImplementation once WebContent reports them unhandled instead. Pages keep their first shot at the keys, and Page::traverse_the_history_by_delta, the separate IPC message, and HistoryTraversalPrecheck all disappear. WebDriver key actions dispatch into the page and never reach a browser accelerator, so the session-history suite now drives its browser traversal scenarios through the from-UI test hook. The key path itself is covered by the browser history traversal test.
shannonbooth
force-pushed
the
site-isolation-part-20
branch
from
August 12, 2026 16:10
d07cef9 to
0b311a6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
removed
the
conflicts
shannonbooth
marked this pull request as draft
August 12, 2026 16:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This simplifies browser-UI back and forward now that the UI process
owns session history, and fixes (some specific cases of) rapid traversals.
The traversable starts its traversal operation directly where it
decides what to do, instead of returning a HistoryTraversalDecision for
ViewImplementation to interpret, and each traversal reports a single
completion. Targets now resolve at the traversal's queue position as
the specification describes, which fixes two rapid Forward presses
moving by only one entry.
Keyboard back and forward move out of WebContent and become UI
accelerators that fire when WebContent reports the keys unhandled.
Pages keep their first shot at the keys, and WebContent's dedicated
traversal IPC message is deleted. A new integration test drives both
the rapid-traversal fix and the keys through a live WebContent process.
This leaves less machinery to carry when traversal operations learn to
survive WebContent process swaps.