Bug 58622 - Make JSNodeFilterCondition handle its lifetime correctly
Summary: Make JSNodeFilterCondition handle its lifetime correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-14 18:23 PDT by Oliver Hunt
Modified: 2011-04-15 12:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.18 KB, patch)
2011-04-14 18:24 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (8.12 KB, patch)
2011-04-14 21:39 PDT, Oliver Hunt
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-04-14 18:23:04 PDT
Make JSNodeFilterCondition handle its lifetime correctly
Comment 1 Oliver Hunt 2011-04-14 18:24:58 PDT
Created attachment 89713 [details]
Patch
Comment 2 Build Bot 2011-04-14 20:06:17 PDT
Attachment 89713 [details] did not build on win:
Build output: http://queues.webkit.org/results/8442029
Comment 3 Oliver Hunt 2011-04-14 21:39:07 PDT
Created attachment 89733 [details]
Patch
Comment 4 Geoffrey Garen 2011-04-15 09:04:40 PDT
Comment on attachment 89733 [details]
Patch

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.
Comment 5 Geoffrey Garen 2011-04-15 09:11:05 PDT
(In reply to comment #4)
> Then, you could remove the markAggregate function from JSNodeFilterCondition, and just have NodeFilter's markAggregate add itself as an opaque root.

Taking this a step further, you could even remove the markAggregate function from NodeFilter, and just have the NodeFilter owners (TreeWalker and NodeIterator) add the NodeFilter as an opaque root. Yeah, I'm starting to like that idea even more.
Comment 6 Oliver Hunt 2011-04-15 12:56:31 PDT
Committed r84021: <http://trac.webkit.org/changeset/84021>