make nodeList iterable #13081

hsinewu · 2016-08-27T11:21:33Z

Not sure what to test in the script.
Need some advice. o_O

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13021 (github issue number if applicable).

There are tests for these changes

This change is

highfive · 2016-08-27T11:21:37Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webidls/NodeList.webidl

highfive · 2016-08-27T11:21:38Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

hsinewu · 2016-08-27T11:52:48Z

Well...
I can comment out iterable<Node?>, rebuild, and still pass the test...
So looks like the test script is quite pointless.
What should I do?

izgzhen · 2016-08-27T11:58:54Z

tests/wpt/metadata/MANIFEST.json

+            "path": "dom/nodes/NodeList-Iterable.html",
+            "url": "/dom/nodes/NodeList-Iterable.html"
+          }
+        ],


Looks like you hand-write this? Try ./mach update-manifest for adding test files

@izgzhen I'm not bold enough to modify manifest file :p
I was using ./mach create-wpt tests/.../foo.html, as recommended here
I'll check out your advice too.

Oh, do you mean the inconsistency of the path?
I have no idea about that :)

Never mind :) it passed the CI so it is good. I am just surprised that the new entry it the first one in items

izgzhen · 2016-08-27T12:06:00Z

BTW, consider adding a descriptive title to the testing HTML file like "NodeList Iterable Test".

Good luck!

Ms2ger · 2016-08-27T13:10:37Z

You can test for the presence of the Symbol.iterator property and whether for/of loops work

Ms2ger · 2016-08-27T13:10:55Z

(sorry, fat fingers)

hsinewu · 2016-08-27T14:07:30Z

@Ms2ger I haven't heard of that Symbol.iterator thing, I'll study about it!

hsinewu · 2016-08-28T03:18:06Z

This test makes more sense to me.
But still, it passes even with webidl unchanged, and that's not what I expected.

hsinewu · 2016-08-28T03:23:42Z

I'm thinking of ways to create NodeList without using function like querySelectorAll, but looks like it always involves using some sort of dom function.

There is a NodeList constructor(I test it on firefox) but I can't construct it with new operator.
Any ideas?

hsinewu · 2016-08-28T03:25:52Z

tests/wpt/web-platform-tests/dom/nodes/NodeList-Iterable.html

+		test(function() {
+			for(var method of methods)
+				assert_true(method in nodes);
+		}, "NodeList has " /*+ methods.join()*/ + " methods.");


for some reason when I invoke join method, the test always timeout, reporting find no tests.

KiChjang · 2016-08-28T05:38:24Z

It doesn't look like anything's changed in nodelist.rs, did you not commit changes in that file?

KiChjang · 2016-08-28T05:38:54Z

tests/wpt/web-platform-tests/dom/nodes/NodeList-Iterable.html

+		nothing = document.querySelectorAll('span');
+	})
+	testIterable(paragraphs);
+	testIterable(nothing);


Also, this test is full of tabs, please change them into spaces instead.

hsinewu · 2016-08-28T05:44:23Z

@KiChjang So there should be a impl Iterable for NodeList{ ... } in nodelist.rs ?

hsinewu · 2016-08-28T05:58:24Z

I'm not sure, I thought @jdm means changing webidl and it woiuld be done, per the ticket.
Am I supposed to work on nodelist.rs?

izgzhen · 2016-08-28T09:58:27Z

Okay ... looks like the basic indexing is implemented before, but except for Symbol.iterator (which is automatically generated if you uncomment the line in WebIDL, I guess), you may want to test the existence of forEach method, entries property and values property. You can check the below links for more information:

As I tested locally, these forEach etc. are not supported now, and nodelist.rs must implement Iterable to support it. Note that the headers example I gave previously is a pair iterator, but NodeList will be a value iterator.

izgzhen · 2016-08-28T10:44:34Z

