Bug 147980

Created attachment 259444 [details] WIP Patch

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.

Created attachment 259445 [details] WIP Patch

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

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

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

Created attachment 259479 [details] Patch

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 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 on attachment 259479 [details] Patch Clearing flags on attachment: 259479 Committed r188735: <http://trac.webkit.org/changeset/188735>

All reviewed patches have been landed. Closing bug.

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.