Bug 159115

Summary: DOMIterators should be assigned a correct prototype
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, cdumez, commit-queue, rniwa, zalan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 151937    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Patch
none
Using pragma once
none
Patch
none
Patch
none
Patch none

Description youenn fablet 2016-06-25 06:13:35 PDT
JSC iterator prototype defines @@iterator property that returns the iterator object.
This notably allows doing for-of loops on iterable entries/values/keys methods.
Comment 1 youenn fablet 2016-06-25 06:40:13 PDT
Created attachment 282071 [details]
Patch
Comment 2 Build Bot 2016-06-25 07:25:27 PDT
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
Comment 3 Build Bot 2016-06-25 07:25:31 PDT
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 4 Build Bot 2016-06-25 07:27:33 PDT
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
Comment 5 Build Bot 2016-06-25 07:27:37 PDT
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 6 Build Bot 2016-06-25 07:34:35 PDT
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
Comment 7 Build Bot 2016-06-25 07:34:38 PDT
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 8 Build Bot 2016-06-25 07:38:49 PDT
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
Comment 9 Build Bot 2016-06-25 07:38:51 PDT
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
Comment 10 youenn fablet 2016-06-26 02:15:42 PDT
Created attachment 282090 [details]
Patch
Comment 11 youenn fablet 2016-06-26 02:55:48 PDT
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 12 Alex Christensen 2016-06-27 23:05:03 PDT
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
Comment 13 youenn fablet 2016-06-27 23:19:24 PDT
(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.
Comment 14 youenn fablet 2016-06-27 23:56:06 PDT
Created attachment 282216 [details]
Using pragma once
Comment 15 youenn fablet 2016-06-29 01:23:55 PDT
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.
Comment 16 youenn fablet 2016-07-14 07:00:44 PDT
Created attachment 283640 [details]
Patch
Comment 17 youenn fablet 2016-07-14 08:10:59 PDT
Created attachment 283641 [details]
Patch
Comment 18 Chris Dumez 2016-07-14 09:48:24 PDT
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?
Comment 19 youenn fablet 2016-07-14 10:49:06 PDT
(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.
Comment 20 youenn fablet 2016-07-14 11:14:03 PDT
Created attachment 283662 [details]
Patch
Comment 21 Chris Dumez 2016-07-14 11:36:20 PDT
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 :)
Comment 22 youenn fablet 2016-07-14 12:12:29 PDT
(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 23 WebKit Commit Bot 2016-07-14 12:33:13 PDT
Comment on attachment 283662 [details]
Patch

Clearing flags on attachment: 283662

Committed r203235: <http://trac.webkit.org/changeset/203235>
Comment 24 WebKit Commit Bot 2016-07-14 12:33:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 youenn fablet 2016-07-15 08:14:37 PDT
> > > 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