DOM4 and HTML5 specifications make HTMLFormControlsCollection and HTMLOptionsCollection’s namedItem and named getter return a node list when there are multiple matches. However, by default HTMLCollection’s namedItem and named getter returns the first item that matched the name, and this behavior can be much more efficient since it can stop the search as soon as it finds the first match. We should introduce the former two interfaces (they’re currently both implemented as HTMLCollection in WebKit) so that we can confine the said obnoxious, inefficient, behavior to those two interfaces.
HTMLCollection specification: http://www.w3.org/TR/domcore/#htmlcollection HTML*Collection specifications: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#collections-0
Created attachment 172507 [details] Work in progress Somehow v8 binding is broken and I don’t know why. I’m gonna just give up for now and come back to it later.
Comment on attachment 172507 [details] Work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=172507&action=review > Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp:54 > + return v8Undefined(); You have the isolate. You should pass it to v8Undefined. > Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp:67 > + return v8Undefined(); ditto
(In reply to comment #1) > HTMLCollection specification: > http://www.w3.org/TR/domcore/#htmlcollection That's an obsolete link, the up to date link is: http://dom.spec.whatwg.org/
Created attachment 172670 [details] Patch
Created attachment 172672 [details] Patch
I’m getting one test failure like this on fast/dom/wrapper-classes.html: -PASS jsWrapperClass(document.createElement('select').options.constructor) is 'HTMLOptionsCollectionConstructor' +FAIL jsWrapperClass(document.createElement('select').options.constructor) should be HTMLOptionsCollectionConstructor. Was Function. As far as I can tell HTMLCollection has the same failure above. Does anyone know how to fix this?
Attachment 172672 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/html/HTMLFormControlsCollection.h:55: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 1 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 172672 [details] Patch Attachment 172672 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14745493
Comment on attachment 172672 [details] Patch Attachment 172672 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14761140
Comment on attachment 172672 [details] Patch Attachment 172672 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14760170
Created attachment 172703 [details] Fixed builds
Comment on attachment 172703 [details] Fixed builds Attachment 172703 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14733935
7>c1xx : fatal error C1083: Cannot open source file: '..\html\HTMLFormCollection.cpp': No such file or directory 7>IDBFactoryBackendInterface.cpp I can fix that easily :) once the review is done.
Comment on attachment 172703 [details] Fixed builds Attachment 172703 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14747651 New failing tests: fast/dom/dom-constructors.html fast/dom/wrapper-classes.html inspector/console/console-format-collections.html
Comment on attachment 172703 [details] Fixed builds Attachment 172703 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14765020 New failing tests: dom/html/level2/html/HTMLCollection06.html dom/html/level2/html/HTMLCollection04.html dom/xhtml/level2/html/HTMLCollection09.xhtml dom/xhtml/level2/html/HTMLCollection05.xhtml dom/xhtml/level2/html/HTMLCollection07.xhtml dom/html/level2/html/HTMLDocument07.html dom/xhtml/level2/html/HTMLCollection04.xhtml dom/html/level2/html/HTMLCollection10.html dom/html/level2/html/HTMLDocument08.html dom/xhtml/level2/html/HTMLCollection11.xhtml dom/html/level2/html/HTMLCollection12.html dom/html/level2/html/HTMLCollection09.html dom/html/level2/html/HTMLDocument09.html dom/html/level2/html/HTMLDocument10.html dom/xhtml/level2/html/HTMLCollection12.xhtml dom/xhtml/level2/html/HTMLCollection03.xhtml dom/xhtml/level2/html/HTMLCollection08.xhtml dom/html/level2/html/HTMLCollection11.html dom/xhtml/level2/html/HTMLCollection10.xhtml dom/xhtml/level2/html/HTMLCollection01.xhtml dom/html/level2/html/HTMLCollection08.html dom/html/level2/html/HTMLDocument11.html dom/html/level2/html/HTMLCollection02.html dom/html/level2/html/HTMLCollection03.html dom/xhtml/level2/html/HTMLCollection06.xhtml dom/html/level2/html/HTMLCollection07.html dom/html/level2/html/HTMLCollection01.html dom/html/level2/html/HTMLCollection05.html dom/html/level2/html/AppletsCollection.html dom/xhtml/level2/html/HTMLCollection02.xhtml
Comment on attachment 172703 [details] Fixed builds View in context: https://bugs.webkit.org/attachment.cgi?id=172703&action=review > Source/WebCore/page/DOMWindow.idl:473 > + attribute HTMLFormControlsCollectionConstructor HTMLFormControlsCollection; > + attribute HTMLOptionsCollectionConstructor HTMLOptionsCollection; I guess the failures are caused by this change. Once you expose HTMLOptionsCollection to DOMWindow, window.HTMLOptionsCollection should look like a Function: function HTMLOptionsCollection() { [native code] } So the right way to fix the problem would be just to update the test cases. You can write a test that just checks if (an instance of HTMLOptionsCollection).constructor === HTMLOptionsCollection.
(In reply to comment #3) > (From update of attachment 172507 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172507&action=review > > > Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp:54 > > + return v8Undefined(); > > You have the isolate. You should pass it to v8Undefined. > > > Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp:67 > > + return v8Undefined(); > > ditto Unfortunately, doing this in namedPropertyGetter results in not falling back to access properties :( So I’m reverting that back.
Created attachment 172864 [details] Reverted v8::Undefined(isolate) change to fix tests
Comment on attachment 172864 [details] Reverted v8::Undefined(isolate) change to fix tests Attachment 172864 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14745814
Comment on attachment 172864 [details] Reverted v8::Undefined(isolate) change to fix tests Attachment 172864 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14771135 New failing tests: fast/dom/htmlcollection-reachable.html fast/dom/html-collections-named-getter.html fast/dom/gc-9.html inspector/console/console-format-collections.html
Created attachment 173182 [details] Fixed more tests
Comment on attachment 173182 [details] Fixed more tests View in context: https://bugs.webkit.org/attachment.cgi?id=173182&action=review > Source/WebCore/ChangeLog:10 > + there are multiple matches. Introducing these two interfaces allow us to "regular" HTMLCollection's named getter typo > LayoutTests/fast/dom/html-collections-named-getter.html:30 > +var newDoc = document.implementation.createDocument(null, 'html', null); This is never used > LayoutTests/fast/dom/html-collections-named-getter.html:101 > +var successfullyParsed = true; No need for this line > LayoutTests/fast/dom/wrapper-classes-expected.txt:145 > +FAIL jsWrapperClass(document.createElement('select').options.constructor) should be HTMLOptionsCollectionConstructor. Was Function. How come this broke by this change?
(In reply to comment #23) > (From update of attachment 173182 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173182&action=review > > > Source/WebCore/ChangeLog:10 > > + there are multiple matches. Introducing these two interfaces allow us to "regular" HTMLCollection's named getter > > typo Where? > > LayoutTests/fast/dom/html-collections-named-getter.html:30 > > +var newDoc = document.implementation.createDocument(null, 'html', null); > > This is never used Will remove. > > LayoutTests/fast/dom/html-collections-named-getter.html:101 > > +var successfullyParsed = true; > > No need for this line Nah… I’d like to keep it there. > > LayoutTests/fast/dom/wrapper-classes-expected.txt:145 > > +FAIL jsWrapperClass(document.createElement('select').options.constructor) should be HTMLOptionsCollectionConstructor. Was Function. > > How come this broke by this change? See haraken’s comment #17.
Comment on attachment 173182 [details] Fixed more tests View in context: https://bugs.webkit.org/attachment.cgi?id=173182&action=review >>> Source/WebCore/ChangeLog:10 >>> + there are multiple matches. Introducing these two interfaces allow us to "regular" HTMLCollection's named getter >> >> typo > > Where? allow us to "regular"... >>> LayoutTests/fast/dom/html-collections-named-getter.html:101 >>> +var successfullyParsed = true; >> >> No need for this line > > Nah… I’d like to keep it there. Why? This has no effect.; js-test-*.js takes care of successfullyParsed for you already. If there is a parse error js-test-* will take care of it.
Comment on attachment 173182 [details] Fixed more tests View in context: https://bugs.webkit.org/attachment.cgi?id=173182&action=review >>>> Source/WebCore/ChangeLog:10 >>>> + there are multiple matches. Introducing these two interfaces allow us to "regular" HTMLCollection's named getter >>> >>> typo >> >> Where? > > allow us to "regular"... regular is spelled correctly, right? I precisely meant http://www.answers.com/topic/regular.
Created attachment 173345 [details] Synced to ToT
(In reply to comment #26) > (From update of attachment 173182 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173182&action=review > > >>>> Source/WebCore/ChangeLog:10 > >>>> + there are multiple matches. Introducing these two interfaces allow us to "regular" HTMLCollection's named getter > >>> > >>> typo > >> > >> Where? > > > > allow us to "regular"... > > regular is spelled correctly, right? I precisely meant http://www.answers.com/topic/regular. not a typo... just grammar problem. You need a verb somewhere.
Comment on attachment 173182 [details] Fixed more tests View in context: https://bugs.webkit.org/attachment.cgi?id=173182&action=review >>>>>> Source/WebCore/ChangeLog:10 >>>>>> + there are multiple matches. Introducing these two interfaces allow us to "regular" HTMLCollection's named getter >>>>> >>>>> typo >>>> >>>> Where? >>> >>> allow us to "regular"... >> >> regular is spelled correctly, right? I precisely meant http://www.answers.com/topic/regular. > > not a typo... just grammar problem. You need a verb somewhere. Oh, I see. Yeah, it’s missing "make" there.
Comment on attachment 173345 [details] Synced to ToT Attachment 173345 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14771839 New failing tests: inspector/console/console-format-collections.html
Comment on attachment 173345 [details] Synced to ToT Attachment 173345 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14775752 New failing tests: inspector/console/console-format-collections.html
Created attachment 174813 [details] Patch
Comment on attachment 174813 [details] Patch Attachment 174813 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14875291 New failing tests: inspector/console/console-format-collections.html
Comment on attachment 174813 [details] Patch Attachment 174813 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14861514 New failing tests: inspector/console/console-format-collections.html
Comment on attachment 174813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174813&action=review > Source/WebCore/ChangeLog:59 > + (WebCore): Stray change log line here should be removed. > Source/WebCore/ChangeLog:63 > + (WebCore): Stray change log line here should be removed. > Source/WebCore/ChangeLog:70 > + (WebCore): Stray change log line here should be removed. > Source/WebCore/ChangeLog:74 > + (WebCore): Removed Document::objects since it was not used anywhere. Comment should be here, but not the (WebCore): prefix. > Source/WebCore/ChangeLog:76 > + (Document): Stray change log line here should be removed. > Source/WebCore/ChangeLog:96 > + (HTMLFormControlsCollection): Stray change log line here should be removed. > Source/WebCore/ChangeLog:100 > + (WebCore): Stray change log line here should be removed. > Source/WebCore/html/HTMLFormControlsCollection.h:34 > // This class is just a big hack to find form elements even in malformed HTML elements. Should add a blank line before this comment. > LayoutTests/fast/dom/wrapper-classes-expected.txt:145 > -PASS jsWrapperClass(document.createElement('select').options.constructor) is 'HTMLOptionsCollectionConstructor' > +FAIL jsWrapperClass(document.createElement('select').options.constructor) should be HTMLOptionsCollectionConstructor. Was Function. I don’t see any comment about this new expected failure in the change log.
Thanks for the review. Will address the comments before I land the patch.
Committed r135093: <http://trac.webkit.org/changeset/135093>
It appears global-constructors.html needs updated expectation after this patch: --- /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/fast/js/global-constructors-expected.txt +++ /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/fast/js/global-constructors-actual.txt @@ -75,6 +75,7 @@ PASS HTMLEmbedElement.toString() is '[object HTMLEmbedElementConstructor]' PASS HTMLFieldSetElement.toString() is '[object HTMLFieldSetElementConstructor]' PASS HTMLFontElement.toString() is '[object HTMLFontElementConstructor]' +PASS HTMLFormControlsCollection.toString() is '[object HTMLFormControlsCollectionConstructor]' PASS HTMLFormElement.toString() is '[object HTMLFormElementConstructor]' PASS HTMLFrameElement.toString() is '[object HTMLFrameElementConstructor]' PASS HTMLFrameSetElement.toString() is '[object HTMLFrameSetElementConstructor]' @@ -102,6 +103,7 @@ PASS HTMLObjectElement.toString() is '[object HTMLObjectElementConstructor]' PASS HTMLOptGroupElement.toString() is '[object HTMLOptGroupElementConstructor]' PASS HTMLOptionElement.toString() is '[object HTMLOptionElementConstructor]' +PASS HTMLOptionsCollection.toString() is '[object HTMLOptionsCollectionConstructor]' PASS HTMLOutputElement.toString() is '[object HTMLOutputElementConstructor]' PASS HTMLParagraphElement.toString() is '[object HTMLParagraphElementConstructor]' PASS HTMLParamElement.toString() is '[object HTMLParamElementConstructor]'