[webkit-reviews] review granted: [Bug 58622] Make JSNodeFilterCondition handle its lifetime correctly : [Attachment 89733] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 58622: Make JSNodeFilterCondition handle its lifetime correctly
https://bugs.webkit.org/show_bug.cgi?id=58622

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

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

Though it works, it seems a little strange to treat the filter condition's weak
owner data member as the filter's opaque root. The idea of an opaque root is
that it's the root of the DOM data structure that owns the weak handle. But
here, you're using an internal data member of a leaf in the data structure.

I think it would be a little clearer if JSNodeFilterCondition took its
NodeFilter owner as a constructor argument, and used that as its opaque root.
Then, you could remove the markAggregate function from JSNodeFilterCondition,
and just have NodeFilter's markAggregate add itself as an opaque root.

I'll say r+ because this patch works as is, but I think you should make that
change before landing.

> Source/WebCore/bindings/js/JSNodeFilterCondition.h:48
> +	   WeakOwner m_weakOwner;

I would call this m_filterWeakOwner, to specify what it owns.