Implement NodeIterator #5981

Jinwoo-Song · 2015-05-08T06:28:00Z

Implement NodeIterator's basic functionality. (Fixes #1235) But the cases for node removals are not implemented yet.

highfive · 2015-05-08T06:28:04Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @glennw (or someone else) soon.

hoppipolla-critic-bot · 2015-05-08T06:28:07Z

Critic review: https://critic.hoppipolla.co.uk/r/4935

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

nox · 2015-05-08T11:13:26Z

components/script/dom/nodeiterator.rs

-    reflector_: Reflector
+    reflector_: Reflector,
+    root_node: JS<Node>,
+    reference_node: MutNullableHeap<JS<Node>>,


AFAICT you never set it to None, so MutHeap would be enough here.

larsbergstrom · 2015-05-08T19:36:50Z

@jdm @Ms2ger Will one of the two of you be reviewing this?

nox · 2015-05-14T12:14:54Z

^{components/script/dom/nodeiterator.rs, line 308 [r4] (raw file):}
This can be confusing given we have dom::bindings::js::JS.

^{tests/wpt/metadata/dom/interfaces.html.ini, line 444 [r4] (raw file):}
If #5902 gets merged before this PR, this test is going to pass.

^{tests/wpt/metadata/dom/traversal/NodeIterator.html.ini, line 5 [r4] (raw file):}
Why isn't that test passing?

Comments from the review on Reviewable.io

nox · 2015-05-14T12:27:08Z

^{components/script/dom/nodeiterator.rs, line 97 [r4] (raw file):}
No need for a helper if it is already a DOM method, remove prev_node().

^{components/script/dom/nodeiterator.rs, line 102 [r4] (raw file):}
Idem, remove next_node().

^{components/script/dom/nodeiterator.rs, line 118 [r4] (raw file):}
It is a bit confusing that the methods in the trait and the impl aren't in the same order.

^{components/script/dom/nodeiterator.rs, line 121 [r4] (raw file):}
To clean this helper and first_preceding_node_not_preceding_root, I'm pretty sure we can instead tweak TreeIterator in node.rs to take both a root and a current node.

^{components/script/dom/nodeiterator.rs, line 194 [r4] (raw file):}
Just write "Step 1.", "Step 2.", etc like in other files.

^{components/script/dom/nodeiterator.rs, line 234 [r4] (raw file):}
Link to spec.

^{components/script/dom/nodeiterator.rs, line 235 [r4] (raw file):}
Step 1.

^{components/script/dom/nodeiterator.rs, line 236 [r4] (raw file):}
Root this right there.

^{components/script/dom/nodeiterator.rs, line 250 [r4] (raw file):}
Why do you need the ref forever?

^{components/script/dom/nodeiterator.rs, line 269 [r4] (raw file):}
Why do you need the ref forever?

^{components/script/dom/nodeiterator.rs, line 282 [r4] (raw file):}
Why do you need the ref forever?

Comments from the review on Reviewable.io

jdm · 2015-05-14T14:23:45Z

-S-awaiting-review +S-needs-code-changes
@nox reviewed this with my blessing. Thanks for implementing this, @Jinwoo-Song!

Reviewed files:

components/script/dom/document.rs @ r3
components/script/dom/nodeiterator.rs @ r1, r2, r3, r4
components/script/dom/webidls/Document.webidl @ r3
components/script/dom/webidls/NodeIterator.webidl @ r1, r2, r4
tests/wpt/metadata/dom/interfaces.html.ini @ r4
tests/wpt/metadata/dom/traversal/NodeIterator.html.ini @ r4

^{components/script/dom/nodeiterator.rs, line 25 [r1] (raw file):}
nit: indent these to match root_node

^{components/script/dom/nodeiterator.rs, line 91 [r4] (raw file):}
Add a spec link.

^{components/script/dom/nodeiterator.rs, line 104 [r2] (raw file):}
Let's just move the implementation of these into NextNode and PrevNode.

Comments from the review on Reviewable.io

bors-servo · 2015-05-14T14:43:46Z

