649057 - Make all nsINodeLists (except maybe the XBL one) inherit from nsWrapperCache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I hear tell there's a partial fix for this.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 525542 [details] [diff] [review]
v1

This seems to pass on tryserver.

Comment 3

[Boris Zbarsky [:bzbarsky]]

nsAnonymousContentList should unlink mContent.

Can you please catch me tomorrow?  I'd like to understand the exact behavior of parent stuff here, and in particular why it's ok for nsBaseContentList to QI to nsWrapperCache without reporting a useful parent.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, the key is that we need the parent to ensure only one wrapper, right?  Seems like nothing ensures that right now for nsBaseContentList, so shouldn't the QI to nsWrapperCache live on nsSimpleContentList?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Or would that fail due to inheriting from nsWrapperCache anyway?

After this patch, do we still instantiate nsBaseContentList?  If so, do we only do it in places that won't get JS-wrapped or something?

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to comment #5)
> After this patch, do we still instantiate nsBaseContentList?

We do not, should be impossible because its GetParentObject is pure virtual (inherited from nsINodeList). I opted for that because there was only one remaining use of it (which also wasn't exposed to JS), I think it's simpler now.
Having the QI for nsWrapperCache live on nsBaseContentList is just convenience, I can instead add it to all the derived classes if you think that makes more sense (might be a tiny bit faster too).
The GetParentObject implementations all return a sensible parent I think, let me know if you agree :-).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 525542 [details] [diff] [review]
v1

> We do not, should be impossible because its GetParentObject is pure virtual

Aha, perfect.  I'd missed that.

r=me with the comment 3 bit about unlinking, then.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Peter, can you get this updated and checked in?  I'd really like to make the nodelist binding code depend on this patch....

Comment 9

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/623563df5bd1