RESOLVED FIXED 167091
JavaScript for-of does not work on a lot of collection types (e.g. HTMLCollection) 
https://bugs.webkit.org/show_bug.cgi?id=167091
Summary JavaScript for-of does not work on a lot of collection types (e.g. HTMLCollec...
Jimmy Joe Shabadoo
Reported 2017-01-16 08:05:08 PST
Consider the following code: <html> <head> <meta charset="utf-8"> </head> <body> <div class="cell">div 1 text</div> <div class="cell">div 2 text</div> <div class="cell">div 3 text</div> <script type="text/javascript"> let list = document.getElementsByClassName("cell") for(let y of list){ console.log("y:", y) } </script> </body> </html> When run in Chrome V55.0, it produces the following console output: y: <div class="cell">div 1 text</div> y: <div class="cell">div 2 text</div> y: <div class="cell">div 3 text</div> Firefox V50.1 returns similar results as well. However when run in Safari, this same file generates an error stating TypeError: y of list is not a function. (In 'y of list', 'y of list' is >undefined). This appears to be because the value returned to the list variable is not an array, but an HTML collection as described in the following link: https://bugs.chromium.org/p/chromium/issues/detail?id=538558
Attachments
WIP Patch (5.38 KB, patch)
2017-01-20 20:54 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
WIP Patch (14.96 KB, patch)
2017-01-21 09:07 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
WIP Patch (18.91 KB, patch)
2017-01-21 09:22 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews113 for mac-elcapitan (1.69 MB, application/zip)
2017-01-21 10:37 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (828.99 KB, application/zip)
2017-01-21 10:43 PST, Build Bot 		no flags
Details
Patch (38.18 KB, patch)
2017-01-21 11:22 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews117 for mac-elcapitan (370.72 KB, application/zip)
2017-01-21 12:15 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (4.30 MB, application/zip)
2017-01-21 12:45 PST, Build Bot 		no flags
Details
Patch (38.35 KB, patch)
2017-01-21 12:52 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews117 for mac-elcapitan (350.53 KB, application/zip)
2017-01-21 13:45 PST, Build Bot 		no flags
Details
Patch (38.37 KB, patch)
2017-01-21 14:38 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (39.21 KB, patch)
2017-01-21 20:38 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2017-01-17 18:30:00 PST
Yeah, we should do this.
Chris Dumez
Comment 2 2017-01-17 19:20:49 PST
Per https://heycam.github.io/webidl/#es-iterator: There should be an @@iterator if: "an indexed property getter and an integer-typed attribute named “length”" This applies to HTMLCollection for e.g. but I believe our bindings generator currently only adds an @@iterator if there is an maplike/setlike/iterable declaration in the IDL. However, I believe HTMLCollection is intentionally not marked as iterable<> in the IDL because this would add extra methods (forEach, keys, values, entries) that we do not want to expose because HTMLCollection also has a named property getter. I can implement this soon unless someone gets to it first.
Ryosuke Niwa
Comment 3 2017-01-17 19:39:43 PST
Oh yeah, I do recall some discussions about named properties and new methods. I think there was some compat. concern there.
Chris Dumez
Comment 4 2017-01-20 16:59:09 PST
Started working on this.
Chris Dumez
Comment 5 2017-01-20 20:10:14 PST
WebKit interfaces that seem to be impacted by this: AudioTrackList ClientRectList CSSRuleList CSSStyleDeclaration DataTransferItemList DeprecatedCSSOMValueList DOMMimeTypeArray DOMNamedFlowCollection DOMPlugin DOMPluginArray DOMStringList FileList HTMLAllCollection HTMLCollection HTMLFormElement HTMLOptionsCollection HTMLSelectElement MediaList NamedNodeMap SourceBufferList StyleSheetList TextTrackCueList TextTrackList TouchList VideoTrackList VTTRegionList
Chris Dumez
Comment 6 2017-01-20 20:54:49 PST
Created attachment 299421 [details] WIP Patch Needs tests.
Chris Dumez
Comment 7 2017-01-21 09:07:05 PST
Created attachment 299431 [details] WIP Patch
Chris Dumez
Comment 8 2017-01-21 09:22:23 PST
Created attachment 299432 [details] WIP Patch
Chris Dumez
Comment 9 2017-01-21 09:27:02 PST
Comment on attachment 299432 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299432&action=review > LayoutTests/fast/dom/collection-iterators-expected.txt:98 > +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know.
Chris Dumez
Comment 10 2017-01-21 09:29:25 PST
Comment on attachment 299432 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299432&action=review >> LayoutTests/fast/dom/collection-iterators-expected.txt:98 >> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined > > @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know. > Array.prototype.values.call(document.all) < TypeError: Array.prototype.values requires that |this| not be null or undefined
Chris Dumez
Comment 11 2017-01-21 09:39:39 PST
Comment on attachment 299432 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299432&action=review >>> LayoutTests/fast/dom/collection-iterators-expected.txt:98 >>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined >> >> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know. > > From ArrayPrototype.js: function values() { "use strict"; if (this == null) @throwTypeError("Array.prototype.values requires that |this| not be null or undefined"); return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext); } So somehow |this| ends up being null even though I am calling with an object: Array.prototype.values.call(document.all)
Chris Dumez
Comment 12 2017-01-21 09:48:27 PST
Comment on attachment 299432 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299432&action=review >>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98 >>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined >>> >>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know. >> >> > > From ArrayPrototype.js: > > function values() > { > "use strict"; > > if (this == null) > @throwTypeError("Array.prototype.values requires that |this| not be null or undefined"); > > return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext); > } > > So somehow |this| ends up being null even though I am calling with an object: > Array.prototype.values.call(document.all) Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined. Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow. I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it.
Chris Dumez
Comment 13 2017-01-21 09:53:00 PST
Comment on attachment 299432 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299432&action=review >>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98 >>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined >>>> >>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know. >>> >>> >> >> From ArrayPrototype.js: >> >> function values() >> { >> "use strict"; >> >> if (this == null) >> @throwTypeError("Array.prototype.values requires that |this| not be null or undefined"); >> >> return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext); >> } >> >> So somehow |this| ends up being null even though I am calling with an object: >> Array.prototype.values.call(document.all) > > Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined. > > Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow. > I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it. From https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all: """ The object returned for all has several unusual behaviors: The user agent must act as if the ToBoolean abstract operation in JavaScript returns false when given the object returned for all. The user agent must act as if the Abstract Equality Comparison algorithm, when given the object returned for all, returns true when compared to the undefined and null values. (Comparisons using the Strict Equality Comparison algorithm, and Abstract Equality comparisons to other values such as strings or objects, are unaffected.) The user agent must act such that the typeof operator in JavaScript returns the string "undefined" when applied to the object returned for all. """ So I think our MasqueradesAsUndefined is trying to achieve this but is too aggressive and masquerades document.all as undefined in cases where it should not.
Chris Dumez
Comment 14 2017-01-21 10:17:21 PST
Comment on attachment 299432 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299432&action=review >>>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98 >>>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined >>>>> >>>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know. >>>> >>>> >>> >>> From ArrayPrototype.js: >>> >>> function values() >>> { >>> "use strict"; >>> >>> if (this == null) >>> @throwTypeError("Array.prototype.values requires that |this| not be null or undefined"); >>> >>> return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext); >>> } >>> >>> So somehow |this| ends up being null even though I am calling with an object: >>> Array.prototype.values.call(document.all) >> >> Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined. >> >> Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow. >> I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it. > > From https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all: > """ > The object returned for all has several unusual behaviors: > > The user agent must act as if the ToBoolean abstract operation in JavaScript returns false when given the object returned for all. > > The user agent must act as if the Abstract Equality Comparison algorithm, when given the object returned for all, returns true when compared to the undefined and null values. (Comparisons using the Strict Equality Comparison algorithm, and Abstract Equality comparisons to other values such as strings or objects, are unaffected.) > > The user agent must act such that the typeof operator in JavaScript returns the string "undefined" when applied to the object returned for all. > """ > > So I think our MasqueradesAsUndefined is trying to achieve this but is too aggressive and masquerades document.all as undefined in cases where it should not. EcmaScript spec is at: https://www.ecma-international.org/ecma-262/7.0/index.html#sec-array.prototype.values It says: """ Let O be ? ToObject(this value). Return CreateArrayIterator(O, "value"). """ And the section for ToObject says to throw a TypeError if the argument type is undefined or null. Our code does a "this == null" check which returns true if the type masquerades as undefined. I think changing the check to (this === null || this === undefined) would match the EcmaScript specification and allow the Array prototype functions to be used on types that masquerades as undefined (such as document.all).
Build Bot
Comment 15 2017-01-21 10:37:52 PST
Comment on attachment 299432 [details] WIP Patch Attachment 299432 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2926437 New failing tests: fast/dom/FileList-iterator.html
Build Bot
Comment 16 2017-01-21 10:37:57 PST
Created attachment 299433 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 17 2017-01-21 10:43:00 PST
Comment on attachment 299432 [details] WIP Patch Attachment 299432 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2926438 New failing tests: fast/dom/collection-iterators.html
Build Bot
Comment 18 2017-01-21 10:43:05 PST
Created attachment 299434 [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.6
Chris Dumez
Comment 19 2017-01-21 11:22:38 PST
Created attachment 299437 [details] Patch
Build Bot
Comment 20 2017-01-21 12:15:45 PST
Comment on attachment 299437 [details] Patch Attachment 299437 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2926783 Number of test failures exceeded the failure limit.
Build Bot
Comment 21 2017-01-21 12:15:50 PST
Created attachment 299439 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Chris Dumez
Comment 22 2017-01-21 12:43:14 PST
(In reply to comment #14) > Comment on attachment 299432 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299432&action=review > > >>>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98 > >>>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined > >>>>> > >>>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know. > >>>> > >>>> > >>> > >>> From ArrayPrototype.js: > >>> > >>> function values() > >>> { > >>> "use strict"; > >>> > >>> if (this == null) > >>> @throwTypeError("Array.prototype.values requires that |this| not be null or undefined"); > >>> > >>> return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext); > >>> } > >>> > >>> So somehow |this| ends up being null even though I am calling with an object: > >>> Array.prototype.values.call(document.all) > >> > >> Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined. > >> > >> Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow. > >> I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it. > > > > From https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all: > > """ > > The object returned for all has several unusual behaviors: > > > > The user agent must act as if the ToBoolean abstract operation in JavaScript returns false when given the object returned for all. > > > > The user agent must act as if the Abstract Equality Comparison algorithm, when given the object returned for all, returns true when compared to the undefined and null values. (Comparisons using the Strict Equality Comparison algorithm, and Abstract Equality comparisons to other values such as strings or objects, are unaffected.) > > > > The user agent must act such that the typeof operator in JavaScript returns the string "undefined" when applied to the object returned for all. > > """ > > > > So I think our MasqueradesAsUndefined is trying to achieve this but is too aggressive and masquerades document.all as undefined in cases where it should not. > > EcmaScript spec is at: > https://www.ecma-international.org/ecma-262/7.0/index.html#sec-array. > prototype.values > > It says: > """ > Let O be ? ToObject(this value). > Return CreateArrayIterator(O, "value"). > """ > > And the section for ToObject says to throw a TypeError if the argument type > is undefined or null. > > Our code does a "this == null" check which returns true if the type > masquerades as undefined. I think changing the check to (this === null || > this === undefined) would match the EcmaScript specification and allow the > Array prototype functions to be used on types that masquerades as undefined > (such as document.all). Looks like the debug Build does not like my proposal: stderr: Bad global capture in builtin: 'undefined' (function () { "use strict"; if (this === null || this === undefined) @throwTypeError("Array.prototype.values requires that |this| not be null or undefined"); return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext); }) 1 0x10ee82740 WTFCrash 2 0x10eb074ba JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode) 3 0x10df8a333 std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::Parser<JSC::Lexer<unsigned char>
Build Bot
Comment 23 2017-01-21 12:45:04 PST
Comment on attachment 299437 [details] Patch Attachment 299437 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2926838 New failing tests: fast/dom/collection-iterators.html
Build Bot
Comment 24 2017-01-21 12:45:10 PST
Created attachment 299440 [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.6
Chris Dumez
Comment 25 2017-01-21 12:52:07 PST
Created attachment 299441 [details] Patch
Chris Dumez
Comment 26 2017-01-21 13:04:37 PST
(In reply to comment #22) > (In reply to comment #14) > > Comment on attachment 299432 [details] > > WIP Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=299432&action=review > > > > >>>>>> LayoutTests/fast/dom/collection-iterators-expected.txt:98 > > >>>>>> +Exception: TypeError: Array.prototype.values requires that |this| not be null or undefined > > >>>>> > > >>>>> @Youenn: I have not figured out yet why iterating over an HTMLAllCollection throws. If you have an idea, please let me know. > > >>>> > > >>>> > > >>> > > >>> From ArrayPrototype.js: > > >>> > > >>> function values() > > >>> { > > >>> "use strict"; > > >>> > > >>> if (this == null) > > >>> @throwTypeError("Array.prototype.values requires that |this| not be null or undefined"); > > >>> > > >>> return new @createArrayIterator(@Object(this), "value", @arrayIteratorValueNext); > > >>> } > > >>> > > >>> So somehow |this| ends up being null even though I am calling with an object: > > >>> Array.prototype.values.call(document.all) > > >> > > >> Hmm, so this is caused by MasqueradesAsUndefined in HTMLAllCollection.idl which adds the JSC::MasqueradesAsUndefined. > > >> > > >> Array.prototype.values.call(document.all) works in both Chrome and Firefox. for..of also works on document.all in those browsers so I think we need to get this working somehow. > > >> I do not know the reason HTMLAllCollection masquerades as undefined but I assume this was an attempt to deprecate it. > > > > > > From https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all: > > > """ > > > The object returned for all has several unusual behaviors: > > > > > > The user agent must act as if the ToBoolean abstract operation in JavaScript returns false when given the object returned for all. > > > > > > The user agent must act as if the Abstract Equality Comparison algorithm, when given the object returned for all, returns true when compared to the undefined and null values. (Comparisons using the Strict Equality Comparison algorithm, and Abstract Equality comparisons to other values such as strings or objects, are unaffected.) > > > > > > The user agent must act such that the typeof operator in JavaScript returns the string "undefined" when applied to the object returned for all. > > > """ > > > > > > So I think our MasqueradesAsUndefined is trying to achieve this but is too aggressive and masquerades document.all as undefined in cases where it should not. > > > > EcmaScript spec is at: > > https://www.ecma-international.org/ecma-262/7.0/index.html#sec-array. > > prototype.values > > > > It says: > > """ > > Let O be ? ToObject(this value). > > Return CreateArrayIterator(O, "value"). > > """ > > > > And the section for ToObject says to throw a TypeError if the argument type > > is undefined or null. > > > > Our code does a "this == null" check which returns true if the type > > masquerades as undefined. I think changing the check to (this === null || > > this === undefined) would match the EcmaScript specification and allow the > > Array prototype functions to be used on types that masquerades as undefined > > (such as document.all). > > Looks like the debug Build does not like my proposal: > stderr: > Bad global capture in builtin: 'undefined' > (function () > { > "use strict"; > if (this === null || this === undefined) > @throwTypeError("Array.prototype.values requires that |this| not be > null or undefined"); > return new @createArrayIterator(@Object(this), "value", > @arrayIteratorValueNext); > }) > 1 0x10ee82740 WTFCrash > 2 0x10eb074ba JSC::Parser<JSC::Lexer<unsigned char> > >::parseInner(JSC::Identifier const&, JSC::SourceParseMode) > 3 0x10df8a333 std::__1::unique_ptr<JSC::ProgramNode, > std::__1::default_delete<JSC::ProgramNode> > JSC::Parser<JSC::Lexer<unsigned > char> The asserting code looks like: #ifndef NDEBUG if (m_parsingBuiltin && isProgramParseMode(parseMode)) { VariableEnvironment& lexicalVariables = scope->lexicalVariables(); const HashSet<UniquedStringImpl*>& closedVariableCandidates = scope->closedVariableCandidates(); for (UniquedStringImpl* candidate : closedVariableCandidates) { if (!lexicalVariables.contains(candidate) && !varDeclarations.contains(candidate) && !candidate->isSymbol()) { dataLog("Bad global capture in builtin: '", candidate, "'\n"); dataLog(m_source->view()); CRASH(); } } } #endif // NDEBUG Could a person more knowledgeable with JSC help me out? Why doesn't it allow me to do a "this === null || this === undefined" in ArrayPrototype.js ? Based on the error message, looks like it does not like the part with "undefined", but this looks valid to me.
youenn fablet
Comment 27 2017-01-21 13:25:33 PST
Try @undefined
Build Bot
Comment 28 2017-01-21 13:45:39 PST
Comment on attachment 299441 [details] Patch Attachment 299441 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2927102 Number of test failures exceeded the failure limit.
Build Bot
Comment 29 2017-01-21 13:45:43 PST
Created attachment 299443 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Chris Dumez
Comment 30 2017-01-21 14:38:16 PST
Created attachment 299445 [details] Patch
Chris Dumez
Comment 31 2017-01-21 14:38:52 PST
(In reply to comment #27) > Try @undefined Thanks, this seems to do the trick.
Darin Adler
Comment 32 2017-01-21 19:01:52 PST
Comment on attachment 299445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299445&action=review > Source/JavaScriptCore/ChangeLog:12 > + Update Array methods to throw a TypeError when (this === null || this === undefined) > + instead of when (this == null). This is because (this == null) returns true for types > + that masquerades as undefined (such as document.all) and this prevented use of the > + Array API on such types. The specification only stays to use ToObject(), which throws > + when the input is undefined or null. I hope the JavaScript compiler still optimizes this new case well. > Source/WebCore/bindings/js/JSDOMIterator.cpp:70 > + JSC::ArrayPrototype* arrayPrototype = globalObject.arrayPrototype(); > + ASSERT(arrayPrototype); > + > + JSC::ExecState* state = globalObject.globalExec(); > + ASSERT(state); > + JSC::VM& vm = state->vm(); > + > + auto copyProperty = [&] (const JSC::Identifier& arrayIdentifier, const JSC::Identifier& otherIdentifier, unsigned attributes = 0) { > + JSC::JSValue value = arrayPrototype->getDirect(vm, arrayIdentifier); > + ASSERT(value); > + prototype.putDirect(vm, otherIdentifier, value, attributes); > + }; > + > copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), JSC::DontEnum); I must prefer to write functions like this with auto and references. Also, the three assertions here that things are not null don’t seem important to me; null is highly unlikely and the immediate crash we will get if something is null seems likely simple to debug. And the copyProperty function does not seem, to me, to be an abstraction that makes the code read more clearly. If you apply all of that thinking, then here is my version of this function: auto& vm = globalObject.globalExec()->vm(); auto& builtinNames = vm.propertyNames->builtinNames(); auto values = globalObject.arrayPrototype()->getDirect(vm, builtinNames.valuesPrivateName()); prototype.putDirect(vm, builtinNames.iteratorSymbol(), values, JSC::DontEnum); Or the last two lines could be combined and we could do away with the local variable. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2990 > + #return 1 if $interface->maplike; > + #return 1 if $interface->setlike; These commented-out lines of code need a comment explaining why they are commented out.
Darin Adler
Comment 33 2017-01-21 19:02:55 PST
Comment on attachment 299445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299445&action=review >> Source/WebCore/bindings/js/JSDOMIterator.cpp:70 >> copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), JSC::DontEnum); > > I must prefer to write functions like this with auto and references. Also, the three assertions here that things are not null don’t seem important to me; null is highly unlikely and the immediate crash we will get if something is null seems likely simple to debug. And the copyProperty function does not seem, to me, to be an abstraction that makes the code read more clearly. > > If you apply all of that thinking, then here is my version of this function: > > auto& vm = globalObject.globalExec()->vm(); > auto& builtinNames = vm.propertyNames->builtinNames(); > auto values = globalObject.arrayPrototype()->getDirect(vm, builtinNames.valuesPrivateName()); > prototype.putDirect(vm, builtinNames.iteratorSymbol(), values, JSC::DontEnum); > > Or the last two lines could be combined and we could do away with the local variable. Meant to say "much prefer" not "must prefer".
Darin Adler
Comment 34 2017-01-21 19:04:19 PST
Comment on attachment 299445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299445&action=review >>> Source/WebCore/bindings/js/JSDOMIterator.cpp:70 >>> copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), JSC::DontEnum); >> >> I must prefer to write functions like this with auto and references. Also, the three assertions here that things are not null don’t seem important to me; null is highly unlikely and the immediate crash we will get if something is null seems likely simple to debug. And the copyProperty function does not seem, to me, to be an abstraction that makes the code read more clearly. >> >> If you apply all of that thinking, then here is my version of this function: >> >> auto& vm = globalObject.globalExec()->vm(); >> auto& builtinNames = vm.propertyNames->builtinNames(); >> auto values = globalObject.arrayPrototype()->getDirect(vm, builtinNames.valuesPrivateName()); >> prototype.putDirect(vm, builtinNames.iteratorSymbol(), values, JSC::DontEnum); >> >> Or the last two lines could be combined and we could do away with the local variable. > > Meant to say "much prefer" not "must prefer". Or maybe this version: auto& vm = globalObject.globalExec()->vm(); auto& names = vm.propertyNames->builtinNames(); prototype.putDirect(vm, names.iteratorSymbol(), globalObject.arrayPrototype()->getDirect(vm, names.valuesPrivateName()), JSC::DontEnum);
Darin Adler
Comment 35 2017-01-21 19:05:36 PST
Comment on attachment 299445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299445&action=review >>>> Source/WebCore/bindings/js/JSDOMIterator.cpp:70 >>>> copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), JSC::DontEnum); >>> >>> I must prefer to write functions like this with auto and references. Also, the three assertions here that things are not null don’t seem important to me; null is highly unlikely and the immediate crash we will get if something is null seems likely simple to debug. And the copyProperty function does not seem, to me, to be an abstraction that makes the code read more clearly. >>> >>> If you apply all of that thinking, then here is my version of this function: >>> >>> auto& vm = globalObject.globalExec()->vm(); >>> auto& builtinNames = vm.propertyNames->builtinNames(); >>> auto values = globalObject.arrayPrototype()->getDirect(vm, builtinNames.valuesPrivateName()); >>> prototype.putDirect(vm, builtinNames.iteratorSymbol(), values, JSC::DontEnum); >>> >>> Or the last two lines could be combined and we could do away with the local variable. >> >> Meant to say "much prefer" not "must prefer". > > Or maybe this version: > > auto& vm = globalObject.globalExec()->vm(); > auto& names = vm.propertyNames->builtinNames(); > prototype.putDirect(vm, names.iteratorSymbol(), globalObject.arrayPrototype()->getDirect(vm, names.valuesPrivateName()), JSC::DontEnum); I see now that this is using the idiom of the function above. I still like my version better!
youenn fablet
Comment 36 2017-01-21 19:15:05 PST
Comment on attachment 299445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299445&action=review > Source/WebCore/bindings/js/JSDOMIterator.cpp:66 > + ASSERT(value); I like this assert, it ensures that we have a meaningful property. Maybe we should have a more precise check, for instance that this is a function. > Source/WebCore/bindings/js/JSDOMIterator.h:37 > +void addValueIterator(JSC::JSGlobalObject&, JSC::JSObject&); Maybe we should have better names for these two, something like copyArrayIterableProperties and copyArrayIteratorProperty
Chris Dumez
Comment 37 2017-01-21 19:29:55 PST
(In reply to comment #36) > Comment on attachment 299445 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299445&action=review > > > Source/WebCore/bindings/js/JSDOMIterator.cpp:66 > > + ASSERT(value); > > I like this assert, it ensures that we have a meaningful property. > Maybe we should have a more precise check, for instance that this is a > function. > > > Source/WebCore/bindings/js/JSDOMIterator.h:37 > > +void addValueIterator(JSC::JSGlobalObject&, JSC::JSObject&); > > Maybe we should have better names for these two, something like > copyArrayIterableProperties and copyArrayIteratorProperty Actually, since addValueIterator is basically a one-liner, I think I'd prefer inlining it in the bindings generator instead of making a function call. Also, this is more consistent with the iterator for IsKeyValueIterableInterface() interfaces that is already inlined in the generator.
Chris Dumez
Comment 38 2017-01-21 20:38:53 PST
Created attachment 299455 [details] Patch
WebKit Commit Bot
Comment 39 2017-01-21 21:49:42 PST
Comment on attachment 299455 [details] Patch Clearing flags on attachment: 299455 Committed r211024: <http://trac.webkit.org/changeset/211024>
WebKit Commit Bot
Comment 40 2017-01-21 21:49:50 PST
All reviewed patches have been landed. Closing bug.
Geoffrey Garen
Comment 41 2017-01-22 09:20:25 PST
Comment on attachment 299455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299455&action=review > Source/JavaScriptCore/ChangeLog:12 > + Update Array methods to throw a TypeError when (this === null || this === undefined) > + instead of when (this == null). This is because (this == null) returns true for types > + that masquerades as undefined (such as document.all) and this prevented use of the > + Array API on such types. The specification only stays to use ToObject(), which throws > + when the input is undefined or null. Do we really want to edit new features to make them backwards-compatible with document.all? document.all is not standard, and we only support it for backwards compatibility.
Chris Dumez
Comment 42 2017-01-22 09:24:08 PST
(In reply to comment #41) > Comment on attachment 299455 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299455&action=review > > > Source/JavaScriptCore/ChangeLog:12 > > + Update Array methods to throw a TypeError when (this === null || this === undefined) > > + instead of when (this == null). This is because (this == null) returns true for types > > + that masquerades as undefined (such as document.all) and this prevented use of the > > + Array API on such types. The specification only stays to use ToObject(), which throws > > + when the input is undefined or null. > > Do we really want to edit new features to make them backwards-compatible > with document.all? document.all is not standard, and we only support it for > backwards compatibility. As I mentioned above, for..of works on document.all in other browsers. Not supporting it would be a compatibility risk IMHO.
Chris Dumez
Comment 43 2017-01-22 09:30:46 PST
(In reply to comment #42) > (In reply to comment #41) > > Comment on attachment 299455 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=299455&action=review > > > > > Source/JavaScriptCore/ChangeLog:12 > > > + Update Array methods to throw a TypeError when (this === null || this === undefined) > > > + instead of when (this == null). This is because (this == null) returns true for types > > > + that masquerades as undefined (such as document.all) and this prevented use of the > > > + Array API on such types. The specification only stays to use ToObject(), which throws > > > + when the input is undefined or null. > > > > Do we really want to edit new features to make them backwards-compatible > > with document.all? document.all is not standard, and we only support it for > > backwards compatibility. > > As I mentioned above, for..of works on document.all in other browsers. Not > supporting it would be a compatibility risk IMHO. I guess it comes to this question: Does this make the Array API code worse to support document.all? I am not familiar enough JS to answer this question. For e.g. is the new code slower? To me, it looks like the cost is very low to support document.all so I did it. If you disagree though, we can try not supporting for..of on document.all. While I try to be compatible with other browsers as much as possible, I think the compatibility risk here would be low given that this is a legacy API and DOM API iterators are new. Let me know what you think. I can put up a patch.
Joseph Pecoraro
Comment 44 2017-01-22 19:02:58 PST
(In reply to comment #43) > (In reply to comment #42) > > (In reply to comment #41) > > > Comment on attachment 299455 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=299455&action=review > > > > > > > Source/JavaScriptCore/ChangeLog:12 > > > > + Update Array methods to throw a TypeError when (this === null || this === undefined) > > > > + instead of when (this == null). This is because (this == null) returns true for types > > > > + that masquerades as undefined (such as document.all) and this prevented use of the > > > > + Array API on such types. The specification only stays to use ToObject(), which throws > > > > + when the input is undefined or null. > > > > > > Do we really want to edit new features to make them backwards-compatible > > > with document.all? document.all is not standard, and we only support it for > > > backwards compatibility. > > > > As I mentioned above, for..of works on document.all in other browsers. Not > > supporting it would be a compatibility risk IMHO. > > I guess it comes to this question: Does this make the Array API code worse > to support document.all? I am not familiar enough JS to answer this > question. The exact same case came up in bug 144367. I didn't land that because I was okay with ignoring document.all to discourage its use. > For e.g. is the new code slower? `this == null` was chosen over other patterns because it produced noticeably smaller bytecode (see bug 159899). In theory that could help the chances of bigger functions reaching higher tiers if this was inlined into the function. But I know of no test cases that saw performance benefits. > To me, it looks like the cost is very low to support document.all so I did > it. If you disagree though, we can try not supporting for..of on > document.all. While I try to be compatible with other browsers as much as > possible, I think the compatibility risk here would be low given that this > is a legacy API and DOM API iterators are new. Let me know what you think. I > can put up a patch. If you want to keep this, I suggest updating Array.from to work as well (which was bug 144367 that has a near identical code change not addressed here).
Darin Adler
Comment 45 2017-01-22 22:23:53 PST
I’d lean toward not writing special code just for document.all. On the other hand, document.all behavior differently effectively exposes the fact that we used JavaScript and an explicit null check to implement these functions, which is definitely a bad thing. Recall that the document.all trick is trying to trick website JavaScript code. Rather than writing the code differently, I could imagine us doing something at a lower level so that our built-in JavaScript code is not fooled by "masquerade as undefined". Generally, though, it seems bad if document.all has any effect on the JavaScript code we write! Seems like the wrong thing to focus on.
Geoffrey Garen
Comment 46 2017-01-23 10:43:51 PST
> Generally, though, it seems bad if document.all has any effect on the > JavaScript code we write! Seems like the wrong thing to focus on. This is my main concern -- not any specific performance problem (although I agree with Joe's analysis of potential performance knock-on affects), but rather the problem of allowing a weird backwards-compatibility quirk to grow rather than shrink and eventually disappear.
Note You need to log in before you can comment on or make changes to this bug.