It's ridiculous that every JS library is converting the result of querySelectorAll into an array. Since querySelectorAll returns a new NodeList each time, I don't think the backwards compatibility risk is that high. We should make it return a JS array instead.
How would this help with the fact that library code copies out the result into a new array? Are you proposing a spec change, which other browsers would adopt, which would then lead to library authors removing their copying code?
Anne says he can change the spec if we successfully change our implementation.
(In reply to comment #1) > How would this help with the fact that library code copies out the result into a new array? > > Are you proposing a spec change, which other browsers would adopt, which would then lead to library authors removing their copying code? Yes. Also JSC can optimize native JS array operations better than the one involving NodeLists although we should be teaching JSC now to access NodeList fast as well. e.g. jQuery executes: push.apply( results, slice.call( newContext.querySelectorAll( newSelector ), 0 ) ); and optimizing this code is much easier if querySelectorAll simply returned a JS array.
Also note that queryAll in DOM4 returns Elements, which is a subclass of Array so making querySelectorAll return Array or Elements when it's introduced will align the return types of two functions: Excerpts from http://dom.spec.whatwg.org/#collections:-elements interface ParentNode { readonly attribute HTMLCollection children; readonly attribute Element? firstElementChild; readonly attribute Element? lastElementChild; readonly attribute unsigned long childElementCount; void prepend((Node or DOMString)... nodes); void append((Node or DOMString)... nodes); Element? query(DOMString relativeSelectors); [NewObject] Elements queryAll(DOMString relativeSelectors); Element? querySelector(DOMString selectors); [NewObject] NodeList querySelectorAll(DOMString selectors); }; Document implements ParentNode; DocumentFragment implements ParentNode; Element implements ParentNode; class Elements extends Array { Element? query(DOMString relativeSelectors); Elements queryAll(DOMString relativeSelectors); };
Erik (arv) from Google also says they will happily change Blink to match the new behavior if we successfully make this change on our side as well.
I think NodeList / HTMLCollection are now iteratorable so we should be able to fix this bug with that!
<rdar://problem/25731519>
What is left to be done here concretely?
for (var e of document.all) console.log(e) needs to work. Someone from JSC team should be able to tell us what we need to make an object iterable.
(In reply to comment #9) > for (var e of document.all) > console.log(e) > > needs to work. Someone from JSC team should be able to tell us what we need > to make an object iterable. document.all ??
(In reply to comment #10) > (In reply to comment #9) > > for (var e of document.all) > > console.log(e) > > > > needs to work. Someone from JSC team should be able to tell us what we need > > to make an object iterable. > > document.all ?? Oh oops, this is about querySelectorAll, so it should be: for (var e of document.querySelectorAll('*')) console.log(e)
I see, so basically we need to make NodeList iterable to match the spec: [Exposed=Window] interface NodeList { getter Node? item(unsigned long index); readonly attribute unsigned long length; iterable<Node>; // We are missing this. };
Created attachment 277991 [details] WIP
Created attachment 277995 [details] Rebasing WIP
Comment on attachment 277991 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=277991&action=review > Source/WebCore/css/FontFaceSet.cpp:89 > + return RefPtr<FontFace>(m_target->backing()[m_index++].wrapper(state)); What error do we get without the explicit RefPtr<FontFace> typecast? > Source/WebCore/css/FontFaceSet.h:64 > + Optional<RefPtr<FontFace>> next(JSC::ExecState&); I’d like to know more about the type Optional<RefPtr<X>> for this? Is there a distinct null value separate from Nullopt? Can this just be RefPtr and use nullptr to mean null? > Source/WebCore/dom/NodeList.h:34 > +}; Semicolon here is incorrect.
Comment on attachment 277995 [details] Rebasing WIP View in context: https://bugs.webkit.org/attachment.cgi?id=277995&action=review > Source/WebCore/css/FontFaceSet.h:64 > + Optional<RefPtr<FontFace>> next(JSC::ExecState&); Another question about the return type: Does this need to be RefPtr instead of a raw pointer? If so, why?
Comment on attachment 277995 [details] Rebasing WIP Attachment 277995 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1261213 New failing tests: fast/dom/domListEnumeration.html fast/text/font-face-set-javascript.html
Created attachment 277998 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 277995 [details] Rebasing WIP Attachment 277995 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1261227 New failing tests: fast/dom/domListEnumeration.html fast/text/font-face-set-javascript.html
Created attachment 278000 [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 on attachment 277995 [details] Rebasing WIP Attachment 277995 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1261280 New failing tests: fast/dom/domListEnumeration.html fast/text/font-face-set-javascript.html
Created attachment 278001 [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 277995 [details] Rebasing WIP Attachment 277995 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1261287 New failing tests: fast/dom/domListEnumeration.html fast/text/font-face-set-javascript.html
Created attachment 278002 [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.4
> I’d like to know more about the type Optional<RefPtr<X>> for this? Is there > a distinct null value separate from Nullopt? Can this just be RefPtr and use > nullptr to mean null? I wanted to check that and will update the patch if there cannot be a null JS value in the iteration. JSKeyValueIterator updated implementation should just require operator* and operator bool() for set iterators. > > Source/WebCore/css/FontFaceSet.h:64 > > + Optional<RefPtr<FontFace>> next(JSC::ExecState&); > > Another question about the return type: Does this need to be RefPtr instead > of a raw pointer? If so, why? I seem to recall there was a need for it but do not remember that. Probably mmaxfield would know more about this.
Comment on attachment 277995 [details] Rebasing WIP View in context: https://bugs.webkit.org/attachment.cgi?id=277995&action=review > Source/WebCore/dom/NodeList.h:69 > + if (m_index < m_list->length()) This is inefficient because length() can cache the entire node list / collection. Just call item(m_index++) and check the return type instead.
Attachment 277995 [details] did not pass style-queue: ERROR: Source/WebCore/dom/NodeList.h:50: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 278080 [details] Patch
Attachment 278080 [details] did not pass style-queue: ERROR: Source/WebCore/dom/NodeList.h:49: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 278080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278080&action=review > Source/WebCore/dom/NodeList.h:33 > +}; Will i
Comment on attachment 278080 [details] Patch Attachment 278080 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1265801 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html
Created attachment 278084 [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
> > Source/WebCore/css/FontFaceSet.h:64 > > + Optional<RefPtr<FontFace>> next(JSC::ExecState&); > > I’d like to know more about the type Optional<RefPtr<X>> for this? Is there > a distinct null value separate from Nullopt? Can this just be RefPtr and use > nullptr to mean null? I think it can be a RefPtr<> since CSSFontFace.wrapper() is returning a Ref<>. nullptr will mean end of the iteration. CSSFontFace is creating a FontFace (and keeping a WeakPtr). I don't know the reason but this require using a RefPtr<> as returned value to next(). > > Source/WebCore/dom/NodeList.h:34 > > +}; > > Semicolon here is incorrect. OK, forgot to fix it in uploaded patch but will do in next one. > > Source/WebCore/dom/NodeList.h:69 > > + if (m_index < m_list->length()) > > This is inefficient because length() can cache the entire node list / > collection. > Just call item(m_index++) and check the return type instead. OK
Comment on attachment 278080 [details] Patch Attachment 278080 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1265806 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html
Created attachment 278085 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 278080 [details] Patch Attachment 278080 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1265833 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html
Created attachment 278086 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 278080 [details] Patch Attachment 278080 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1265847 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html
Created attachment 278087 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 278088 [details] Fixing key/value order
Attachment 278088 [details] did not pass style-queue: ERROR: Source/WebCore/dom/NodeList.h:49: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 278088 [details] Fixing key/value order View in context: https://bugs.webkit.org/attachment.cgi?id=278088&action=review Looks good. When we have function templates in a header, I think it’s useful to declare the templates first and then define them, so we can look at the declarations and get an overview of what’s present. If we just put all the template definitions in, then the implementation and interface are all mixed up. For bindings code, I think it’s valuable to use generic programming techniques. This means that we want to use overloading rather than naming the types of the arguments in function names, letting the type system work for us to make our code less type specific. That’s why there are a lot of pre-existing functions named "toJS", and I think that "iteratorValueToJS" should just be named toJS. Or some other clearer name, but not one that specifies which types are involved. I suggest we also consider giving fillForEachArgumentsWithIteratorValue a shorter name that doesn’t name the type involved. Maybe appendForEachArguments. > Source/WebCore/dom/NodeList.h:49 > + explicit Iterator(NodeList& list) : m_list(list) { } I think this can probably be private. I believe that the outer class gets access to the private members of classes that are nested inside it.
(In reply to comment #42) > Comment on attachment 278088 [details] > Fixing key/value order > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278088&action=review > > Looks good. > > When we have function templates in a header, I think it’s useful to declare > the templates first and then define them, so we can look at the declarations > and get an overview of what’s present. If we just put all the template > definitions in, then the implementation and interface are all mixed up. > > For bindings code, I think it’s valuable to use generic programming > techniques. This means that we want to use overloading rather than naming > the types of the arguments in function names, letting the type system work > for us to make our code less type specific. That’s why there are a lot of > pre-existing functions named "toJS", and I think that "iteratorValueToJS" > should just be named toJS. Or some other clearer name, but not one that > specifies which types are involved. I suggest we also consider giving > fillForEachArgumentsWithIteratorValue a shorter name that doesn’t name the > type involved. Maybe appendForEachArguments. > > > Source/WebCore/dom/NodeList.h:49 > > + explicit Iterator(NodeList& list) : m_list(list) { } > > I think this can probably be private. I believe that the outer class gets > access to the private members of classes that are nested inside it. Thanks, I'll try to improve the patch accordingly. There is an additional thing which I am not yet sure about. Currently, DOMClass::Iterators are keeping a Ref<> to the iterated object. I wonder whether we might want to move that to a RefPtr<>. That would allow releasing the ref when the iterator is done. That would also ensure that a "done: true" iterator could never be "done: false" later on, like in the case of an inserted node in NodeList or if calling NodeList::Iterator::next() enough times so that the iterator index is back to zero.
I'll tackle this issue in a follow up patch, maybe at JSDomIterator level.
Created attachment 278335 [details] Patch
Attachment 278335 [details] did not pass style-queue: ERROR: Source/WebCore/dom/NodeList.h:49: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 278335 [details] Patch Attachment 278335 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1283661 New failing tests: inspector/console/command-line-api.html
Created attachment 278338 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 278335 [details] Patch Attachment 278335 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1283657 New failing tests: inspector/console/command-line-api.html
Created attachment 278339 [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 278335 [details] Patch Attachment 278335 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1283666 New failing tests: inspector/console/command-line-api.html
Created attachment 278340 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 278404 [details] Fixing for-of loop for set iterators
Attachment 278404 [details] did not pass style-queue: ERROR: Source/WebCore/dom/NodeList.h:45: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #53) > Created attachment 278404 [details] > Fixing for-of loop for set iterators This patch does not provide the correct behavior for entries and forEach for set iterators (a counter value should be provided). Also, it does not ensure done iterators remain done. I plan to fix these two limitations in a follow-up patch.
Comment on attachment 278404 [details] Fixing for-of loop for set iterators View in context: https://bugs.webkit.org/attachment.cgi?id=278404&action=review Do we really need to call this createDOMIterator and domIteratorForEach? Maybe instead iteratorCreate and iteratorForEach? > Source/WebCore/bindings/js/JSDOMBinding.h:647 > +template<typename Value1, typename Value2> inline JSC::JSValue jsPair(JSC::ExecState& state, JSDOMGlobalObject* globalObject, const Value1& value1, const Value2& value2) The types should not be named "Value".
Created attachment 278482 [details] Patch for landing
(In reply to comment #56) > Comment on attachment 278404 [details] > Fixing for-of loop for set iterators > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278404&action=review > > Do we really need to call this createDOMIterator and domIteratorForEach? > Maybe instead iteratorCreate and iteratorForEach? OK. > > Source/WebCore/bindings/js/JSDOMBinding.h:647 > > +template<typename Value1, typename Value2> inline JSC::JSValue jsPair(JSC::ExecState& state, JSDOMGlobalObject* globalObject, const Value1& value1, const Value2& value2) > > The types should not be named "Value". Updated to FirstType/SecondType.
Comment on attachment 278482 [details] Patch for landing Clearing flags on attachment: 278482 Committed r200619: <http://trac.webkit.org/changeset/200619>
All reviewed patches have been landed. Closing bug.
Comment on attachment 278482 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=278482&action=review > LayoutTests/fast/dom/nodeListIterator-expected.txt:12 > +FAIL forEachIndex should be 0 (of type number). Was [object HTMLDivElement] (of type object). What are all these FAIL lines?
This patch seems to have broken m.yahoo.co.jp (content does not load at the bottom of the page). JS Error: [Error] TypeError: this.isCollectedElem is not a function. (In 'this.isCollectedElem(b)', 'this.isCollectedElem' is undefined) (anonymous function) (rappie_stream-1.8.0.js:4:6139) forEach forEach (rappie_stream-1.8.0.js:3:1710) collectTargets (rappie_stream-1.8.0.js:4:6018) (anonymous function) (rappie_stream-1.8.0.js:4:5450) forEach forEach (rappie_stream-1.8.0.js:3:1710) initRapid (rappie_stream-1.8.0.js:4:5380) r (rappie_stream-1.8.0.js:4:4024) (anonymous function) (rappie_stream-1.8.0.js:4:11207) (anonymous function) (rappie_stream-1.8.0.js:4:11497) Global Code (rappie_stream-1.8.0.js:4:11500) [Error] TypeError: undefined is not an object (evaluating 'RappieStream.setNewPageParameter') initializer (mtop-page-top.js:657:2451) _initHierarchy (mtop-page-top.js:197:3768) _baseInit (mtop-page-top.js:197:1101) init (mtop-page-top.js:205:704) _initBase (mtop-page-top.js:197:954) v (mtop-page-top.js:197:105) o (mtop-page-top.js:213) n (mtop-page-top.js:279) r (mtop-page-top.js:639:3635) o (mtop-page-top.js:657:1955) initializer (mtop-page-top.js:655:288) _initHierarchy (mtop-page-top.js:197:3768) _baseInit (mtop-page-top.js:197:1101) init (mtop-page-top.js:205:704) _initBase (mtop-page-top.js:197:954) v (mtop-page-top.js:197:105) o (mtop-page-top.js:213) n (mtop-page-top.js:279) r (mtop-page-top.js:639:3635) r (mtop-page-top.js:654:5182) initializer (mtop-page-top.js:639:3251) r (mtop-page-top.js:639:1784) (anonymous function) (mtop-page-top.js:676) _notify (mtop-page-top.js:9:1497) (anonymous function) (mtop-page-top.js:9:974) _notify (mtop-page-top.js:9:1497) T (mtop-page-top.js:9:2429) _finish (mtop-page-top.js:16:4858) _onSuccess (mtop-page-top.js:16:5375) p (mtop-page-top.js:17:1326) _insert (mtop-page-top.js:17:1390) (anonymous function) (mtop-page-top.js:17:2386) _continue (mtop-page-top.js:17:2273) insert (mtop-page-top.js:17:2412) _use (mtop-page-top.js:9:2988) use (mtop-page-top.js:9:948) Global Code (mtop-page-top.js:676) The page falls back nicely if forEach is not available: forEach: function(a, b, c) { if (a.forEach) a.forEach(b, c); else for (var d = 0; d < a.length; d++) b.call(c, a[d], d, a) }, So presumably the if and else code paths are currently not equivalent. c.f. rdar://problem/26812953
Reverted r200619 for reason: This incompleted feature broke http://m.yahoo.co.jp. Roll it out together with r200678. Committed r202303: <http://trac.webkit.org/changeset/202303>
Reverted r202302, r202303, r202305, and r202306 for reason: Roll out the rollouts because of breaking the build. Committed r202307: <http://trac.webkit.org/changeset/202307>
(In reply to comment #61) > Comment on attachment 278482 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278482&action=review > > > LayoutTests/fast/dom/nodeListIterator-expected.txt:12 > > +FAIL forEachIndex should be 0 (of type number). Was [object HTMLDivElement] (of type object). > > What are all these FAIL lines? This was fixed in bug 157453.
It might be that support for thisArg is missing. Let me check further.
(In reply to comment #66) > It might be that support for thisArg is missing. Let me check further. I filed bug 159020 for that purpose.
Can we close this one and bug 157453?
(In reply to comment #68) > Can we close this one and bug 157453? Yes, they eventually weren't rolled out after all.
Jiewen will follow up if issues remain but things looked good to me after your fix.