Make NodeList iterable

[jdm]

Blocked on #12819. We're missing iterable<Node?>; from NodeList.webidl. I believe that adding it should be all that's necessary to make iteration over NodeList values like the result of document.querySelectorAll.

Code: components/script/dom/webidls/NodeList.webidl, components/script/dom/nodelist.rs
Test: create a test in tests/wpt/web-platform-tests/dom/nodes that tests the properties of the iterating over a NodeList

[hsinewu]

I would like to work on this.

[KiChjang]

Sure thing! Here are a couple of key things to look out for: dom::bindings::iterable::Iterable and its trait methods, get_iterable_length, get_value_at_index and get_key_at_index.

[jdm]

No, those are only used for pair iterables; the value iterables like this one are less complex.

[hsinewu]

No idea what webidl is doing there Orz

What does the question mark mean? nullable?
Looks like I could just uncomment that line (iterable), as it matches the spec definition.
But @jdm refer to it as iterable<Node?>, what's the difference?

The feeling of reading specs...

[jdm]

The ? does mean nullable, yes. I copied the line from Firefox's WebIDL, but we could see if the non-nullable one works.

[hsinewu]

Got
WebIDL.WebIDLError: error: Iterable type does not match indexed getter type
when using iterable<Node>,
while using iterable<Node?> can compile.

No idea what's going on, just do some recording.

[hsinewu]

How can I prove nodes to be iterable? for-loop? Any specific dom function to test out?
I don't know what is not working before this change.

[hsinewu]

Oh, querySelectorAll

[izgzhen]

You may take #12998 as a reference

[izgzhen]

And continue my last advice, the tests/wpt/metadata/fetch/api/headers/headers-structure.html might be a good example of testing iterable

[hsinewu]

@izgzhen I found no html files in that commit, it's some rust and .html.ini files, or do you mean those impl are the feature needed to be tested, and can served as hints to do corresponding test in javascript?

Also, I can't find headers-structure.html file you refer to, or is it buried in some other branch?

[izgzhen]

This one https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/fetch/api/headers/headers-structure.html

[hsinewu]

Oh, I was looking for it under metadata folder, it was my wrong :/

[izgzhen]

@jdm As far as I find, we don't codegen anything for value iterator? But looks like values and entries interfaces are specified in spec?

[jdm]

They are generated, but they're self-hosted JS code so no changes are necessary in the rust implementation.

[hsinewu]

@jdm I don't understand what you mean by 'self-hosted', would you clarify it for me?

I assume, by iterable it means it has forEach, vlalues, entries, keys methods, but there were not in my experiment, which means either

1. Have to use dom::bindings::iterable::Iterable; and impls.
2. Missing some use dom::bindings::codegen::Bindings::

Is this the right way to do it?

Also, would you take a look at #13081?

[jdm]

Here, 'self-hosted' means that a custom implementation of forEach, values, etc. for NodeList isn't necessary and we delegate to SpiderMonkey's default instead: https://github.com/servo/mozjs/blob/master/mozjs/js/src/builtin/Array.js#L228

[jdm]

@hsinewu I added the iterable<Node?> line to NodeList.webidl, recompiled, then loaded this HTML file:

<span>hi</span>
<script>
var o = document.querySelectorAll('span');
console.log('length' in o);
console.log('forEach' in o);
console.log('values' in o);
console.log('keys' in o);
console.log(Symbol.iterator in o);
</script>

Every line printed true for me, so I'm not sure why your NodeList isn't working...

[hsinewu]

@jdm Fail to reproduce the problem, I might have been messed up with the build.
It looks well now, thanks for your help :)