Bug 121772 – CTTE: Use references more in ContainerNode, ContainerNodeAlgorithms and related classes

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

CTTE: Use references more in ContainerNode, ContainerNodeAlgorithms and related classes

[Sam Weinig]

Created attachment 212314 [details]
Patch

[WebKit Commit Bot]

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.

[Early Warning System Bot]

Comment on attachment 212314 [details]
Patch

Attachment 212314 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/2030109

[Early Warning System Bot]

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

[Andreas Kling]

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

[Sam Weinig]

Committed r156256: <http://trac.webkit.org/changeset/156256>