☔ The latest upstream changes (presumably #6037) made this pull request unmergeable. Please resolve the merge conflicts.

Jinwoo-Song · 2015-05-21T08:13:59Z

Review status: all files reviewed, 18 unresolved discussions, some commit checks failed.

^{components/script/dom/nodeiterator.rs, line 24 [r4] (raw file):}
Done. Yes, latest spec says referenceNode cannot be NULL.

^{components/script/dom/nodeiterator.rs, line 33 [r1] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 91 [r4] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 97 [r4] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 102 [r4] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 118 [r4] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 121 [r4] (raw file):}
Done. I added following_node() and preceding_nod() in node.rs and used them in nodeiterator.rs

^{components/script/dom/nodeiterator.rs, line 194 [r4] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 219 [r2] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 234 [r4] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 235 [r4] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 236 [r4] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 250 [r4] (raw file):}
Done. Fixed the code not to use ref forever.

^{components/script/dom/nodeiterator.rs, line 269 [r4] (raw file):}
Done. Fixed the code not to use ref forever.

^{components/script/dom/nodeiterator.rs, line 282 [r4] (raw file):}
Done. Fixed the code not to use ref forever.

^{components/script/dom/nodeiterator.rs, line 308 [r4] (raw file):}
Would you suggest a candidate? :)

^{tests/wpt/metadata/dom/interfaces.html.ini, line 444 [r4] (raw file):}
Great!

^{tests/wpt/metadata/dom/traversal/NodeIterator.html.ini, line 5 [r4] (raw file):}
Actually, I have no idea for this why this fails. Could you give me some advice?

Comments from the review on Reviewable.io

nox · 2015-05-25T21:06:56Z

Review status: 1 of 7 files reviewed, 10 unresolved discussions, all commit checks successful.

^{components/script/dom/document.rs, line 1318 [r5] (raw file):}
https://dom.spec.whatwg.org/#dom-document-createnodeiteratorroot-whattoshow-filter

^{components/script/dom/node.rs, line 1306 [r5] (raw file):}
As a follow-up ticket to file, or if you want to do it right now, TreeIterator and FollowingRootIterator can be merged. Their only difference is that I stored the current depth instead of the root in the former.

Otherwise that's exactly what I had in mind.

^{components/script/dom/nodeiterator.rs, line 98 [r4] (raw file):}
Mmmh, that's a bit similar isn't? :) The point was to remove helper methods when they are used in a proper DOM method as-is. No need to have extra layers of indirections.

^{components/script/dom/nodeiterator.rs, line 103 [r5] (raw file):}
Ditto.

^{components/script/dom/nodeiterator.rs, line 157 [r5] (raw file):}
if result == NodeFilterConstants::FILTER_ACCEPT { … }

^{components/script/dom/nodeiterator.rs, line 247 [r4] (raw file):}
I don't have any good candidate. :s

Callback? Interpreted? NotNative? :P

^{tests/wpt/metadata/dom/interfaces.html.ini, line 357 [r4] (raw file):}
In fact, it won't, I was wrong I fixed other lengths but not this one. Oh well. :)

^{tests/wpt/metadata/dom/traversal/NodeIterator.html.ini, line 5 [r4] (raw file):}
I'll look into it.

Comments from the review on Reviewable.io

nox · 2015-05-25T23:29:25Z

Review status: 1 of 7 files reviewed, 11 unresolved discussions, all commit checks successful.

^{tests/wpt/metadata/dom/traversal/NodeIterator.html.ini, line 5 [r4] (raw file):}
So I've looked into it, and apparently it comes from callback::CallSetup::drop() not taking into account the CallSetup's exception handling mode. It shouldn't report the exception if the value is ExceptionHandling::Rethrow. You can either fix it or file a follow-up issue and I'll take care of it.

Comments from the review on Reviewable.io

nox · 2015-05-25T23:33:46Z

Review status: 1 of 7 files reviewed, 14 unresolved discussions, all commit checks successful.

^{components/script/dom/node.rs, line 790 [r5] (raw file):}
following_nodes

^{components/script/dom/node.rs, line 797 [r5] (raw file):}
preceding_nodes

^{components/script/dom/node.rs, line 804 [r5] (raw file):}
descending_last_children? I find that name confusing but I can't find a better one right now.

Comments from the review on Reviewable.io

nox · 2015-05-25T23:35:18Z

Review status: 2 of 7 files reviewed, 14 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/node.rs @ r5

Comments from the review on Reviewable.io

Jinwoo-Song · 2015-05-27T07:33:26Z

Review status: 2 of 7 files reviewed, 14 unresolved discussions, all commit checks successful.

^{components/script/dom/document.rs, line 1318 [r5] (raw file):}
Done.