No, don't care about forEach etc. ... Looks like I get it wrong (from WebIDL.py):

        # We only need to add entries/keys/values here if we're a pair iterator.
        # Value iterators just copy these from %ArrayPrototype% instead.
        if not self.isPairIterator():
            return

But looks like even if I didn't uncomment this line (on my local version), the querySelectorAll still works.

hsinewu · 2016-08-29T03:56:10Z

@izgzhen Yes, there is no forEach, entries, values methods associated with it.
Unfortunately, Symbol.iterator IS defined in the build without iterable<Node?>.
Maybe this is how things supposed to be?

hsinewu · 2016-08-29T05:20:18Z

I just realize I could have look it up elsewhere.
Refer to this test, they use this block of code to test the behavior of childNodes, which is of type NodeList.

... (scroll to end of the page)
  assert_equals(list[Symbol.iterator], Array.prototype[Symbol.iterator]);
  assert_equals(list.keys, Array.prototype.keys);
  assert_equals(list.values, Array.prototype.values);
  assert_equals(list.entries, Array.prototype.entries);
  assert_equals(list.forEach, Array.prototype.forEach);
}, "Iterator behavior of Node.childNodes");

Is this test script applicable in our case?

izgzhen · 2016-08-29T08:21:36Z

I think you are right. Only Symbol.iterator is "built-in", and other four need to be generated automagically through uncommenting out the line in IDL.

hsinewu · 2016-08-29T12:07:19Z

But they are NOT there with that line uncommented, in my experiment?
I'm confused.
What is the missing piece? The iterable implementation?

Actually, I have no idea how declaration in IDL affect the build.

bors-servo · 2016-08-30T19:15:21Z

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

hsinewu · 2016-09-03T10:37:53Z

@KiChjang need reviewing :3

KiChjang · 2016-09-03T23:46:52Z

tests/wpt/web-platform-tests/dom/nodes/NodeList-Iterable.html

