Bug 8331 – DOMNodeLists returned to Objective-C are not properly wrapped

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[James G. Speth]

if you use valueForKey: or evaluateWebScript: from objective-c to access a DOMNodeList, the returned value is not properly wrapped and shows up as an instance of WebScriptObject, not DOMNodeList.

For example:

id nodes = [domDocument valueForKey:@"childNodes"];    // returns instance of class WebScriptObject
id nodes2 = [domDocument childNodes];  // returns instance of class DOMNodeList

[James G. Speth]

Created attachment 7650 [details]
patch for WebCore/bindings/objc/DOMUtility.mm

[Timothy Hatcher]

This is the right approach. We should look at other classes that need to be handled in this function to get the proper wrapper allocated. Some that come to mind are DOMCSSStyleDeclaration, DOMCSSRuleList, DOMCSSRule, etc.

[Eric Seidel (no email)]

If this patch is for review, we'll need to mark it with a review ? flag.

[James G. Speth]

Created attachment 8343 [details]
patch, please review

[Darin Adler]

Comment on attachment 8343 [details]
patch, please review

Lets not check in the RGBColor, Rect, and Counter stuff commented-out. I don't like landing code with #if 0 around it.

Best way to keep track of that loose end is to write a bug report about it.

In valueForKey: it seems to me that we should be calling super in the "!isSafeScript" case too.

Otherwise, this patch looks perfect!

I think I'm going to review+ this even though it has the #if 0 in it.

[James G. Speth]

Created attachment 8480 [details]
removed #if 0 code, improved array accessors, please review

i've opened bug 9058 to track the changes the had been in the #if 0 code.  also in here are improvements to the indexed array accessors we use for cocoa bindings.  this makes communicating with the NSTreeController a much smoother process.

[Eric Seidel (no email)]

Comment on attachment 8480 [details]
removed #if 0 code, improved array accessors, please review

The createObjCDOM* methods seem a bit odd.  They end up as 4 lines of code instead of 1.

Otherwise the code looks fine.  I think xenon or darin shoudl comment however.

[James G. Speth]

Created attachment 8505 [details]
much cleaner DOMUtility.mm in this one

this one also has KVO hooks for notifying observers when a nodeList changes.  please look at Node.cpp, Node.h, and DOM.mm closely and let me know if this is the right way to go about calling out from the Node to the Obj-C API.  thanks.

[Maciej Stachowiak]

The fix to wrap the miscellaneous DOM objects properly is a great one. However, it's mixed in with a bunch of other KVO-related changes which should really be separate patches.

Some specific comments about some of the changes:

I don't think it's a good idea to sprinkle KVO-specific changes into the core DOM code, such as this change in Node.cpp:

+    
+    willChangeValueForKey("childNodesArray");
+    
     NodeListSet::iterator end = m_nodeLists->end();
     for (NodeListSet::iterator i = m_nodeLists->begin(); i != end; ++i)
         (*i)->rootNodeChildrenChanged();
+    
+    didChangeValueForKey("childNodesArray");

It also doesn't seem right; the willChange is getting called after the value actually gets changed.

I think it would be better for KVO to hook in using a more general DOM mechanism.

I think the change to create the right wrapper is great, so please re-upload that, and put the other stuff in a separate patch in its own bug please.

[James G. Speth]

Created attachment 8995 [details]
patch addressing this bug only

Here's a patch for the additional wrapper support without the KVO calls (since you're right, and they are unrelated to this bug, I'll resubmit them on a separate report).

[Darin Adler]

Comment on attachment 8995 [details]
patch addressing this bug only

This all looks great except for one thing I don't understand.

Why the use of objc_getClass in DOMUtility.mm instead of the usual syntax for invoking class methods?

[Darin Adler]

Comment on attachment 8995 [details]
patch addressing this bug only

Oh, I see now -- you wanted to avoid having to create separate inline functions for each class -- I guess that's OK. It's slightly better to have the compile-time checking that you get with the usual syntax, but this is a great improvement so lets land it.

r=me

[Darin Adler]

Comment on attachment 8995 [details]
patch addressing this bug only

I really do like the DOMUtility.mm from http://bugzilla.opendarwin.org/attachment.cgi?id=8480 better, but it's no big deal.

[David Kilzer (:ddkilzer)]

Committed revision 15018.

[James G. Speth]

i know it just landed, but if you really don't want to use the objc_getClass function, we can get rid of it and use the compile-time @compatibility_alias directive like this:

@compatibility_alias ObjC_DOMNode DOMNode;

then the code below looks like this:

    if (value->inherits(&DOMNode::info))
        newObj = [ObjC_DOMNode _nodeWith:static_cast<DOMNode *>(value)->impl()];

this reads cleanly, is checked at compile-time, and @compatibility_alias is documented at file:///Developer/ADC%20Reference%20Library/documentation/DeveloperTools/gcc-4.0.1/gcc/compatibility_005falias.html#compatibility_005falias

let me know if you want another patch, otherwise, i'm going to move on with my life. :)