New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] 15270 Parameterize DOM Node Traversal Methods #15503
Conversation
Heads up! This PR modifies the following files:
|
r? @jdm |
Status - At least one of my iterators is currently broken and I'm trying to figure out which one(s) are incorrect. I've rewritten them several times but I'm not currently sure which one to focus on - I think I may have been staring at this too long and need a new set of eyes. Once the iterators are working, I'd like to add while() loops to all of them, as in the first two examples from #14451 so the parameter can be used for filtering if necessary. Finally, I'll take advantage of that where the iterators are currently being called from. |
Did you manage to progress on this, @DominoTree? |
I meant to take a look at this last week but got swamped by other work. If I don't get a chance to do so today or tomorrow, I'll hand it off to someone else. |
Haven't had a chance to dig into this much further yet, but any guidance would be appreciated. I should have some time this week. The first commit was my first attempt, and the second was mostly cleaning things up to isolate issues a bit more. I think there were a few other minor changes I made to my local copy to fix some bugs that I haven't pushed up, but I'll have to take a look at it and see where things ended up. |
@bors-servo try |
[WIP] 15270 Parameterize DOM Node Traversal Methods <!-- Please describe your changes on the following line: --> Think this is getting pretty close now. Made some ugly edits to try and ensure exact functionality is preserved with iterators. Once I identify the issue I'm seeing, I'll clean them up. I've also temporarily made all of the places calling iterators with the new type parameters only use `Node` or `Element` - this way they should have identical functionality as they did before, which should make debugging the iterators themselves easier. Once the iterators are done, we can clean up where they're being called from as well. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #15270 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15503) <!-- Reviewable:end -->
@bors-servo try- Never mind, didn't notice the second commit. Will experiment locally. |
I believe all of the iterators should have |
@DominoTree What loop do you mean? |
Sorry, in the first commit, if you look at the |
@DominoTree Please remove the second commit, I don't want the simplifications that undo the improvements from the first in the tree, and I don't want to review it either. I think I found the issue with your refactoring and will soon submit the review. |
@nox I will get that reverted shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few logic errors in the iterators and many nits.
@@ -225,12 +225,12 @@ impl CharacterDataMethods for CharacterData { | |||
|
|||
// https://dom.spec.whatwg.org/#dom-nondocumenttypechildnode-previouselementsibling | |||
fn GetPreviousElementSibling(&self) -> Option<Root<Element>> { | |||
self.upcast::<Node>().preceding_siblings().filter_map(Root::downcast).next() | |||
self.upcast::<Node>().preceding_siblings::<Element>().next() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a type annotation here.
} | ||
|
||
// https://dom.spec.whatwg.org/#dom-nondocumenttypechildnode-nextelementsibling | ||
fn GetNextElementSibling(&self) -> Option<Root<Element>> { | ||
self.upcast::<Node>().following_siblings().filter_map(Root::downcast).next() | ||
self.upcast::<Node>().following_siblings::<Element>().next() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a type annotation here.
components/script/dom/document.rs
Outdated
@@ -482,7 +482,7 @@ impl Document { | |||
/// https://github.com/w3c/web-platform-tests/issues/2122 | |||
pub fn refresh_base_element(&self) { | |||
let base = self.upcast::<Node>() | |||
.traverse_preorder() | |||
.traverse_preorder::<Node>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.upcast::<Node>().traverse_preorder::<HTMLBaseElement>()
components/script/dom/document.rs
Outdated
@@ -679,7 +679,7 @@ impl Document { | |||
.map_or(false, |attr| &**attr.value() == name) | |||
}; | |||
let doc_node = self.upcast::<Node>(); | |||
doc_node.traverse_preorder() | |||
doc_node.traverse_preorder::<Node>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove both the type annotation and the .filter_map(Root::downcast)
part. If it doesn't compile, you can still do:
doc_node.traverse_preorder::<HTMLAnchorElement>()
components/script/dom/document.rs
Outdated
@@ -1809,8 +1809,7 @@ impl Document { | |||
/// Iterate over all iframes in the document. | |||
pub fn iter_iframes(&self) -> impl Iterator<Item=Root<HTMLIFrameElement>> { | |||
self.upcast::<Node>() | |||
.traverse_preorder() | |||
.filter_map(Root::downcast::<HTMLIFrameElement>) | |||
.traverse_preorder::<HTMLIFrameElement>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this type annotation is needed.
components/script/dom/node.rs
Outdated
self.current = Some(next_sibling); | ||
return Some(current); | ||
self.current = Some(next_sibling.clone()); | ||
return Root::downcast::<T>(current); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark about None
, the iterator should never return None
until it's done with the whole tree.
components/script/dom/node.rs
Outdated
} | ||
self.depth -= 1; | ||
} | ||
debug_assert!(self.depth == 0); | ||
self.current = None; | ||
Some(current) | ||
//TODO: dominotree - not sure how to handle failures here - is returning None okay if it can't cast to T? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark about downcasting and unwrap
.
components/script/dom/node.rs
Outdated
self.current = Some(first_child.clone()); | ||
if let Some(first_child) = Root::downcast::<T>(first_child) { | ||
return Some(first_child); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not seem correct to me.
components/script/dom/node.rs
Outdated
if self.child_elements().any(|c| c.upcast::<Node>() != child) { | ||
return Err(Error::HierarchyRequest); | ||
if self.children::<Element>().any(|c| c.upcast::<Node>() != child) { | ||
return Err(Error::HierarchyRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: indentation changed.
@@ -19,6 +19,8 @@ | |||
|
|||
#![deny(unsafe_code)] | |||
#![allow(non_snake_case)] | |||
// TODO: dominotree remove this | |||
#![allow(unrooted_must_root)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
@nox I hadn't reverted that commit yet and I just sat down to do it, should I go ahead and do it now? Or should I add your changes here that are applicable and then rebase? |
Yes, but I want it removed, not reverted as in what |
Okay, the commit is removed. |
Ok! Now all my comments appear on the Files tab, address them and I'll make another review pass. Thanks for your contribution and keeping up with it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The iterators are still broken and an additional nit.
@@ -91,7 +91,7 @@ pub trait LayoutNode: Debug + GetLayoutData + TNode { | |||
current: self.last_child(), | |||
}) | |||
} | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: broken indentation.
@@ -221,7 +221,7 @@ impl HTMLCollection { | |||
pub fn elements_iter_after<'a>(&'a self, after: &'a Node) -> impl Iterator<Item=Root<Element>> + 'a { | |||
// Iterate forwards from a node. | |||
HTMLCollectionElementsIter { | |||
node_iter: after.following_nodes(&self.root), | |||
node_iter: after.following_nodes::<Node>(&self.root), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type annotation is useless, given node_iter
must be an iterator that yields Root<Node>
values.
@@ -235,7 +235,7 @@ impl HTMLCollection { | |||
pub fn elements_iter_before<'a>(&'a self, before: &'a Node) -> impl Iterator<Item=Root<Element>> + 'a { | |||
// Iterate backwards from a node. | |||
HTMLCollectionElementsIter { | |||
node_iter: before.preceding_nodes(&self.root), | |||
node_iter: before.preceding_nodes::<Node>(&self.root), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type annotation is useless, given node_iter
must be an iterator that yields Root<Node>
values.
@@ -138,7 +138,7 @@ impl HTMLTableElement { | |||
fn get_rows(&self) -> TableRowFilter { | |||
TableRowFilter { | |||
sections: self.upcast::<Node>() | |||
.children() | |||
.children::<Node>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to simplify this one.
components/script/dom/node.rs
Outdated
if let Some(first_child) = Root::downcast::<T>(first_child) { | ||
self.current = Some(Root::upcast::<Node>(first_child.clone())); | ||
return Some(first_child); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you are trying to do by repeating the loop, and current
is still never yielded again.
@nox As far as the iterators go, I was working based on the proof-of-concept linked to in #15270 and the POC iterators seemed to work properly in some brief testing. Would a better approach be to simply attempt to upcast/downcast the values which would be returned from the existing iterators as they exist in the This is really the part where I'm having trouble figuring out what the correct approach is. |
@nox updated those methods, and it's still exhibiting the same behavior. I'll try to dig into it some more today - let me know if anything jumps out at you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found the issue in your current code.
components/script/dom/node.rs
Outdated
pub fn inclusively_following_siblings<T>(&self) -> SiblingIterator<T> | ||
where T: DerivedFrom<Node> | ||
{ | ||
SiblingIterator::<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the type annotation isn't required here.
components/script/dom/node.rs
Outdated
where T: DerivedFrom<Node> | ||
{ | ||
SiblingIterator::<T> { | ||
current: Root::downcast::<T>(Root::from_ref(self)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, the node following self
may be a T
, but if self
is not a T
, current
will be set to None
.
You probably want:
SiblingIterator {
current: Root::downcast(Root::from_ref(self)).or_else(|| get_next_sibling(self)),
phantom: PhantomData,
}
components/script/dom/node.rs
Outdated
where T: DerivedFrom<Node> | ||
{ | ||
ReverseSiblingIterator::<T> { | ||
current: Root::downcast::<T>(Root::from_ref(self)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark here:
Root::downcast(Root::from_ref(self)).or_else(|| get_previous_sibling(self))
@nox Thanks, that makes sense - I made those changes and was able to debug a bit more. On servo.org, when it's inserting the element for The code in that method is simply
|
The document element should be the first child of the Document. |
I suppose I could've scrolled up until I got to |
☔ The latest upstream changes (presumably #15904) made this pull request unmergeable. Please resolve the merge conflicts. |
where T: DerivedFrom<Node> { | ||
let mut current = Root::from_ref(start); | ||
//Should while loop here should probably be an if block | ||
//or should we go through all the parents? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
where T: DerivedFrom<Node> | ||
{ | ||
SiblingIterator { | ||
current: Root::downcast(Root::from_ref(self)).or_else(||get_next_sibling(self)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: space after ||
.
ReverseSiblingIterator { | ||
current: Some(Root::from_ref(self)), | ||
current: Root::downcast(Root::from_ref(self)).or_else(||get_previous_sibling(self)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: space after ||
.
current: Some(Root::from_ref(self)), | ||
pub fn following_nodes<T>(&self, root: &Node) -> FollowingIterator<T> where T: DerivedFrom<Node> { | ||
FollowingIterator::<T> { | ||
current: Root::downcast::<T>(Root::from_ref(self)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct.
current: Some(Root::from_ref(self)), | ||
pub fn preceding_nodes<T>(&self, root: &Node) -> PrecedingIterator<T> where T: DerivedFrom<Node> { | ||
PrecedingIterator::<T> { | ||
current: Root::downcast::<T>(Root::from_ref(self)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct.
@@ -729,15 +743,17 @@ impl Node { | |||
Ok(NodeList::new_simple_list(&window, iter)) | |||
} | |||
|
|||
pub fn ancestors(&self) -> AncestorIterator { | |||
pub fn ancestors(&self) -> AncestorIterator<Node> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this one parameterised?
} | ||
} | ||
|
||
pub fn inclusive_ancestors(&self) -> AncestorIterator { | ||
pub fn inclusive_ancestors(&self) -> AncestorIterator<Node> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this one parameterised?
ReverseSiblingIterator { | ||
current: self.GetLastChild(), | ||
current: Root::downcast(Root::from_ref(self)).or_else(||get_last_child(self)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: space after ||
.
fn get_first_child<T>(start: &Node) -> Option<Root<T>> | ||
where T: DerivedFrom<Node> { | ||
let mut current = Root::from_ref(start); | ||
//The while loop here should probably just be an if block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
fn get_last_child<T>(start: &Node) -> Option<Root<T>> | ||
where T: DerivedFrom<Node> { | ||
let mut current = Root::from_ref(start); | ||
//The while loop here should probably just be an if block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
☔ The latest upstream changes (presumably #16521) made this pull request unmergeable. Please resolve the merge conflicts. |
Is this PR wanted anymore? |
@KiChjang Not sure what to do at this point - when I opened this PR, there were some DOM parsing issues that had been introduced by my changes. After fixing nits and trying a few different approaches (as well as several rounds of updates to make things merge cleanly), the same DOM parsing issues still exist, and I have been unable to figure out how to resolve them. I'd be glad to continue putting effort into this PR, but until the fundamental DOM issues can be resolved, there doesn't seem to be much value here, and I'm really not sure how to move forward. |
Think this is getting pretty close now.
Made some ugly edits to try and ensure exact functionality is preserved with iterators. Once I identify the issue I'm seeing, I'll clean them up.
I've also temporarily made all of the places calling iterators with the new type parameters only use
Node
orElement
- this way they should have identical functionality as they did before, which should make debugging the iterators themselves easier.Once the iterators are done, we can clean up where they're being called from as well.
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is