^{components/script/dom/node.rs, line 790 [r5] (raw file):}
Done.

^{components/script/dom/node.rs, line 797 [r5] (raw file):}
Done.

^{components/script/dom/node.rs, line 804 [r5] (raw file):}
Done.

^{components/script/dom/node.rs, line 1306 [r5] (raw file):}
I'll follow up your advice in next patch. :)

^{components/script/dom/nodeiterator.rs, line 98 [r4] (raw file):}
Okay, I removed the helper function and moved the implementation in here.

^{components/script/dom/nodeiterator.rs, line 103 [r5] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 157 [r5] (raw file):}
Done.

^{components/script/dom/nodeiterator.rs, line 247 [r4] (raw file):}
I vote to 'Callback'. :)

^{tests/wpt/metadata/dom/traversal/NodeIterator.html.ini, line 5 [r4] (raw file):}
I'll raise another issue for this, thanks!

Comments from the review on Reviewable.io


        Implement NodeIterator's attributes

- root, whatToShow, and filter


        Implement more NodeIterator attribute and methods

Just implement the skelectons - referenceNode attribute - nextNode() and previousNode()


        Implement Document.createNodeIterator and rebase patch


        Implement nextNode(), previousNode() and detach() in NodeIterator

First patch to resolve #1235. We need more implementation for handling node removals, and I'll implement in next patch.

nox · 2015-05-27T07:38:54Z

Review status: 2 of 7 files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/node.rs @ r6

Comments from the review on Reviewable.io

Jinwoo-Song · 2015-05-27T08:09:50Z

Review status: 2 of 7 files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/node.rs @ r6

^{tests/wpt/metadata/dom/interfaces.html.ini, line 444 [r4] (raw file):}
It seems that all other tests for 'object length' fail. But I do not have idea for this right now.

Comments from the review on Reviewable.io

Manishearth · 2015-05-27T18:49:54Z

Review status: 5 of 7 files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/webidls/Document.webidl @ r5
components/script/dom/webidls/NodeIterator.webidl @ r5
tests/wpt/metadata/dom/interfaces.html.ini @ r5

^{tests/wpt/metadata/dom/interfaces.html.ini, line 444 [r4] (raw file):}
Try rebasing again and testing

Comments from the review on Reviewable.io

nox · 2015-05-27T21:00:55Z

Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/document.rs @ r6
components/script/dom/nodeiterator.rs @ r6
components/script/dom/webidls/Document.webidl @ r5
components/script/dom/webidls/NodeIterator.webidl @ r5
tests/wpt/metadata/dom/interfaces.html.ini @ r5
tests/wpt/metadata/dom/traversal/NodeIterator.html.ini @ r4

^{tests/wpt/metadata/dom/interfaces.html.ini, line 357 [r4] (raw file):}
@Manisheart Nah I was saying I thought I fixed them but I didn't, so even if he rebases nothing will change. I just made a remark for nothing.

Comments from the review on Reviewable.io

Manishearth · 2015-05-28T01:18:02Z

@nox Is there anything left to resolve? Or should I r+?

nox · 2015-05-28T08:30:05Z

@Manishearth No, LGTM. :)

Manishearth · 2015-05-28T08:34:34Z

@bors-servo: r+

bors-servo · 2015-05-28T08:34:34Z

📌 Commit 6d570e9 has been approved by Manishearth

bors-servo · 2015-05-28T08:34:37Z

⌛ Testing commit 6d570e9 with merge 06e7b51...


        Auto merge of #5981 - Jinwoo-Song:nodeiterator, r=Manishearth

@jdm

Implement NodeIterator's basic functionality. (Fixes #1235) But the cases for node removals are not implemented yet. r? @jdm cc @yichoi  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5981)

bors-servo · 2015-05-28T09:05:08Z

💔 Test failed - mac1

nox · 2015-05-28T09:06:18Z

@Jinwoo-Song

/html/dom/interfaces.html
-------------------------
PASS expected FAIL Document interface: document.implementation.createDocument(null, "", null) must inherit property "createNodeIterator" with the proper type (25)
PASS expected FAIL Document interface: calling createNodeIterator(Node,unsigned long,NodeFilter) on document.implementation.createDocument(null, "", null) with too few arguments must throw TypeError

Almost done! :)


        Update NodeIterator code to apply review comments.

Jinwoo-Song · 2015-05-28T09:43:08Z

Updated test result!