+<script>
+    var paragraphs;
+    setup(function() {
+        paragraphs = document.querySelectorAll('p');


I don't think querySelectorAll is implemented on the document, and I'm not sure whether there's work going towards implementing CSS Selectors DOM interface.

Pinging @SimonSapin to see if we'd want to work on that.

However, getElementsByName is implemented, so instead of depending on querySelectorsAll, you may want to use the former instead.

Easily verified: https://dxr.mozilla.org/servo/source/components/script/dom/document.rs#2641

Weird, I wasn't able to find it in the WebIDL file...

KiChjang · 2016-09-04T00:00:52Z

The test doesn't look like it properly tests for the iterable properties of a NodeList - it only tests for the presence of methods to call in NodeList. As @Ms2ger had described, testing for the existence of the Symbol.iterator property and using a for/of iterator would properly verify that NodeList does have iterable elements.


        Make NodeList iterable

Ms2ger · 2016-09-05T14:45:22Z

@bors-servo r+

bors-servo · 2016-09-05T14:45:23Z

📌 Commit b4ac513 has been approved by Ms2ger

bors-servo · 2016-09-05T14:45:25Z

⌛ Testing commit b4ac513 with merge 1901a21...


        Auto merge of #13081 - hsinewu:nodelist-iterable, r=Ms2ger

make nodeList iterable  Not sure what to test in the script. Need some advice. o_O ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13021 (github issue number if applicable).  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13081)

bors-servo · 2016-09-05T15:41:59Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned KiChjang Aug 27, 2016

highfive added the S-awaiting-review label Aug 27, 2016

izgzhen reviewed Aug 27, 2016
View changes

Ms2ger closed this Aug 27, 2016

Ms2ger reopened this Aug 27, 2016

hsinewu reviewed Aug 28, 2016
View changes

KiChjang reviewed Aug 28, 2016
View changes

izgzhen mentioned this pull request Aug 28, 2016

Make DOMTokenList iterable #13019

Closed

highfive added the S-needs-rebase label Aug 30, 2016

hsinewu mentioned this pull request Sep 1, 2016

Make NodeList iterable #13021

Closed

KiChjang removed the S-needs-rebase label Sep 3, 2016

KiChjang reviewed Sep 3, 2016
View changes

KiChjang added S-awaiting-answer S-needs-code-changes and removed S-awaiting-review S-awaiting-answer labels Sep 3, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 5, 2016

Make NodeList iterable

Loading status checks…

b4ac513

highfive assigned Ms2ger and unassigned KiChjang Sep 5, 2016

highfive removed the S-awaiting-review label Sep 5, 2016

highfive added the S-awaiting-merge label Sep 5, 2016

bors-servo merged commit b4ac513 into servo:master Sep 5, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 5, 2016

servo / servo

make nodeList iterable #13081

make nodeList iterable #13081

hsinewu commented Aug 27, 2016 •

edited

highfive commented Aug 27, 2016

highfive commented Aug 27, 2016

hsinewu commented Aug 27, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

izgzhen commented Aug 27, 2016 •

edited

Ms2ger commented Aug 27, 2016

Ms2ger commented Aug 27, 2016

hsinewu commented Aug 27, 2016

hsinewu commented Aug 28, 2016

hsinewu commented Aug 28, 2016

This comment has been minimized.

KiChjang commented Aug 28, 2016

This comment has been minimized.

This comment has been minimized.

hsinewu commented Aug 28, 2016 •

edited

hsinewu commented Aug 28, 2016 •

edited

izgzhen commented Aug 28, 2016

izgzhen commented Aug 28, 2016

hsinewu commented Aug 29, 2016 •

edited

hsinewu commented Aug 29, 2016

izgzhen commented Aug 29, 2016

hsinewu commented Aug 29, 2016 •

edited

bors-servo commented Aug 30, 2016

hsinewu commented Sep 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Sep 4, 2016 •

edited

Ms2ger commented Sep 5, 2016

bors-servo commented Sep 5, 2016

bors-servo commented Sep 5, 2016

bors-servo commented Sep 5, 2016

servo / servo

Join GitHub today

make nodeList iterable #13081

make nodeList iterable #13081

Conversation

hsinewu commented Aug 27, 2016 • edited

highfive commented Aug 27, 2016

highfive commented Aug 27, 2016

hsinewu commented Aug 27, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

izgzhen commented Aug 27, 2016 • edited

Ms2ger commented Aug 27, 2016

Ms2ger commented Aug 27, 2016

hsinewu commented Aug 27, 2016

hsinewu commented Aug 28, 2016

hsinewu commented Aug 28, 2016

This comment has been minimized.

KiChjang commented Aug 28, 2016

This comment has been minimized.

This comment has been minimized.

hsinewu commented Aug 28, 2016 • edited

hsinewu commented Aug 28, 2016 • edited

izgzhen commented Aug 28, 2016

izgzhen commented Aug 28, 2016

hsinewu commented Aug 29, 2016 • edited

hsinewu commented Aug 29, 2016

izgzhen commented Aug 29, 2016

hsinewu commented Aug 29, 2016 • edited

bors-servo commented Aug 30, 2016

hsinewu commented Sep 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm Sep 3, 2016 • edited

This comment has been minimized.

KiChjang commented Sep 4, 2016 • edited

Ms2ger commented Sep 5, 2016

bors-servo commented Sep 5, 2016

bors-servo commented Sep 5, 2016

bors-servo commented Sep 5, 2016

hsinewu commented Aug 27, 2016 •

edited

izgzhen commented Aug 27, 2016 •

edited

hsinewu commented Aug 28, 2016 •

edited

hsinewu commented Aug 28, 2016 •

edited

hsinewu commented Aug 29, 2016 •

edited

hsinewu commented Aug 29, 2016 •

edited

jdm Sep 3, 2016 •

edited

KiChjang commented Sep 4, 2016 •

edited