[webkit-reviews] review denied: [Bug 8850] cocoa bindings - crash when DOM nodes are deleted while NSTreeController+NSTableView are bound : [Attachment 9081] patch implementing comment #11

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 8850: cocoa bindings - crash when DOM nodes are deleted while
NSTreeController+NSTableView are bound
http://bugzilla.opendarwin.org/show_bug.cgi?id=8850

Attachment 9081: patch implementing comment #11
http://bugzilla.opendarwin.org/attachment.cgi?id=9081&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
A DOMNodeList is a dynamic list. The list itself shouldn't be changing -- the
contents of it should be changing. I suspect it's a bug that every call to
childNodes produces a distinct DOMNodeList. Existing DOMNodeList objects are
automatically updated to reflect the new child nodes.  

I'm not convinced about argument that will/did should be at the same time for
childNodes. I don't think it's good that for KVO we return a distinct static
array of child nodes every time. Wouldn't it be better if the KVO list was
dynamic just the way a DOMNodeList is? I'd think it would be straightforward to
wrap the DOMNodeList in something that is a dynamic NSArray rather than
allocating a static NSArray.

So for KVO, I suspect the node should not be sending a notification at all.
Instead, the list should be sending a notification indicating that its contents
are changing.

As far as the changes to WebCore::Node are concerned, I don't think it's
acceptable to add 12 bytes to every DOM node for this feature. We should
instead use a single bit to identify that a node is observed and store the ref
counts, callbacks, and contexts in a separate data structure (in a hash table,
I would think).

The context pointer should just be void*, not const void*.