[webkit-reviews] review denied: [Bug 34231] XPathResult should keep its node set's JS wrappers alive : [Attachment 91808] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 34231: XPathResult should keep its node set's JS wrappers alive
https://bugs.webkit.org/show_bug.cgi?id=34231

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=91808&action=review

The tests here are great, and I think this patch would work as-is, but I think
it's worth it to implement the optimization I suggested above.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:49
> +    const XPath::Value& xpathValue = impl()->value();
> +    if (xpathValue.isNodeSet()) {
> +	   const XPath::NodeSet& nodesToMark = xpathValue.toNodeSet();
> +	   for (size_t i = 0; i < nodesToMark.size(); ++i) {
> +	       Node* node = nodesToMark[i];
> +	       visitor.addOpaqueRoot(root(node));
> +	   }

I think you can make this more efficient.

All nodes in an XPathResult are in the same document, which is
XPathResult::m_document, and iteration is only allowed so long as the document
has not changed. Therefore, you can just do this:

visitor.addOpaqueRoot(impl()->document());

and this:

class XPathResult {
...
    Document* document() { return m_document.get(); }
};