script: Check whether window is top level before looking for it in the ScriptThread#42390
Merged
TimvdLippe merged 3 commits intoservo:mainfrom Feb 6, 2026
Merged
Conversation
Signed-off-by: Jo Steven Novaryo <[email protected]>
servo-highfive
added
the
S-awaiting-review
Signed-off-by: Jo Steven Novaryo <[email protected]>
mrobinson
reviewed
Feb 6, 2026
components/script/dom/window.rs
Outdated
| /// Returns the window proxy of the webview, which is the top-level ancestor browsing context. | ||
| /// Get the active [`Document`] of top-level browsing context, or return [`Window`]'s [`Document`] | ||
| /// if it's browing context is the top-level browsing context. Returning none if the [`WindowProxy`] | ||
| /// is discarded or the [`Document`] is in another thread (having dissimilar origin). |
Member
There was a problem hiding this comment.
A ScriptThread isn't necessarily in another thread (might be a process) and it might not have a dissimilar origin:
Suggested change
| /// is discarded or the [`Document`] is in another thread (having dissimilar origin). | |
| /// is discarded or the [`Document`] is in another `ScriptThread` |
Contributor
Author
There was a problem hiding this comment.
Ah, thanks! I have updated the comments.
components/script/dom/window.rs
Outdated
Comment on lines
+656
to
+657
| let window_proxy = self.undiscarded_window_proxy()?; | ||
| if window_proxy.is_top_level() { |
Member
There was a problem hiding this comment.
Why not just check Window::is_top_level here?
Contributor
Author
There was a problem hiding this comment.
Missed it! Thanks for pointing out.
components/script/dom/window.rs
Outdated
Comment on lines
+660
to
+663
| let top_level_window_proxy = self | ||
| .script_window_proxies | ||
| .find_window_proxy(window_proxy.webview_id().into())?; | ||
| top_level_window_proxy.document() |
Member
There was a problem hiding this comment.
No need for a temporary here:
Suggested change
| let top_level_window_proxy = self | |
| .script_window_proxies | |
| .find_window_proxy(window_proxy.webview_id().into())?; | |
| top_level_window_proxy.document() | |
| self | |
| .script_window_proxies | |
| .find_window_proxy(window_proxy.webview_id().into())? | |
| .document() |
components/script/dom/window.rs
Outdated
| .expect("Should always have a WindowProxy when calling WebdriverWindow"); | ||
| // Window must be top level browsing context. | ||
| assert!(window_proxy.browsing_context_id() == window_proxy.webview_id()); | ||
| assert!(window_proxy.is_top_level(), "Window must be top level browsing context."); |
Member
There was a problem hiding this comment.
Suggested change
| assert!(window_proxy.is_top_level(), "Window must be top level browsing context."); | |
| assert!(self.is_top_level(), "Window must be top level browsing context."); |
components/script/dom/window.rs
Outdated
| self.script_window_proxies | ||
| .find_window_proxy(window_proxy.webview_id().into()) | ||
| }) | ||
| pub(crate) fn top_level_document(&self) -> Option<DomRoot<Document>> { |
Member
There was a problem hiding this comment.
Maybe clarify that this returns the top-level Document only when it is local:
Suggested change
| pub(crate) fn top_level_document(&self) -> Option<DomRoot<Document>> { | |
| pub(crate) fn top_level_document_if_local(&self) -> Option<DomRoot<Document>> { |
mrobinson
changed the title
ScriptThread
mrobinson
approved these changes
Feb 6, 2026
| .script_window_proxies | ||
| .find_window_proxy(window_proxy.webview_id().into())? | ||
| .document() | ||
| } |
Member
There was a problem hiding this comment.
Small optimization here. Wait until knowing that the Window isn't top level to call Window::undiscarded_window_proxy(). Feel free to land with this change:
Suggested change
| } | |
| if self.is_top_level() { | |
| return Some(self.Document()) | |
| } | |
| let window_proxy = self.undiscarded_window_proxy()?; | |
| self | |
| .script_window_proxies | |
| .find_window_proxy(window_proxy.webview_id().into())? | |
| .document() |
servo-highfive
removed
the
S-awaiting-review
stevennovaryo
force-pushed
the
window-top-level
branch
from
076f393 to
ebc16d6
Compare
servo-highfive
added
the
S-awaiting-review
stevennovaryo
force-pushed
the
window-top-level
branch
from
ebc16d6 to
20d2111
Compare
Signed-off-by: Jo Steven Novaryo <[email protected]>
stevennovaryo
force-pushed
the
window-top-level
branch
from
20d2111 to
f53651b
Compare
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
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
4 participants
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.
Instead of immediately looking for the top-level browsing context's
Documentin theScriptThread, check whetherWindowis top-level beforehand. This is a small optimization.Testing: Existing WPTs (no behavior changes)