[webkit-reviews] review denied: [Bug 8850] cocoa bindings - crash when DOM nodes are deleted while NSTreeController+NSTableView are bound : [Attachment 9057] 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 9057: patch for review
http://bugzilla.opendarwin.org/attachment.cgi?id=9057&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
At the very least, this needs to be ifdef'd properly so that it will not break
platforms other than OS X.

I don't see why willChangeValueForKey and didChangeValueForKey are virtual
functions.

The declaration "const char *key" should be "const char* key".

I don't think the name of the KVC key should be in the platform-independent
code.

This code:

+    DOMNode *cachedNode;
+    cachedNode = getDOMWrapper(this);
+    if (cachedNode)
+	 [cachedNode willChangeValueForKey:[NSString
stringWithUTF8String:key]];

should be formatted like this:

    DOMNode* cachedNode = getDOMWrapper(this);
    if (cachedNode)
	[cachedNode willChangeValueForKey:[NSString stringWithUTF8String:key]];


If this code is really going to be hot, then we need to avoid calling
stringWithUTF8String every time. One way we could do that is to relegate the
key part to Macintosh specific code and just use @"" syntax for it. At the very
least we don't want to do the autoreleased form.

Calling getDOMWrapper every time a child is added or removed is likely to cause
measurable performance impact. We're going to need to figure out what to do
about that. I'd like to see this structured so that you don't do everything
twice.

It seems clear the willChangeValueForKey call needs to be done before the child
list is changed, and the didChangeValueForKey needs to be done after. They
should not be done one after another.

The part of this not related to the change notifications looks good and about
ready to land, but the change notification part needs work.