[Webkit-unassigned] [Bug 8850] cocoa bindings - crash when DOM nodes are deleted while NSTreeController+NSTableView are bound

[bugzilla-daemon at opendarwin.org]

http://bugzilla.opendarwin.org/show_bug.cgi?id=8850


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #9057|review?                     |review-
               Flag|                            |




------- Comment #10 from darin at apple.com  2006-06-27 09:41 PDT -------
(From update of attachment 9057)
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.


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.