Bug 147980 - getElementsByClassName() should return an HTMLCollection
Summary: getElementsByClassName() should return an HTMLCollection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://dom.spec.whatwg.org/#document
Keywords: WebExposed
Depends on: 147979 148039 148080 148092 148116
Blocks: 148117
  Show dependency treegraph
 
Reported: 2015-08-13 09:27 PDT by Chris Dumez
Modified: 2015-08-20 22:02 PDT (History)
8 users (show)

See Also:


Attachments
WIP Patch (43.36 KB, patch)
2015-08-19 21:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (44.73 KB, patch)
2015-08-19 21:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (556.81 KB, application/zip)
2015-08-19 22:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (605.03 KB, application/zip)
2015-08-19 22:44 PDT, Build Bot
no flags Details
Patch (52.75 KB, patch)
2015-08-20 10:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-08-13 09:27:17 PDT
getElementsByClassName() should return an HTMLCollection instead of a NodeList to match to specification, and the behavior of other major browsers.
Comment 1 Chris Dumez 2015-08-19 21:26:20 PDT
Created attachment 259444 [details]
WIP Patch
Comment 2 WebKit Commit Bot 2015-08-19 21:28:10 PDT
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.
Comment 3 Chris Dumez 2015-08-19 21:33:32 PDT
Created attachment 259445 [details]
WIP Patch
Comment 4 WebKit Commit Bot 2015-08-19 21:35:28 PDT
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.
Comment 5 Build Bot 2015-08-19 22:16:36 PDT
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
Comment 6 Build Bot 2015-08-19 22:16:41 PDT
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
Comment 7 Build Bot 2015-08-19 22:44:18 PDT
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
Comment 8 Build Bot 2015-08-19 22:44:21 PDT
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
Comment 9 Chris Dumez 2015-08-20 10:41:29 PDT
Created attachment 259479 [details]
Patch
Comment 10 WebKit Commit Bot 2015-08-20 10:44:21 PDT
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.
Comment 11 Darin Adler 2015-08-20 18:13:28 PDT
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?
Comment 12 WebKit Commit Bot 2015-08-20 19:10:39 PDT
Comment on attachment 259479 [details]
Patch

Clearing flags on attachment: 259479

Committed r188735: <http://trac.webkit.org/changeset/188735>
Comment 13 WebKit Commit Bot 2015-08-20 19:10:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Chris Dumez 2015-08-20 21:06:16 PDT
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.