Improve handling of about: URLs#10721
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughWebUI page metadata is centralized and exposed for lookup. About-page loading separates success and error callbacks. Autocomplete now suggests matching ChangesAbout pages and autocomplete
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Omnibox
participant Autocomplete
participant WebUI_pages
participant RemoteAutocomplete
Omnibox->>Autocomplete: query about:set
Autocomplete->>WebUI_pages: enumerate matching pages
WebUI_pages-->>Autocomplete: return about:settings suggestion
Autocomplete->>RemoteAutocomplete: skip remote query
Autocomplete-->>Omnibox: deliver WebUI suggestion
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Libraries/LibWebView/WebUI.cpp (1)
61-81: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAvoid making registry additions crash the process.
PageType::Dynamicallows a page into the creation path, but the creator dispatch remains a separate hard-coded chain. If a future dynamic page is added tos_pageswithout adding a branch here, line 80 reachesVERIFY(web_ui)and aborts. Couple each dynamic registry entry to its creator, or return a normal handled error for an unregistered implementation.🤖 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/WebUI.cpp` around lines 61 - 81, Update the WebUI creation dispatch around create_web_ui and VERIFY(web_ui) so every PageType::Dynamic entry from s_pages is guaranteed to have a corresponding creator, or replace the VERIFY failure with a normal handled error for missing implementations. Preserve the existing static-page rejection and registrations while preventing unknown dynamic pages from aborting the process.Tests/LibWebView/TestAutocomplete.cpp (1)
220-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMake remote-query suppression observable.
The autocomplete engine is disabled globally, so this passes even if the
about:early return is removed: the normal no-engine path also produces no remote search results. Configure an observable test engine/request hook and assert that anabout:setquery schedules zero remote requests.🤖 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 `@Tests/LibWebView/TestAutocomplete.cpp` around lines 220 - 238, The autocomplete test around about_query_completed currently cannot distinguish about: suppression from the globally disabled engine path. Configure an observable test autocomplete engine or request hook before invoking query_autocomplete_engine, track remote requests, and assert that querying "about:set" schedules zero remote requests while preserving the existing suggestion assertions.
🤖 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/WebUI.cpp`:
- Around line 61-81: Update the WebUI creation dispatch around create_web_ui and
VERIFY(web_ui) so every PageType::Dynamic entry from s_pages is guaranteed to
have a corresponding creator, or replace the VERIFY failure with a normal
handled error for missing implementations. Preserve the existing static-page
rejection and registrations while preventing unknown dynamic pages from aborting
the process.
In `@Tests/LibWebView/TestAutocomplete.cpp`:
- Around line 220-238: The autocomplete test around about_query_completed
currently cannot distinguish about: suppression from the globally disabled
engine path. Configure an observable test autocomplete engine or request hook
before invoking query_autocomplete_engine, track remote requests, and assert
that querying "about:set" schedules zero remote requests while preserving the
existing suggestion assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 308cb960-e570-4d96-aea3-709a03fa0f09
📒 Files selected for processing (8)
Libraries/LibWeb/Loader/ResourceLoader.cppLibraries/LibWeb/Loader/ResourceLoader.hLibraries/LibWebView/Autocomplete.cppLibraries/LibWebView/Autocomplete.hLibraries/LibWebView/WebUI.cppLibraries/LibWebView/WebUI.hTests/LibWebView/TestAutocomplete.cppTests/LibWebView/TestOmnibox.cpp
bf163f9 to
f2900fe
Compare
|
Rebased, and inserted a new commit at the start: with the previous changes, we now (correctly) error on invalid |
Setting an object's URL to `about:srcdoc` is not actually valid, and relies on our current buggy behaviour of always returning a blank page for unrecognized `about:` urls. Instead, point these at blank.html. For the contentWindow test, we also need to mark it to load from the HTTP server as otherwise, reading that property counts as accessing a cross-origin frame.
Treat a missing backing resource for an about URL as a failed load. This lets navigation create the existing error document instead of parsing an empty HTML response.
Keep internal page names, titles, and handler requirements in one registry. Handler creation now consults it, allowing other WebView features to discover the available pages.
Offer case-insensitive prefix matches for known about pages and support inline completion of those URLs. Keep about queries local instead of sending them to a remote autocomplete provider.
f2900fe to
67d1e6b
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 participant
Firstly, fix the error reporting for invalid
about:fooURLs. We had code to detect and report these, but weren't actually hitting it, and instead just produced a blank white page. Now we get an error.And then, show
about:URLs in the autocomplete suggestions. When the input starts withabout:, other suggestions are disabled.