[webkit-reviews] review granted: [Bug 215673] Make it possible to create a WeakPtr to Node and use it store assigned nodes in SlotAssignment : [Attachment 407257] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 215673: Make it possible to create a WeakPtr to Node and use it store
assigned nodes in SlotAssignment
https://bugs.webkit.org/show_bug.cgi?id=215673

Attachment 407257: Patch

https://bugs.webkit.org/attachment.cgi?id=407257&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 407257
  --> https://bugs.webkit.org/attachment.cgi?id=407257
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407257&action=review

I’m going to say review+ but I am surprised that we can afford this much extra
memory for every text node.

If we can afford this extra memory, then it seems like we can cut down the code
size and complexity of accessibility code by taking advantage.
AXObjectCache::remove has a lot of code in it to respond to the deletion of a
node. Needs to remove it from each of many separate data structures have nodes
as keys or values. AccessibilityNodeObject::m_node is a raw pointer that is
nulled out by a hand-written WeakPtr implementation in a function named
"detachRemoteParts".

> Source/WebCore/html/HTMLSlotElement.cpp:122
> +    for (const WeakPtr<Node>& nodePtr : *assignedNodes) {

auto&

> Source/WebCore/html/HTMLSlotElement.cpp:125
> +	       ASSERT_NOT_REACHED();

Why can we assert this?

> Source/WebCore/html/HTMLSlotElement.cpp:157
> -    return assignedNodes->map([] (Node* node) { return makeRef(*node); });
> +
> +    Vector<Ref<Node>> nodes;
> +    nodes.reserveInitialCapacity(assignedNodes->size());
> +    for (auto& nodePtr : *assignedNodes) {
> +	   auto* node = nodePtr.get();
> +	   if (UNLIKELY(!node))
> +	       continue;
> +	   nodes.uncheckedAppend(*node);
> +    }
> +
> +    return nodes;

I’d like us to create a Vector::map variant that allows removal of the nodes
during the mapping process. I included this feature in makeVector, although the
design isn’t super-elegant, and have found it to often be useful.