[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 bugzilla-request-daemon at opendarwin.org
Thu May 11 21:40:28 PDT 2006


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.



More information about the webkit-reviews mailing list