nox · 2015-05-28T09:44:19Z

Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/document.rs @ r7
components/script/dom/node.rs @ r7
tests/wpt/metadata/html/dom/interfaces.html.ini @ r7

Comments from the review on Reviewable.io

Manishearth · 2015-05-28T10:14:06Z

@bors-servo: r+

bors-servo · 2015-05-28T10:14:06Z

📌 Commit db25be7 has been approved by Manishearth

bors-servo · 2015-05-28T10:14:08Z

⌛ Testing commit db25be7 with merge 2b52006...


        Auto merge of #5981 - Jinwoo-Song:nodeiterator, r=Manishearth

@jdm

Implement NodeIterator's basic functionality. (Fixes #1235) But the cases for node removals are not implemented yet. r? @jdm cc @yichoi  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5981)

bors-servo · 2015-05-28T10:54:07Z

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

highfive added the S-awaiting-review label May 8, 2015

Jinwoo-Song force-pushed the Jinwoo-Song:nodeiterator branch 2 times, most recently from 336486a to 22983ff May 8, 2015

nox reviewed May 8, 2015
View changes

jdm self-assigned this May 8, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels May 14, 2015

jdm added the S-needs-rebase label May 14, 2015

Jinwoo-Song force-pushed the Jinwoo-Song:nodeiterator branch from 22983ff to e0726fe May 21, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels May 21, 2015

jdm removed the S-needs-rebase label May 21, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels May 25, 2015

Jinwoo-Song force-pushed the Jinwoo-Song:nodeiterator branch from e0726fe to 6d570e9 May 27, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels May 27, 2015

Jinwoo-Song added 4 commits Apr 27, 2015

Implement NodeIterator's attributes

2701c26

- root, whatToShow, and filter

Implement more NodeIterator attribute and methods

0e4c792

Just implement the skelectons - referenceNode attribute - nextNode() and previousNode()

Implement Document.createNodeIterator and rebase patch

4e8005c

Implement nextNode(), previousNode() and detach() in NodeIterator

cb2bdf1

First patch to resolve #1235. We need more implementation for handling node removals, and I'll implement in next patch.

Update NodeIterator code to apply review comments.

Loading status checks…

db25be7

Jinwoo-Song force-pushed the Jinwoo-Song:nodeiterator branch from 6d570e9 to db25be7 May 28, 2015

bors-servo merged commit db25be7 into servo:master May 28, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

Implement NodeIterator #5981

Implement NodeIterator #5981

Jinwoo-Song commented May 8, 2015

highfive commented May 8, 2015

hoppipolla-critic-bot commented May 8, 2015

This comment has been minimized.

larsbergstrom commented May 8, 2015

nox commented May 14, 2015

nox commented May 14, 2015

jdm commented May 14, 2015

bors-servo commented May 14, 2015

Jinwoo-Song commented May 21, 2015

nox commented May 25, 2015

nox commented May 25, 2015

nox commented May 25, 2015

nox commented May 25, 2015

Jinwoo-Song commented May 27, 2015

nox commented May 27, 2015

Jinwoo-Song commented May 27, 2015

Manishearth commented May 27, 2015

nox commented May 27, 2015

Manishearth commented May 28, 2015

nox commented May 28, 2015

Manishearth commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

nox commented May 28, 2015

Jinwoo-Song commented May 28, 2015

nox commented May 28, 2015

Manishearth commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

servo / servo

Join GitHub today

Implement NodeIterator #5981

Implement NodeIterator #5981

Conversation

Jinwoo-Song commented May 8, 2015

highfive commented May 8, 2015

hoppipolla-critic-bot commented May 8, 2015

This comment has been minimized.

nox May 8, 2015

larsbergstrom commented May 8, 2015

nox commented May 14, 2015

nox commented May 14, 2015

jdm commented May 14, 2015

bors-servo commented May 14, 2015

Jinwoo-Song commented May 21, 2015

nox commented May 25, 2015

nox commented May 25, 2015

nox commented May 25, 2015

nox commented May 25, 2015

Jinwoo-Song commented May 27, 2015

nox commented May 27, 2015

Jinwoo-Song commented May 27, 2015

Manishearth commented May 27, 2015

nox commented May 27, 2015

Manishearth commented May 28, 2015

nox commented May 28, 2015

Manishearth commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

nox commented May 28, 2015

Jinwoo-Song commented May 28, 2015

nox commented May 28, 2015

Manishearth commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015