[webkit-reviews] review denied: [Bug 8389] support for Cocoa bindings - binding an NSTreeController to the WebView's DOM : [Attachment 7709] patch for additional support of cocoa bindings in webkit

[bugzilla-request-daemon at opendarwin.org]

Timothy Hatcher <timothy at hatcher.name> has denied Timothy Hatcher
<timothy at hatcher.name>'s request for review:
Bug 8389: support for Cocoa bindings - binding an NSTreeController to the
WebView's DOM
http://bugzilla.opendarwin.org/show_bug.cgi?id=8389

Attachment 7709: patch for additional support of cocoa bindings in webkit
http://bugzilla.opendarwin.org/attachment.cgi?id=7709&action=edit

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
Looks good, here are few things that should be cleaned up.

+ at interface WebUndefined (WebKitCocoaBindings)
+
+- (NSString *)description;
+
+ at end

No need for this interface, NSObject exposes description. I agree with Eric,
the WebUndefined's description should be "undefined". No need for this to be in
a category, just add description to the main implementation of WebUndefined.

+- (unsigned int)count
+{
+    return [[self valueForKey:@"length"] intValue];
+}

You should not assume "length" will always have a intValue method. You should
check for isKindOfClass: NSString or NSNumber, or respondsToSelector:.

+- (DOMDocument *)mainFrameDOM;

This should be called "mainFrameDocument".

+    [self setChildrenKeyPath:@"none"];

Should that be [self setChildrenKeyPath:nil]?

+- (void)setContent:(id)newContent
+{
+    id content = [[self content] retain];
+    NSArray *paths = [[self selectionIndexPaths] retain];
+    NSString *childrenKeyPath = [[self childrenKeyPath] retain];
+    
+    [self setSelectionIndexPaths:nil];
+    [super setContent:nil];
+    [self setChildrenKeyPath:@"none"];
+    
+    [super setContent:newContent];
+    [self setChildrenKeyPath:childrenKeyPath];
+    [self setSelectionIndexPaths:paths];
+    
+    [content release];
+    [paths release];
+    [childrenKeyPath release];
+}

What is the reason we can't just use super's setContent:?

+    [webView release];
+    webView = [newWebView retain];

I recommend you do this like follows. 

+    id old = webView;
+    webView = [newWebView retain];
+    [old release];


+    [wv _didChangeValueForKey: _WebMainFrameDOMKey];
+    [wv _willChangeValueForKey: _WebMainFrameDOMKey];

Should these be called in the reverse order?