Bug 159115 – DOMIterators should be assigned a correct prototype

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

JSC iterator prototype defines @@iterator property that returns the iterator object.
This notably allows doing for-of loops on iterable entries/values/keys methods.

[youenn fablet]

Created attachment 282071 [details]
Patch

[Build Bot]

Comment on attachment 282071 [details]
Patch

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

New failing tests:
fast/dom/nodeListIterator.html
fast/text/font-face-set-document.html
fast/text/font-face-set-cssom.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 282073 [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

[Build Bot]

Comment on attachment 282071 [details]
Patch

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

New failing tests:
fast/dom/nodeListIterator.html
fast/text/font-face-set-document.html
fast/text/font-face-set-cssom.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 282074 [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 282071 [details]
Patch

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

New failing tests:
fast/dom/nodeListIterator.html
fast/text/font-face-set-document.html
fast/text/font-face-set-cssom.html
fast/text/font-face-set-javascript.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 282071 [details]
Patch

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

New failing tests:
fast/text/font-face-set-javascript.html
fast/dom/nodeListIterator.html
fast/text/font-face-set-cssom.html
fast/text/font-face-set-document.html

[Build Bot]

Created attachment 282076 [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

[youenn fablet]

Created attachment 282090 [details]
Patch

[youenn fablet]

As can be seen from tests imported from Chrome, the keys() method behaviour is not on par.
The current implementation does not correctly handle the differences between set-like iterators and array-like iterators.
I plan to fix this as a follow-up patch.

[Alex Christensen]

Comment on attachment 282090 [details]
Patch

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

Is this related to https://tc39.github.io/ecma262/#sec-well-known-symbols ?

> Source/WebCore/ForwardingHeaders/runtime/IteratorPrototype.h:2
> +#ifndef WebCore_FWD_IteratorPrototype_h
> +#define WebCore_FWD_IteratorPrototype_h

#pragma once is the latest thing

[youenn fablet]

(In reply to comment #12)
> Comment on attachment 282090 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282090&action=review
> 
> Is this related to https://tc39.github.io/ecma262/#sec-well-known-symbols ?

Yes.
Iterable is defined in http://heycam.github.io/webidl/#idl-iterable which has a pointer to @@iterator and the link you mentionned.
This patch is aligning the behaviour of DOM iterators with JSC iterators which all have a @@iterator that allows for-of loops on iterators retrieved from entries/values/keys methods.


> 
> > Source/WebCore/ForwardingHeaders/runtime/IteratorPrototype.h:2
> > +#ifndef WebCore_FWD_IteratorPrototype_h
> > +#define WebCore_FWD_IteratorPrototype_h
> 
> #pragma once is the latest thing

OK, I will fix it.

[youenn fablet]

Created attachment 282216 [details]
Using pragma once

[youenn fablet]

Cancelling the review for now.
I think it would be better to first separate setlike from iterable (bug 159140).
Then, for value-iterable, we should not use DOMIterator but use directly array prototype  methods.
Then, the prototype for setlike/pair-iterable iterators should be set appropriately.

[youenn fablet]

Created attachment 283640 [details]
Patch

[youenn fablet]

Created attachment 283641 [details]
Patch

[Chris Dumez]

Comment on attachment 283641 [details]
Patch

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

The change looks fine but...

> Source/WebCore/ChangeLog:14
> +        Covered by updated test.

Can you please add a specific test that checks that the prototype is IteratorPrototype and check that it is enumerable / writable / configurable?

> LayoutTests/imported/w3c/ChangeLog:7
> +

Please add a Changelog. Is this a re-import?

[youenn fablet]

(In reply to comment #18)
> Comment on attachment 283641 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283641&action=review
> 
> The change looks fine but...
> 
> > Source/WebCore/ChangeLog:14
> > +        Covered by updated test.
> 
> Can you please add a specific test that checks that the prototype is
> IteratorPrototype and check that it is enumerable / writable / configurable?

OK

> > LayoutTests/imported/w3c/ChangeLog:7
> > +
> 
> Please add a Changelog. Is this a re-import?

No, I will upstream it on w3c Github once landed.

[youenn fablet]

Created attachment 283662 [details]
Patch

[Chris Dumez]

Comment on attachment 283662 [details]
Patch

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

r=me

> Source/WebCore/bindings/js/JSDOMIterator.h:244
> +    JSC_NATIVE_INTRINSIC_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->next, next, 0, 0, JSC::NoIntrinsic);

Good, I see that adding the test found a bug :)

[youenn fablet]

(In reply to comment #21)
> Comment on attachment 283662 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283662&action=review
> 
> r=me
> 
> > Source/WebCore/bindings/js/JSDOMIterator.h:244
> > +    JSC_NATIVE_INTRINSIC_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->next, next, 0, 0, JSC::NoIntrinsic);
> 
> Good, I see that adding the test found a bug :)

Definitely, thanks for the review!

[WebKit Commit Bot]

Comment on attachment 283662 [details]
Patch

Clearing flags on attachment: 283662

Committed r203235: <http://trac.webkit.org/changeset/203235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> > > LayoutTests/imported/w3c/ChangeLog:7
> > > +
> > 
> > Please add a Changelog. Is this a re-import?
> 
> No, I will upstream it on w3c Github once landed.

PR is https://github.com/w3c/web-platform-tests/pull/3300