[webkit-reviews] review granted: [Bug 121772] CTTE: Use references more in ContainerNode, ContainerNodeAlgorithms and related classes : [Attachment 212314] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 121772: CTTE: Use references more in ContainerNode, ContainerNodeAlgorithms
and related classes
https://bugs.webkit.org/show_bug.cgi?id=121772

Attachment 212314: Patch
https://bugs.webkit.org/attachment.cgi?id=212314&action=review

------- Additional Comments from Andreas Kling <akling at apple.com>
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