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:

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

youenn fablet

Reported 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.

Attachments
Patch (8.56 KB, patch) 2016-06-25 06:40 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-yosemite (837.07 KB, application/zip) 2016-06-25 07:25 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (820.26 KB, application/zip) 2016-06-25 07:27 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (691.95 KB, application/zip) 2016-06-25 07:34 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews113 for mac-yosemite (1.45 MB, application/zip) 2016-06-25 07:38 PDT, Build Bot	no flags	Details
Patch (7.96 KB, patch) 2016-06-26 02:15 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Using pragma once (7.86 KB, patch) 2016-06-27 23:56 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (4.53 KB, patch) 2016-07-14 07:00 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (4.92 KB, patch) 2016-07-14 08:10 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (6.71 KB, patch) 2016-07-14 11:14 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

youenn fablet

Comment 1 2016-06-25 06:40:13 PDT

Created attachment 282071 [details] Patch

Build Bot

Comment 2 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

Build Bot

Comment 3 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

Build Bot

Comment 4 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

Build Bot

Comment 5 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

Build Bot

Comment 6 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

Build Bot

Comment 7 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

Build Bot

Comment 8 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

Build Bot

Comment 9 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

youenn fablet

Comment 10 2016-06-26 02:15:42 PDT

Created attachment 282090 [details] Patch

youenn fablet

Comment 11 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.

Alex Christensen

Comment 12 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

youenn fablet

Comment 13 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.

youenn fablet

Comment 14 2016-06-27 23:56:06 PDT

Created attachment 282216 [details] Using pragma once

youenn fablet

Comment 15 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.

youenn fablet

Comment 16 2016-07-14 07:00:44 PDT

Created attachment 283640 [details] Patch

youenn fablet

Comment 17 2016-07-14 08:10:59 PDT

Created attachment 283641 [details] Patch

Chris Dumez

Comment 18 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?

youenn fablet

Comment 19 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.

youenn fablet

Comment 20 2016-07-14 11:14:03 PDT

Created attachment 283662 [details] Patch

Chris Dumez

Comment 21 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 :)

youenn fablet

Comment 22 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!

WebKit Commit Bot

Comment 23 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>

WebKit Commit Bot

Comment 24 2016-07-14 12:33:19 PDT

All reviewed patches have been landed. Closing bug.

youenn fablet

Comment 25 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

Note You need to log in before you can comment on or make changes to this bug.