CTTE: Use references more in ContainerNode, ContainerNodeAlgorithms and related classes
Created attachment 212314 [details] Patch
Attachment 212314 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp', u'Source/WebCore/dom/CharacterData.cpp', u'Source/WebCore/dom/ChildListMutationScope.cpp', u'Source/WebCore/dom/ChildListMutationScope.h', u'Source/WebCore/dom/ContainerNode.cpp', u'Source/WebCore/dom/ContainerNode.h', u'Source/WebCore/dom/ContainerNodeAlgorithms.cpp', u'Source/WebCore/dom/ContainerNodeAlgorithms.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/MutationObserverInterestGroup.cpp', u'Source/WebCore/dom/MutationObserverInterestGroup.h', u'Source/WebCore/dom/MutationRecord.cpp', u'Source/WebCore/dom/MutationRecord.h', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Range.cpp', u'Source/WebCore/dom/Range.h', u'Source/WebCore/dom/StaticNodeList.cpp', u'Source/WebCore/dom/StaticNodeList.h', u'Source/WebCore/editing/ApplyStyleCommand.cpp', u'Source/WebCore/editing/CompositeEditCommand.cpp', u'Source/WebCore/editing/ReplaceNodeWithSpanCommand.cpp', u'Source/WebCore/editing/markup.cpp', u'Source/WebCore/html/HTMLFrameElementBase.cpp', u'Source/WebCore/html/HTMLFrameOwnerElement.cpp', u'Source/WebCore/html/HTMLFrameOwnerElement.h', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLObjectElement.cpp', u'Source/WebCore/html/parser/HTMLTreeBuilder.cpp', u'Source/WebCore/svg/SVGElementInstance.cpp', u'Source/WebCore/svg/SVGElementInstance.h', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/xml/XMLErrors.cpp']" exit_code: 1 Source/WebCore/dom/StaticNodeList.h:42: Missing spaces around >> [whitespace/operators] [3] Source/WebCore/svg/SVGElementInstance.h:32: The parameter name "container" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGElementInstance.h:162: The parameter name "container" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGElementInstance.h:168: The parameter name "container" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/dom/ContainerNodeAlgorithms.h:164: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 212314 [details] Patch Attachment 212314 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/2030109
Comment on attachment 212314 [details] Patch Attachment 212314 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1907566
Comment on attachment 212314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212314&action=review Awesome! r=me > Source/WebCore/dom/ContainerNodeAlgorithms.h:53 > + Vector<RefPtr<Node>> m_postInsertionNotificationTargets; Could this be a Vector of Refs? > Source/WebCore/dom/ContainerNodeAlgorithms.h:283 > Vector<RefPtr<HTMLFrameOwnerElement>, 10> m_frameOwners; Could probably be a Vector of Refs too! > Source/WebCore/editing/markup.cpp:1070 > void replaceChildrenWithFragment(ContainerNode* container, PassRefPtr<DocumentFragment> fragment, ExceptionCode& ec) > { > - RefPtr<ContainerNode> containerNode(container); > - > + Ref<ContainerNode> containerNode(*container); 'container' should be a ContainerNode&, then we wouldn't have the sketchy-looking dereference. All call sites just pass "this" anyway. > Source/WebCore/editing/markup.cpp:1094 > void replaceChildrenWithText(ContainerNode* container, const String& text, ExceptionCode& ec) > { > - RefPtr<ContainerNode> containerNode(container); > - > + Ref<ContainerNode> containerNode(*container); 'container' should be a ContainerNode&, then we wouldn't have the sketchy-looking dereference. All call sites just pass "this" anyway. > Source/WebCore/html/HTMLFrameOwnerElement.cpp:131 > + for (Node* node = &owner; node; node = node->parentOrShadowHostNode()) { Node -> ContainerNode > Source/WebCore/html/HTMLFrameOwnerElement.h:108 > + Node& m_root; This should be a ContainerNode&. > Source/WebCore/testing/Internals.cpp:-1261 > - copyToVector(result.rectBasedTestResult(), matches); It would be better to make copyToVector() work for Vector<Ref<T>>. copyToVector() is pretty stupid; instead of doing a resize() and then operator='ing all the values into place, it should do reserveInitialCapacity/uncheckedAppend. > Source/WebCore/testing/Internals.cpp:1263 > + matches.reserveCapacity(nodeSet.size()); reserveInitialCapacity > Source/WebCore/testing/Internals.cpp:1265 > + matches.append(*it->get()); uncheckedAppend > Source/WebCore/xml/XMLErrors.cpp:140 > - documentElement->parentNode()->parserRemoveChild(documentElement.get()); > + documentElement->parentNode()->parserRemoveChild(*documentElement.get()); *documentElement
Committed r156256: <http://trac.webkit.org/changeset/156256>