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
Member

frewsxcv commented Jul 28, 2015

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

@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
Member Author

frewsxcv commented Jul 28, 2015

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
Member Author

frewsxcv commented Jul 28, 2015

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 frewsxcv:childnode-before-childnode-after branch from ae2ca78 to e785a4b Jul 28, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 28, 2015

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
Member Author

frewsxcv commented Jul 28, 2015

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

@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 29, 2015

@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 29, 2015

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.
@frewsxcv frewsxcv force-pushed the frewsxcv:childnode-before-childnode-after branch from e785a4b to 8cfccda Jul 29, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 29, 2015

The latest force push addresses the second round of comments


Comments from the review on Reviewable.io

@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

bors-servo commented Jul 30, 2015

📌 Commit 8cfccda has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2015

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

bors-servo commented Jul 30, 2015

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

@bors-servo bors-servo merged commit 8cfccda into servo:master Jul 30, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv frewsxcv deleted the frewsxcv:childnode-before-childnode-after branch Aug 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.