Skip to content
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

Implement ChildNode::before & ChildNode::after #6800

Merged
merged 1 commit into from
Jul 30, 2015

Conversation

frewsxcv
Copy link
Contributor

Continued from #6536

The current implementations of ChildNode::before and
ChildNode::after do not match the WHATWG spec. This commit updates the
implementations to match the spec.

Our current implementation of ChildNode::after passes all the WPT
tests. So I made sure to add a regression test that failed with the
current implementation. There are a few other unit tests I added
to exhaust other corner cases I encountered.

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 28, 2015
@Ms2ger Ms2ger added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 28, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 28, 2015

-S-awaiting-review +S-needs-code-changes


Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


components/script/dom/bindings/utils.rs, line 983 [r1] (raw file):
I think I' prefer keeping this in node.rs.


components/script/dom/bindings/utils.rs, line 987 [r1] (raw file):
How about

nodes.find(|node| {
    not_in.iter().all(|n| {
        match n {
            &NodeOrString::eNode(ref n) => *n != node,
            _ => true,
        }
    })
})

?

(I like my iterator methods.)


components/script/dom/node.rs, line 818 [r1] (raw file):
.map(|s| s.root()) should be just .root()


tests/wpt/web-platform-tests/dom/nodes/ChildNode-after.html, line 110 [r1] (raw file):
Ditto


tests/wpt/web-platform-tests/dom/nodes/ChildNode-before.html, line 110 [r1] (raw file):
"all siblings" doesn't seem to be true.


tests/wpt/web-platform-tests/dom/nodes/ChildNode-before.html, line 112 [r1] (raw file):
You seem to have forgotten

var parent = document.createElement("div")
var x = parent.appendChild(document.createElement("a"));
var y = parent.appendChild(document.createElement("b"));
var z = parent.appendChild(document.createElement("c"));
var u = parent.appendChild(document.createElement("d"));
parent.appendChild(child);
child.before(z, u);

from #6536 (comment).


Comments from the review on Reviewable.io

@frewsxcv
Copy link
Contributor Author

tests/wpt/web-platform-tests/dom/nodes/ChildNode-before.html, line 112 [r1] (raw file):
Out of curiosity, what is being tested in the codeblock you pasted that's not already being tested?


Comments from the review on Reviewable.io

@frewsxcv
Copy link
Contributor Author

components/script/dom/node.rs, line 818 [r1] (raw file):
OptionalRootable (which presumably made this method on Option possible) was removed in #6150


Comments from the review on Reviewable.io

@frewsxcv frewsxcv force-pushed the childnode-before-childnode-after branch from ae2ca78 to e785a4b Compare July 28, 2015 10:32
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 28, 2015
@frewsxcv
Copy link
Contributor Author

tests/wpt/web-platform-tests/dom/nodes/ChildNode-before.html, line 112 [r1] (raw file):
I just added it in into the latest force push


Comments from the review on Reviewable.io

@frewsxcv
Copy link
Contributor Author

Addressed all the comments in the latest force push, with exception to the OptionalRootable.


Comments from the review on Reviewable.io

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 29, 2015

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


components/script/dom/node.rs, line 818 [r1] (raw file):
Bah. File an issue to bring it back.


tests/wpt/web-platform-tests/dom/nodes/ChildNode-after.html, line 111 [r2] (raw file):
Separate test


tests/wpt/web-platform-tests/dom/nodes/ChildNode-before.html, line 111 [r2] (raw file):
Separate test?


tests/wpt/web-platform-tests/dom/nodes/ChildNode-before.html, line 126 [r1] (raw file):
The viable sibling not being the first child, iirc


Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 29, 2015
@frewsxcv
Copy link
Contributor Author

@frewsxcv
Copy link
Contributor Author

tests/wpt/web-platform-tests/dom/nodes/ChildNode-before.html, line 112 [r1] (raw file):
Ah, good point. That makes sense


Comments from the review on Reviewable.io

Continued from servo#6536

The current implementations of `ChildNode::before` and
`ChildNode::after` do not match the WHATWG spec. This commit updates the
implementations to match the spec.

Our current implementation of `ChildNode::after` passes all the WPT
tests. So I made sure to add a regression test that failed with the
current implementation. There are a few other unit tests I added
to exhaust other corner cases I encountered.
@frewsxcv frewsxcv force-pushed the childnode-before-childnode-after branch from e785a4b to 8cfccda Compare July 29, 2015 15:44
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 29, 2015
@frewsxcv
Copy link
Contributor Author

The latest force push addresses the second round of comments


Comments from the review on Reviewable.io

@Ms2ger Ms2ger added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 30, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 30, 2015

Thank you for your PR.

-S-awaiting-review +S-awaiting-merge

@bors-servo r+


Reviewed 3 of 3 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

📌 Commit 8cfccda has been approved by Ms2ger

@bors-servo
Copy link
Contributor

⌛ Testing commit 8cfccda with merge 5873a5c...

bors-servo pushed a commit that referenced this pull request Jul 30, 2015
…2ger

Implement ChildNode::before & ChildNode::after

Continued from #6536

The current implementations of `ChildNode::before` and
`ChildNode::after` do not match the WHATWG spec. This commit updates the
implementations to match the spec.

Our current implementation of `ChildNode::after` passes all the WPT
tests. So I made sure to add a regression test that failed with the
current implementation. There are a few other unit tests I added
to exhaust other corner cases I encountered.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6800)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit 8cfccda into servo:master Jul 30, 2015
@frewsxcv frewsxcv deleted the childnode-before-childnode-after branch August 17, 2015 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants