LibWeb: Unload content documents when a navigable container is removed - #10890
LibWeb: Unload content documents when a navigable container is removed#10890sideshowbarker wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIframe removal now unloads active child documents and descendants before cleanup. Ready documents become Sequence Diagram(s)sequenceDiagram
participant IframeTest
participant NavigableContainer
participant ChildDocument
IframeTest->>NavigableContainer: remove iframe
NavigableContainer->>ChildDocument: unload document and descendants
ChildDocument-->>IframeTest: pagehide, visibilitychange, unload
IframeTest->>ChildDocument: assert visibilityState is hidden
Possibly related PRs
🚥 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.
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
`@Tests/LibWeb/Text/input/HTML/view-transition-in-removed-iframe-is-skipped.html`:
- Around line 11-14: Replace the fixed 50 ms sleep in the iframe removal test
with event-driven synchronization: install a visibilitychange listener on
childDocument before iframe.remove(), remove the iframe, and await the event
until childDocument.visibilityState is hidden. Preserve the subsequent
visibility logging and startViewTransition assertions.
🪄 Autofix (Beta)
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: 4b48864e-94c8-40f1-a3e5-c6e1d1db9d47
📒 Files selected for processing (10)
Libraries/LibWeb/DOM/Document.cppLibraries/LibWeb/HTML/NavigableContainer.cppTests/LibWeb/TestConfig.iniTests/LibWeb/Text/expected/HTML/iframe-removal-sets-visibility-state-hidden.txtTests/LibWeb/Text/expected/HTML/view-transition-in-removed-iframe-is-skipped.txtTests/LibWeb/Text/expected/wpt-import/page-visibility/iframe-unload.txtTests/LibWeb/Text/input/HTML/iframe-removal-sets-visibility-state-hidden.htmlTests/LibWeb/Text/input/HTML/view-transition-in-removed-iframe-is-skipped.htmlTests/LibWeb/Text/input/wpt-import/page-visibility/iframe-unload.htmlTests/LibWeb/Text/input/wpt-import/page-visibility/resources/iframe-with-subframes.html
7b900b9 to
89c0640
Compare
89c0640 to
0be384f
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
added
the
conflicts
0be384f to
f2ac9fa
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
Make our implementation of the HTML spec’s “destroy a child navigable” *unload* the content document and descendants — not just destroy them. That ensures each completed-loading page gets pagehide, visibility state set to “hidden” (firing visibilitychange), and unload — in that order. That matches Gecko, WebKit, and Blink. Also, run the unload algorithm’s visibility state update for pages that are ready for post-load tasks – rather than only for “page showing” ones. Thus, the never-navigated initial about:blank documents of child navigables — which engines treat as completely loaded — also become hidden, and fire visibilitychange. Fixes LadybirdBrowser#9893
f2ac9fa to
dbd9079
Compare
1 participant
Make our implementation of the HTML spec’s “destroy a child navigable” unload the content document and descendants — not just destroy them. That ensures each completed-loading page gets
pagehide, visibility state set to “hidden” (firingvisibilitychange), andunload— in that order. That matches Gecko, WebKit, and Blink. Also, run the unload algorithm’s visibility state update for pages that are ready for post-load tasks — rather than only for “page showing” ones. Thus, the never-navigated initialabout:blankdocuments of child navigables — which engines treat as completely loaded — also become hidden, and firevisibilitychange. Fixes #9893.whatwg/html#12288 has already been raised to report the misalignment between the spec as currently written and the actual Gecko/WebKit/Blink behavior.