[webkit-reviews] review denied: [Bug 8850] cocoa bindings - crash when DOM nodes are deleted while NSTreeController+NSTableView are bound : [Attachment 8247] patch for review

[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 8247: patch for review
http://bugzilla.opendarwin.org/attachment.cgi?id=8247&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The mutableCopyWithZone: thing confuses me. These are not copyable objects. Why
is NSTreeController calling mutableCopy on an object that doesn't implement the
NSMutableCopying protocol?

Are you sure that indexOfObjectIdenticalTo: is the only other array method we
need? The reason I ask is that if we instead subclassed from NSArray instead of
NSObject, we'd get *all* the methods from the NSExtendedArray category for
free. And I think it will be binary compatible since there are no fields in
NSArray. I think that's a better solution.

Ironically, this then means that we'll be implementing the NSCopying and
NSMutableCopying protocols. But I believe NSArray will provide the copy
implementations which will actually create new arrays. So subclassing from
NSArray should fix both of these problems.

I don't see any code to remove the event listener. I suspect that by adding the
WebView itself as a listener to the DOMDocument, we could create a situation
where the entire WebView leaks. Did you test that?

We absolutely cannot have a DOM mutation event handler that's installed 100% of
the time. That will definitely cause a performance regression because it will
disable an optimization that we do in the very common case when there are no
event listeners listening to DOM mutation events.