Bug 131443 – NodeList should be iterable

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

It's ridiculous that every JS library is converting the result of querySelectorAll into an array.

Since querySelectorAll returns a new NodeList each time, I don't think the backwards compatibility risk is that high.
We should make it return a JS array instead.

[Geoffrey Garen]

How would this help with the fact that library code copies out the result into a new array?

Are you proposing a spec change, which other browsers would adopt, which would then lead to library authors removing their copying code?

[Ryosuke Niwa]

Anne says he can change the spec if we successfully change our implementation.

[Ryosuke Niwa]

(In reply to comment #1)
> How would this help with the fact that library code copies out the result into a new array?
>
> Are you proposing a spec change, which other browsers would adopt, which would then lead to library authors removing their copying code?

Yes. Also JSC can optimize native JS array operations better than the one involving NodeLists although we should be teaching JSC now to access NodeList fast as well.

e.g. jQuery executes:

push.apply( results, slice.call( newContext.querySelectorAll(
    newSelector
), 0 ) );

and optimizing this code is much easier if querySelectorAll simply returned a JS array.

[Ryosuke Niwa]

Also note that queryAll in DOM4 returns Elements, which is a subclass of Array so making querySelectorAll return Array or Elements when it's introduced will align the return types of two functions:

Excerpts from http://dom.spec.whatwg.org/#collections:-elements

interface ParentNode {
  readonly attribute HTMLCollection children;
  readonly attribute Element? firstElementChild;
  readonly attribute Element? lastElementChild;
  readonly attribute unsigned long childElementCount;

  void prepend((Node or DOMString)... nodes);
  void append((Node or DOMString)... nodes);

  Element? query(DOMString relativeSelectors);
  [NewObject] Elements queryAll(DOMString relativeSelectors);
  Element? querySelector(DOMString selectors);
  [NewObject] NodeList querySelectorAll(DOMString selectors);
};
Document implements ParentNode;
DocumentFragment implements ParentNode;
Element implements ParentNode;

class Elements extends Array {
  Element? query(DOMString relativeSelectors);
  Elements queryAll(DOMString relativeSelectors);
};

[Ryosuke Niwa]

Erik (arv) from Google also says they will happily change Blink to match the new behavior if we successfully make this change on our side as well.

[Ryosuke Niwa]

I think NodeList / HTMLCollection are now iteratorable so we should be able to fix this bug with that!

[Brent Fulgham]

<rdar://problem/25731519>

[Chris Dumez]

What is left to be done here concretely?

[Ryosuke Niwa]

for (var e of document.all)
  console.log(e)

needs to work.  Someone from JSC team should be able to tell us what we need to make an object iterable.

[Chris Dumez]

(In reply to comment #9)
> for (var e of document.all)
>   console.log(e)
> 
> needs to work.  Someone from JSC team should be able to tell us what we need
> to make an object iterable.

document.all ??

[Ryosuke Niwa]

(In reply to comment #10)
> (In reply to comment #9)
> > for (var e of document.all)
> >   console.log(e)
> > 
> > needs to work.  Someone from JSC team should be able to tell us what we need
> > to make an object iterable.
> 
> document.all ??

Oh oops, this is about querySelectorAll, so it should be:

for (var e of document.querySelectorAll('*'))
  console.log(e)

[Chris Dumez]

I see, so basically we need to make NodeList iterable to match the spec:
[Exposed=Window]
interface NodeList {
  getter Node? item(unsigned long index);
  readonly attribute unsigned long length;
  iterable<Node>; // We are missing this.
};

[youenn fablet]

Created attachment 277991 [details]
WIP

[youenn fablet]

Created attachment 277995 [details]
Rebasing WIP

[Darin Adler]

Comment on attachment 277991 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=277991&action=review

> Source/WebCore/css/FontFaceSet.cpp:89
> +    return RefPtr<FontFace>(m_target->backing()[m_index++].wrapper(state));

What error do we get without the explicit RefPtr<FontFace> typecast?

> Source/WebCore/css/FontFaceSet.h:64
> +        Optional<RefPtr<FontFace>> next(JSC::ExecState&);

I’d like to know more about the type Optional<RefPtr<X>> for this? Is there a distinct null value separate from Nullopt? Can this just be RefPtr and use nullptr to mean null?

> Source/WebCore/dom/NodeList.h:34
> +};

Semicolon here is incorrect.

[Darin Adler]

Comment on attachment 277995 [details]
Rebasing WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=277995&action=review

> Source/WebCore/css/FontFaceSet.h:64
> +        Optional<RefPtr<FontFace>> next(JSC::ExecState&);

Another question about the return type: Does this need to be RefPtr instead of a raw pointer? If so, why?

[Build Bot]

Comment on attachment 277995 [details]
Rebasing WIP

Attachment 277995 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1261213

New failing tests:
fast/dom/domListEnumeration.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 277998 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277995 [details]
Rebasing WIP

Attachment 277995 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1261227

New failing tests:
fast/dom/domListEnumeration.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 278000 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277995 [details]
Rebasing WIP

Attachment 277995 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1261280

New failing tests:
fast/dom/domListEnumeration.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 278001 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277995 [details]
Rebasing WIP

Attachment 277995 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1261287

New failing tests:
fast/dom/domListEnumeration.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 278002 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[youenn fablet]

> I’d like to know more about the type Optional<RefPtr<X>> for this? Is there
> a distinct null value separate from Nullopt? Can this just be RefPtr and use
> nullptr to mean null?

I wanted to check that and will update the patch if there cannot be a null JS value in the iteration.
JSKeyValueIterator updated implementation should just require operator* and operator bool() for set iterators.

> > Source/WebCore/css/FontFaceSet.h:64
> > +        Optional<RefPtr<FontFace>> next(JSC::ExecState&);
> 
> Another question about the return type: Does this need to be RefPtr instead
> of a raw pointer? If so, why?

I seem to recall there was a need for it but do not remember that.
Probably mmaxfield would know more about this.

[Ryosuke Niwa]

Comment on attachment 277995 [details]
Rebasing WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=277995&action=review

> Source/WebCore/dom/NodeList.h:69
> +    if (m_index < m_list->length())

This is inefficient because length() can cache the entire node list / collection.
Just call item(m_index++) and check the return type instead.

[WebKit Commit Bot]

Attachment 277995 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeList.h:50:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 278080 [details]
Patch

[WebKit Commit Bot]

Attachment 278080 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeList.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 278080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278080&action=review

> Source/WebCore/dom/NodeList.h:33
> +};

Will i

[Build Bot]

Comment on attachment 278080 [details]
Patch

Attachment 278080 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1265801

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html

[Build Bot]

Created attachment 278084 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

> > Source/WebCore/css/FontFaceSet.h:64
> > +        Optional<RefPtr<FontFace>> next(JSC::ExecState&);
> 
> I’d like to know more about the type Optional<RefPtr<X>> for this? Is there
> a distinct null value separate from Nullopt? Can this just be RefPtr and use
> nullptr to mean null?

I think it can be a RefPtr<> since CSSFontFace.wrapper() is returning a Ref<>.
nullptr will mean end of the iteration.

CSSFontFace is creating a FontFace (and keeping a WeakPtr).
I don't know the reason but this require using a RefPtr<> as returned value to next().

> > Source/WebCore/dom/NodeList.h:34
> > +};
> 
> Semicolon here is incorrect.

OK, forgot to fix it in uploaded patch but will do in next one.

> > Source/WebCore/dom/NodeList.h:69
> > +    if (m_index < m_list->length())
> 
> This is inefficient because length() can cache the entire node list /
> collection.
> Just call item(m_index++) and check the return type instead.

OK

[Build Bot]

Comment on attachment 278080 [details]
Patch

Attachment 278080 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1265806

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html

[Build Bot]

Created attachment 278085 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 278080 [details]
Patch

Attachment 278080 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1265833

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html

[Build Bot]

Created attachment 278086 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278080 [details]
Patch

Attachment 278080 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1265847

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html

[Build Bot]

Created attachment 278087 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 278088 [details]
Fixing key/value order

[WebKit Commit Bot]

Attachment 278088 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeList.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 278088 [details]
Fixing key/value order

View in context: https://bugs.webkit.org/attachment.cgi?id=278088&action=review

Looks good.

When we have function templates in a header, I think it’s useful to declare the templates first and then define them, so we can look at the declarations and get an overview of what’s present. If we just put all the template definitions in, then the implementation and interface are all mixed up.

For bindings code, I think it’s valuable to use generic programming techniques. This means that we want to use overloading rather than naming the types of the arguments in function names, letting the type system work for us to make our code less type specific. That’s why there are a lot of pre-existing functions named "toJS", and I think that "iteratorValueToJS" should just be named toJS. Or some other clearer name, but not one that specifies which types are involved. I suggest we also consider giving fillForEachArgumentsWithIteratorValue a shorter name that doesn’t name the type involved. Maybe appendForEachArguments.

> Source/WebCore/dom/NodeList.h:49
> +        explicit Iterator(NodeList& list) : m_list(list) { }

I think this can probably be private. I believe that the outer class gets access to the private members of classes that are nested inside it.

[youenn fablet]

(In reply to comment #42)
> Comment on attachment 278088 [details]
> Fixing key/value order
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278088&action=review
> 
> Looks good.
> 
> When we have function templates in a header, I think it’s useful to declare
> the templates first and then define them, so we can look at the declarations
> and get an overview of what’s present. If we just put all the template
> definitions in, then the implementation and interface are all mixed up.
> 
> For bindings code, I think it’s valuable to use generic programming
> techniques. This means that we want to use overloading rather than naming
> the types of the arguments in function names, letting the type system work
> for us to make our code less type specific. That’s why there are a lot of
> pre-existing functions named "toJS", and I think that "iteratorValueToJS"
> should just be named toJS. Or some other clearer name, but not one that
> specifies which types are involved. I suggest we also consider giving
> fillForEachArgumentsWithIteratorValue a shorter name that doesn’t name the
> type involved. Maybe appendForEachArguments.
> 
> > Source/WebCore/dom/NodeList.h:49
> > +        explicit Iterator(NodeList& list) : m_list(list) { }
> 
> I think this can probably be private. I believe that the outer class gets
> access to the private members of classes that are nested inside it.

Thanks, I'll try to improve the patch accordingly.

There is an additional thing which I am not yet sure about.
Currently, DOMClass::Iterators are keeping a Ref<> to the iterated object.
I wonder whether we might want to move that to a RefPtr<>.

That would allow releasing the ref when the iterator is done.
That would also ensure that a "done: true" iterator could never be "done: false" later on, like in the case of an inserted node in NodeList or if calling NodeList::Iterator::next() enough times so that the iterator index is back to zero.

[youenn fablet]

I'll tackle this issue in a follow up patch, maybe at JSDomIterator level.

[youenn fablet]

Created attachment 278335 [details]
Patch

[WebKit Commit Bot]

Attachment 278335 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeList.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 278335 [details]
Patch

Attachment 278335 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1283661

New failing tests:
inspector/console/command-line-api.html

[Build Bot]

Created attachment 278338 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278335 [details]
Patch

Attachment 278335 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1283657

New failing tests:
inspector/console/command-line-api.html

[Build Bot]

Created attachment 278339 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278335 [details]
Patch

Attachment 278335 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1283666

New failing tests:
inspector/console/command-line-api.html

[Build Bot]

Created attachment 278340 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 278404 [details]
Fixing for-of loop for set iterators

[WebKit Commit Bot]

Attachment 278404 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeList.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to comment #53)
> Created attachment 278404 [details]
> Fixing for-of loop for set iterators

This patch does not provide the correct behavior for entries and forEach for set iterators (a counter value should be provided).
Also, it does not ensure done iterators remain done.

I plan to fix these two limitations in a follow-up patch.

[Darin Adler]

Comment on attachment 278404 [details]
Fixing for-of loop for set iterators

View in context: https://bugs.webkit.org/attachment.cgi?id=278404&action=review

Do we really need to call this createDOMIterator and domIteratorForEach? Maybe instead iteratorCreate and iteratorForEach?

> Source/WebCore/bindings/js/JSDOMBinding.h:647
> +template<typename Value1, typename Value2> inline JSC::JSValue jsPair(JSC::ExecState& state, JSDOMGlobalObject* globalObject, const Value1& value1, const Value2& value2)

The types should not be named "Value".

[youenn fablet]

Created attachment 278482 [details]
Patch for landing

[youenn fablet]

(In reply to comment #56)
> Comment on attachment 278404 [details]
> Fixing for-of loop for set iterators
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278404&action=review
> 
> Do we really need to call this createDOMIterator and domIteratorForEach?
> Maybe instead iteratorCreate and iteratorForEach?

OK.

> > Source/WebCore/bindings/js/JSDOMBinding.h:647
> > +template<typename Value1, typename Value2> inline JSC::JSValue jsPair(JSC::ExecState& state, JSDOMGlobalObject* globalObject, const Value1& value1, const Value2& value2)
> 
> The types should not be named "Value".

Updated to FirstType/SecondType.

[WebKit Commit Bot]

Comment on attachment 278482 [details]
Patch for landing

Clearing flags on attachment: 278482

Committed r200619: <http://trac.webkit.org/changeset/200619>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 278482 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=278482&action=review

> LayoutTests/fast/dom/nodeListIterator-expected.txt:12
> +FAIL forEachIndex should be 0 (of type number). Was [object HTMLDivElement] (of type object).

What are all these FAIL lines?

[Chris Dumez]

This patch seems to have broken m.yahoo.co.jp (content does not load at the bottom of the page).

JS Error:
[Error] TypeError: this.isCollectedElem is not a function. (In 'this.isCollectedElem(b)', 'this.isCollectedElem' is undefined)
	(anonymous function) (rappie_stream-1.8.0.js:4:6139)
	forEach
	forEach (rappie_stream-1.8.0.js:3:1710)
	collectTargets (rappie_stream-1.8.0.js:4:6018)
	(anonymous function) (rappie_stream-1.8.0.js:4:5450)
	forEach
	forEach (rappie_stream-1.8.0.js:3:1710)
	initRapid (rappie_stream-1.8.0.js:4:5380)
	r (rappie_stream-1.8.0.js:4:4024)
	(anonymous function) (rappie_stream-1.8.0.js:4:11207)
	(anonymous function) (rappie_stream-1.8.0.js:4:11497)
	Global Code (rappie_stream-1.8.0.js:4:11500)
[Error] TypeError: undefined is not an object (evaluating 'RappieStream.setNewPageParameter')
	initializer (mtop-page-top.js:657:2451)
	_initHierarchy (mtop-page-top.js:197:3768)
	_baseInit (mtop-page-top.js:197:1101)
	init (mtop-page-top.js:205:704)
	_initBase (mtop-page-top.js:197:954)
	v (mtop-page-top.js:197:105)
	o (mtop-page-top.js:213)
	n (mtop-page-top.js:279)
	r (mtop-page-top.js:639:3635)
	o (mtop-page-top.js:657:1955)
	initializer (mtop-page-top.js:655:288)
	_initHierarchy (mtop-page-top.js:197:3768)
	_baseInit (mtop-page-top.js:197:1101)
	init (mtop-page-top.js:205:704)
	_initBase (mtop-page-top.js:197:954)
	v (mtop-page-top.js:197:105)
	o (mtop-page-top.js:213)
	n (mtop-page-top.js:279)
	r (mtop-page-top.js:639:3635)
	r (mtop-page-top.js:654:5182)
	initializer (mtop-page-top.js:639:3251)
	r (mtop-page-top.js:639:1784)
	(anonymous function) (mtop-page-top.js:676)
	_notify (mtop-page-top.js:9:1497)
	(anonymous function) (mtop-page-top.js:9:974)
	_notify (mtop-page-top.js:9:1497)
	T (mtop-page-top.js:9:2429)
	_finish (mtop-page-top.js:16:4858)
	_onSuccess (mtop-page-top.js:16:5375)
	p (mtop-page-top.js:17:1326)
	_insert (mtop-page-top.js:17:1390)
	(anonymous function) (mtop-page-top.js:17:2386)
	_continue (mtop-page-top.js:17:2273)
	insert (mtop-page-top.js:17:2412)
	_use (mtop-page-top.js:9:2988)
	use (mtop-page-top.js:9:948)
	Global Code (mtop-page-top.js:676)


The page falls back nicely if forEach is not available:
            forEach: function(a, b, c) {
                if (a.forEach)
                    a.forEach(b, c);
                else
                    for (var d = 0; d < a.length; d++)
                        b.call(c, a[d], d, a)
            },

So presumably the if and else code paths are currently not equivalent.

c.f. rdar://problem/26812953

[Jiewen Tan]

Reverted r200619 for reason:

This incompleted feature broke http://m.yahoo.co.jp. Roll it out together with r200678.

Committed r202303: <http://trac.webkit.org/changeset/202303>

[Jiewen Tan]

Reverted r202302, r202303, r202305, and r202306 for reason:

Roll out the rollouts because of breaking the build.

Committed r202307: <http://trac.webkit.org/changeset/202307>

[youenn fablet]

(In reply to comment #61)
> Comment on attachment 278482 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278482&action=review
> 
> > LayoutTests/fast/dom/nodeListIterator-expected.txt:12
> > +FAIL forEachIndex should be 0 (of type number). Was [object HTMLDivElement] (of type object).
> 
> What are all these FAIL lines?

This was fixed in bug 157453.

[youenn fablet]

It might be that support for thisArg is missing. Let me check further.

[youenn fablet]

(In reply to comment #66)
> It might be that support for thisArg is missing. Let me check further.

I filed bug 159020 for that purpose.

[youenn fablet]

Can we close this one and bug 157453?

[Chris Dumez]

(In reply to comment #68)
> Can we close this one and bug 157453?

Yes, they eventually weren't rolled out after all.

[Chris Dumez]

Jiewen will follow up if issues remain but things looked good to me after your fix.