Bug 121772 - CTTE: Use references more in ContainerNode, ContainerNodeAlgorithms and related classes
Summary: CTTE: Use references more in ContainerNode, ContainerNodeAlgorithms and relat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-22 18:16 PDT by Sam Weinig
Modified: 2013-09-22 20:39 PDT (History)
2 users (show)

See Also:


Attachments
Patch (96.45 KB, patch)
2013-09-22 18:23 PDT, Sam Weinig
kling: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2013-09-22 18:16:11 PDT
CTTE: Use references more in ContainerNode, ContainerNodeAlgorithms and related classes
Comment 1 Sam Weinig 2013-09-22 18:23:30 PDT
Created attachment 212314 [details]
Patch
Comment 2 WebKit Commit Bot 2013-09-22 18:27:11 PDT
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 3 Early Warning System Bot 2013-09-22 18:36:09 PDT
Comment on attachment 212314 [details]
Patch

Attachment 212314 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/2030109
Comment 4 Early Warning System Bot 2013-09-22 18:38:36 PDT
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 5 Andreas Kling 2013-09-22 18:42:47 PDT
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
Comment 6 Sam Weinig 2013-09-22 20:39:34 PDT
Committed r156256: <http://trac.webkit.org/changeset/156256>