JSC iterator prototype defines @@iterator property that returns the iterator object. This notably allows doing for-of loops on iterable entries/values/keys methods.
Created attachment 282071 [details] Patch
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
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
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
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
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
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
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
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
Created attachment 282090 [details] Patch
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.
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
(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.
Created attachment 282216 [details] Using pragma once
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.
Created attachment 283640 [details] Patch
Created attachment 283641 [details] Patch
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?
(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.
Created attachment 283662 [details] Patch
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 :)
(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!
Comment on attachment 283662 [details] Patch Clearing flags on attachment: 283662 Committed r203235: <http://trac.webkit.org/changeset/203235>
All reviewed patches have been landed. Closing bug.
> > > 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