getElementsByClassName() should return an HTMLCollection

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

getElementsByClassName() should return an HTMLCollection instead of a NodeList to match to specification, and the behavior of other major browsers.

[Chris Dumez]

Created attachment 259444 [details]
WIP Patch

[WebKit Commit Bot]

Attachment 259444 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:50:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:53:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:54:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 259445 [details]
WIP Patch

[WebKit Commit Bot]

Attachment 259445 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:50:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:53:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:54:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 259445 [details]
WIP Patch

Attachment 259445 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/77902

New failing tests:
inspector/model/remote-object.html

[Build Bot]

Created attachment 259448 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259445 [details]
WIP Patch

Attachment 259445 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/77933

New failing tests:
inspector/model/remote-object.html

[Build Bot]

Created attachment 259451 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 259479 [details]
Patch

[WebKit Commit Bot]

Attachment 259479 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:50:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:53:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:54:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 259479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259479&action=review

> Source/WebCore/dom/ContainerNode.cpp:893
> +RefPtr<HTMLCollection> ContainerNode::getElementsByClassName(const AtomicString& classNames)

Shouldn’t this return a Ref instead of a RefPtr?

> Source/WebCore/dom/ContainerNode.cpp:898
> +RefPtr<NodeListBase> ContainerNode::getElementsByClassNameForObjC(const AtomicString& classNames)

Shouldn’t this return a Ref instead of a RefPtr?

> Source/WebCore/dom/NodeList.h:35
> -class NodeList : public ScriptWrappable, public RefCounted<NodeList> {
> +class NodeList : public NodeListBase, public ScriptWrappable {

Doesn’t this make NodeList objects bigger, because they now use multiple inheritance from classes that both have virtual functions?

> Source/WebCore/dom/NodeListBase.h:35
> +// This is a common base class for NodeList / HTMLCollection to maintain legacy ObjC API compatibility.

I suggest consider making a class derived from NodeList that is an adapter that points to an HTMLCollection, instead of using a base class like this. That would avoid the multiple inheritance that I believe we are introducing.

> Source/WebCore/html/HTMLCollection.h:61
> -class HTMLCollection : public ScriptWrappable, public RefCounted<HTMLCollection> {
> +class HTMLCollection : public NodeListBase, public ScriptWrappable {

Doesn’t this make HTMLCollection objects bigger, because they now use multiple inheritance from classes that both have virtual functions?

[WebKit Commit Bot]

Comment on attachment 259479 [details]
Patch

Clearing flags on attachment: 259479

Committed r188735: <http://trac.webkit.org/changeset/188735>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 259479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259479&action=review

>> Source/WebCore/dom/ContainerNode.cpp:893
>> +RefPtr<HTMLCollection> ContainerNode::getElementsByClassName(const AtomicString& classNames)
> 
> Shouldn’t this return a Ref instead of a RefPtr?

Yes, I'll do this in a follow-up patch.

>> Source/WebCore/dom/ContainerNode.cpp:898
>> +RefPtr<NodeListBase> ContainerNode::getElementsByClassNameForObjC(const AtomicString& classNames)
> 
> Shouldn’t this return a Ref instead of a RefPtr?

Yes, I'll do this in a follow-up patch.

>> Source/WebCore/dom/NodeList.h:35
>> +class NodeList : public NodeListBase, public ScriptWrappable {
> 
> Doesn’t this make NodeList objects bigger, because they now use multiple inheritance from classes that both have virtual functions?

ScriptWrappable does not have any virtual functions, only NodeListBase.

>> Source/WebCore/dom/NodeListBase.h:35
>> +// This is a common base class for NodeList / HTMLCollection to maintain legacy ObjC API compatibility.
> 
> I suggest consider making a class derived from NodeList that is an adapter that points to an HTMLCollection, instead of using a base class like this. That would avoid the multiple inheritance that I believe we are introducing.

Thinking about this, I don't see any reason why HTMLCollection couldn't simply subclass NodeList. I believe it even used to a while back.

>> Source/WebCore/html/HTMLCollection.h:61
>> +class HTMLCollection : public NodeListBase, public ScriptWrappable {
> 
> Doesn’t this make HTMLCollection objects bigger, because they now use multiple inheritance from classes that both have virtual functions?

ScriptWrappable does not have any virtual functions.