Bug 8389 - support for Cocoa bindings - binding an NSTreeController to the WebView's DOM
Summary: support for Cocoa bindings - binding an NSTreeController to the WebView's DOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-04-14 12:41 PDT by James G. Speth
Modified: 2007-03-31 23:01 PDT (History)
2 users (show)

See Also:


Attachments
patch for additional support of cocoa bindings in webkit (6.64 KB, patch)
2006-04-14 12:43 PDT, James G. Speth
timothy: review-
Details | Formatted Diff | Diff
Interface Builder palette for controller and example NIB file (39.98 KB, application/zip)
2006-04-14 12:48 PDT, James G. Speth
no flags Details
screenshot of example nib file in action (90.49 KB, image/png)
2006-04-14 12:49 PDT, James G. Speth
no flags Details
new patch for review (8.29 KB, patch)
2006-04-14 18:13 PDT, James G. Speth
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James G. Speth 2006-04-14 12:41:17 PDT
I have some small changes that make it possible to bind an NSTreeController to the DOM of a WebView, adding some additional support for Cocoa bindings to WebKit.  Basically, there are a couple of places in WebKit that needed to send out KVO messages, and in JavaScriptCore I made a category that adds the array accessors (count/objectAtIndex:) to WebScriptObject.

I also added a controller class, WebController, that is a subclass of NSTreeController that has a new outlet/binding for the WebView.  It is a very minimal controller that could use some more work, but it does enough to make it possible to connect things up in Interface Builder.

I'll also attach the Interface Builder palette and a sample NIB file for using the bindings.

I'd be happy to make changes to this, as there are several areas I can see that might need to be modified.  I wanted to put this here for review, instead of going too far on my own.
Comment 1 James G. Speth 2006-04-14 12:43:12 PDT
Created attachment 7709 [details]
patch for additional support of cocoa bindings in webkit
Comment 2 James G. Speth 2006-04-14 12:48:14 PDT
Created attachment 7711 [details]
Interface Builder palette for controller and example NIB file

after building with the patch, you should run Interface Builder with the built frameworks.  next, you open the palette in Interface Builder, then the simple.nib file after that.  you can test it by choosing "Test Interface" from the File menu.
Comment 3 James G. Speth 2006-04-14 12:49:11 PDT
Created attachment 7712 [details]
screenshot of example nib file in action
Comment 4 Eric Seidel (no email) 2006-04-14 13:04:26 PDT
Comment on attachment 7709 [details]
patch for additional support of cocoa bindings in webkit

The patch looks fine.  

I'm a bit surprised by this:

+@implementation WebUndefined (WebKitCocoaBindings)
+
+- (NSString *)description
+{
+    return @"";
+}

but honestly I don't know what WebUndefined does.  I would have expected that to return either "nil" or @"undefined".
Comment 5 Timothy Hatcher 2006-04-14 13:30:12 PDT
Comment on attachment 7709 [details]
patch for additional support of cocoa bindings in webkit

Looks good, here are few things that should be cleaned up.

+@interface WebUndefined (WebKitCocoaBindings)
+
+- (NSString *)description;
+
+@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?
Comment 6 Timothy Hatcher 2006-04-14 13:33:32 PDT
+    [wv _didChangeValueForKey: _WebMainFrameDOMKey];
+    [wv _willChangeValueForKey: _WebMainFrameDOMKey];

Also our ObjC coding style does not include a space after colons in the selector.
Comment 7 Timothy Hatcher 2006-04-14 13:47:22 PDT
We should consider adding did/will change binding updates for nodes by hooking listening for DOM mutation events. This would let the outline view be a live tree representation for example if some JavaScript adds or deletes nodes on the fly.
Comment 8 James G. Speth 2006-04-14 18:13:55 PDT
Created attachment 7720 [details]
new patch for review

i snuck one new thing in here which is that WebScriptObject valueForKey: now calls super if the javascript returns undefined.  this is actually what i really wanted with that WebUndefined description business, but i was wimping out for some reason.

if super valueForKey: fails it will call valueForUndefinedKey, which is important because it causes the right behavior to happen with bindings using the "Raises for Not Applicable Keys" flag and the Not Applicable Placeholder setting.

this lets you set an empty string as the placeholder, and get rid of all the "undefined"s in the outline view for nodes that don't have innerHTML (for instance).
Comment 9 Timothy Hatcher 2006-04-14 21:44:41 PDT
Comment on attachment 7720 [details]
new patch for review

Looks great! Thanks for the code comments.

+    if ([length respondsToSelector:@selector(intValue)])
+        return [length intValue];
+    else
+        return 0;

One minor style thing for the future; no need for "else" here.
Comment 10 Timothy Hatcher 2006-04-15 12:25:04 PDT
Landed in r13877.
Comment 11 Mark Rowe (bdash) 2007-03-31 18:32:02 PDT
The attached IB palette does not open for me, with IB logging a message to the console saying: 
Interface Builder Tiger[90541:d03] -[__IBDocument readFromFile:/Volumes/Data/Downloads/palette/WebController.palette/DP.nib ofType:Interface Builder Palette] failed with exception "*** -[NSKeyedUnarchiver decodeObjectForKey:]: cannot decode object of class (WebController)"

What do I need to do to make this work?
Comment 12 Alexey Proskuryakov 2007-03-31 23:01:57 PDT
Mark, will the palette work for you if you run IB under Rosetta?