Bug 101311 – Make namedItem return a node list only in HTMLFormControlsCollection and HTMLOptionsCollection

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Adam Barth]

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

[Ian 'Hixie' Hickson]

(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/

[Ryosuke Niwa]

Created attachment 172670 [details]
Patch

[Ryosuke Niwa]

Created attachment 172672 [details]
Patch

[Ryosuke Niwa]

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?

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 172672 [details]
Patch

Attachment 172672 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14745493

[Build Bot]

Comment on attachment 172672 [details]
Patch

Attachment 172672 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14761140

[Peter Beverloo (cr-android ews)]

Comment on attachment 172672 [details]
Patch

Attachment 172672 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14760170

[Ryosuke Niwa]

Created attachment 172703 [details]
Fixed builds

[Build Bot]

Comment on attachment 172703 [details]
Fixed builds

Attachment 172703 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14733935

[Ryosuke Niwa]

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.

[Build Bot]

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

[WebKit Review Bot]

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

[Kentaro Hara]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 172864 [details]
Reverted v8::Undefined(isolate) change to fix tests

[Build Bot]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

Created attachment 173182 [details]
Fixed more tests

[Erik Arvidsson]

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?

[Ryosuke Niwa]

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

[Erik Arvidsson]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 173345 [details]
Synced to ToT

[Erik Arvidsson]

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

[Ryosuke Niwa]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

Created attachment 174813 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Darin Adler]

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.

[Ryosuke Niwa]

Thanks for the review. Will address the comments before I land the patch.

[Ryosuke Niwa]

Committed r135093: <http://trac.webkit.org/changeset/135093>

[Chris Dumez]

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]'