script: Use stylo dom apis for querySelector/querySelectorAll#42991
Conversation
servo-highfive
added
the
S-awaiting-review
simonwuelker
force-pushed
the
query-selector
branch
from
1688404 to
fd61a02
Compare
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Great stuff! Perhaps other want to look at this too.
|
|
||
| /// Get a reference to the internal value. | ||
| /// | ||
| /// ## SAFETY | ||
| /// This function effectively circumvents all the safety provided by `LayoutDom` as it allows | ||
| /// performing arbitrary (potentially mutating) operations on the value. Use with caution! | ||
| pub(crate) unsafe fn as_ref(self) -> &'dom T { |
There was a problem hiding this comment.
I'm not sure this has to be unsafe. It's pub(crate) which means it cannot be called from layout (the real danger as layout accesses nodes from multiple threads). In addition, this consumes the LayoutDom, which is great, because it ensure that LayoutDom can't later be passed to layout.
There was a problem hiding this comment.
Nothing prevents someone from calling this method on a different thread in script though. pub(crate) doesn't really give us any safety guarantees.
LayoutDom itself is not Send. But its possible to wrap a LayoutDom<Node> in a ServoLayoutNode (safe) which is Send, then pass it to another thread (safe), call get_jsmanaged on it (safe) and then use as_ref to obtain a reference to the node. At that point we have multiple references to a non-sync value on diferent threads, so something we did has to be marked as unsafe. All the steps that exist so far are safe, so let's make as_ref unsafe.
|
|
||
| // Step 3. Return the result of match a selector against a tree with selector | ||
| // and node’s root using scoping root node. | ||
| query_selector::<ServoLayoutElement<'dom>, Q>( |
There was a problem hiding this comment.
Do you know if this uses multiple threads or not?
There was a problem hiding this comment.
I don't know for certain but from quickly glancing at the code it doesn't seem to use threads. The DOM node we pass to query_selector is also not required to implement Send or Sync
| { | ||
| let elements_with_id = self.document.elements_with_id(&id.0); | ||
|
|
||
| // SAFETY: ServoLayoutElement is known to have the same layout and alignment as LayoutDom<Element> |
There was a problem hiding this comment.
Spooky, but it makes sense. Is it possible to use a compile time assertion that the sizes of the two data structures are the same?
There was a problem hiding this comment.
Sure. We already depend on static_assertions via stylo, so I'll use that.
There was a problem hiding this comment.
Unfortunately that doesn't work because these types are generic and generic_const_exprs is not stable yet.
|
Note to self: check if #42976 triggers new asserts in this code. |
|
Yes, that should do it. |
Signed-off-by: Simon Wülker <[email protected]>
Signed-off-by: Simon Wülker <[email protected]>
Signed-off-by: Simon Wülker <[email protected]>
Signed-off-by: Simon Wülker <[email protected]>
Signed-off-by: Simon Wülker <[email protected]>
simonwuelker
force-pushed
the
query-selector
branch
from
f3debd2 to
3c2fa7d
Compare
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
…_layout_dom` (#43032) As requested during the review of #42991. These functions are currently called `{to, from}_layout_js` or sometimes `get_jsmanaged` - this change unifies them as `{to, from}_layout_dom` for clarity. Testing: Covered by existing tests Signed-off-by: Simon Wülker <[email protected]>
…42991) Stylo has ready-to-use apis for `Node::querySelector` and `Node::querySelectorAll`. We currently instead set up our own selector queries, which is neither as correct nor as performant, so this change makes servo use the stylo interfaces instead. This change contains a fair amount of unsafe code - review with caution! Notably, this makes the tab component from the astro framework work correctly in servo (https://starlight.astro.build/components/tabs/). I've seen that (broken) component on multiple websites. Fixes servo#41105 Testing: New tests start to pass --------- Signed-off-by: Simon Wülker <[email protected]>
4 participants
Stylo has ready-to-use apis for
Node::querySelectorandNode::querySelectorAll. We currently instead set up our own selector queries, which is neither as correct nor as performant, so this change makes servo use the stylo interfaces instead.This change contains a fair amount of unsafe code - review with caution!
Notably, this makes the tab component from the astro framework work correctly in servo (https://starlight.astro.build/components/tabs/). I've seen that (broken) component on multiple websites.
Fixes #41105
Testing: New tests start to pass