Bug 4624 – WebCore needs autogenerated Obj-C DOM bindings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Eric Seidel (no email)]

WebCore+SVG needs autogenerated Obj-C DOM bindings

Now that http://bugzilla.opendarwin.org/show_bug.cgi?id=4441 is about to land, and includes the first 
edition of c++ and javascript autobindings support, it's now time for one of us to step up to the plate 
and create the first ever Obj-C SVG DOM Bindings.  This really should be a pretty simple task.  Take a 
look at:

kdom/bindings/IDLCodeGeneratorCpp.pm
and
kdom/bindings/IDLCodeGeneratorJS.pm

to see how the other bindings are being generated.  Obj-C should look a lot like c++.  For multiple 
inheritance, I would suggest you check out Java SVG DOM bindings built for other SVG engines like 
Batik:
http://xml.apache.org/batik/javadoc/org/apache/batik/dom/svg/SVGOMFECompositeElement.html

replacing "interfaces" with "catagories" or protocols.

[Eric Seidel (no email)]

Tobias said he would take a stab at this one. :)

[Eric Seidel (no email)]

Making this a more generic bug, as this now affects more than just SVG.

[Sam Weinig]

Created attachment 10007 [details]
first 22 autogen'ed objective-c bindings patch

I'm attaching a work in progress, but it does compile and work.  It removes 22 of the Objective-C DOMHTML interfaces and instead auto-generates them.

[Eric Seidel (no email)]

Comment on attachment 10007 [details]
first 22 autogen'ed objective-c bindings patch

I know you're not quite asking for a review here yet, but here are my thoughts.

1.  OMG YES!!!
2.  Files you touched need your copyright.
3.  We will need to see at least one of the generated files in order to properly review this.
4.  +    # FIXME: Check dates to see if we need to re-generate anything ( and possibly other comments) can just be removed.  (In this case make takes care of that aspect.)
5. GetNativeType and AddIncludesForType shoudl probably be shared between generators.

[Sam Weinig]

Created attachment 10070 [details]
patch v2

Quite a bit more, still no changelog as I don't think it is quite ready for prime time.  If anyone thinks it is getting too big and I should start committing it in chunks, please comment.  I have fixed some of Erik's issues but no all yet.

[Sam Weinig]

Created attachment 10072 [details]
sample generated header

[Sam Weinig]

Created attachment 10073 [details]
sample generated source file

[Sam Weinig]

Crap, I meant to say Eric, not Erik.  Sorry Eric!

[Sam Weinig]

Created attachment 10153 [details]
patch v3

A new version of the patch.  It's getting close to being fully review ready.  It now has all of DOMCore.h and DOMHTML.h split out of their original files and either auto generated or in cases where that is not possible yet, just split by class name into new files.  Three classes of DOMCSS.h are also done in this way.  Before I put a patch up for review I have to correct an error I made.  Namely, when splitting the files by class name (those that cannot yet be auto-generated), I did not svn cp the original file to keep the history.  This is something I can easily fix, but it is not done yet.  

There a few reasons why some of the files have been split out instead of auto generated.  One reason is that no IDL file exists yet for the class, and I have not yet added it.  Another issue has been that IDL file that exists, differs from what the Objective C binding is expecting.  This happens mostly in files that are being used by the JS bindings, but not completely. (see the HTMLElement.idl file for an example).  I have not yet worked out a way to add the missing methods to the IDL's without breaking the existing JS bindings. 

Comments welcome.

[Sam Weinig]

Created attachment 10161 [details]
patch with just DOMCore changes

This new patch contains only the auto-generation of the DOMCore bindings.  It should be much more manageable.

[Sam Weinig]

hehe, well the patch with just the DOMCore changes should of been much smaller, but due to the way svn cp works, it seems to be a lot bigger (though most of it is the same).  

[Sam Weinig]

Comment on attachment 10161 [details]
patch with just DOMCore changes

Better version forthcoming.

[Sam Weinig]

Created attachment 10185 [details]
patch with just DOMCore changes (now with more autogeneration and smaller size!!!)

Well, this is about as small as it's going to get.  I am now auto-generating all but DOMNode and DOMObject for DOMCore, which has greatly reduced the size.  The remain bloat is caused by the files DOMCore.h and DOM.mm being svn cp'ed to make the the files for DOMNode and DOMObject.  There is also quite a bit from the Xcode project file changes.

[Timothy Hatcher]

This is looking good!

Come comments:

+    # - Add protection against unsupported modules
+    push(@headerContentHeader, "\n#if ${conditional}_SUPPORT\n") if $conditional;

We don't want to add these XXX_SUPPORT checks to the ObjC headers, since these are puclic.

On the note about public headers, we need to make sure the generated API is compatiable with the previous API. This also includes keeping the previous header names. Those old headers can either contain the @interfaces or include other new headers that have one class per header. Developers include specific DOM headers and we need to maintain that build compatibility. We might need a better way to get these headers copied into WebKit, one shell script per header in the WebKit project does not scale well if we will have more DOM headers.

+            if ($attribute->signature->extendedAttributes->{"ObjCPrivate"}) {
+                # FIXME: for now, just skip methods not approved for API yet
+                next;
+            }

We really need to generate private headers for the non-public stuff. A lot of this API is private but still used inside Apple/Safari (if it was private before autogeneration).

The $headerTemplate needs changed to match the header used in the current public headers. The current $headerTemplate can used for .m files. 

Some of the methods you labeled as ObjCPrivate are public now, make sure you sync with the TOT public headers. Such as offsetTop and friends.

[Timothy Hatcher]

Comment on attachment 10185 [details]
patch with just DOMCore changes (now with more autogeneration and smaller size!!!)

r- based on my earlier comments.

[Sam Weinig]

Created attachment 10240 [details]
better patch of DOMCore changes (ZOMG, it's even smaller)

Addresses some of Tim's comments.  I've not svn cp'ed for this one to make reviewing easier, but when it is committed, it should have the histories included for DOMNode and DOMObject.

[Sam Weinig]

Created attachment 10242 [details]
the generated files for DOMCore

[Timothy Hatcher]

Comment on attachment 10240 [details]
better patch of DOMCore changes (ZOMG, it's even smaller)

Gret work!

Some more comments:

DOMCSS.h:
+// Interface that used to be in this file:
+//
+
+// Now included in DOMElement.h
+//@interface DOMElement (DOMElementCSSInlineStyle)
+//- (DOMCSSStyleDeclaration *)style;
+//@end
+
+// Now included in DOMDocument.h
...

We can remove this, since we include the right headers we don't need to tell people where the interfaces moved. Same applies to DOMEvents.h, DOMExtensions.h, DOMRange.h, DOMStylesheets.h, DOMViews.h, DOMXPath.h.

DOMCSS.h:
+// FIXME: this should go into DOMDocument.h
+// REASON: not in IDL yet.

This is a public header, we can't have a FIXME in there. Same applies to DOMEvents.h, DOMExtensions.h and DOMTraversal.h.

+             or $type eq "AtomicString") {

AtomicString should not be exposed in ObjC, they should be converted to NSString.

+    # Default, assume objective-c type is a pointer with the same type name as
+    # idl type prefixed with "DOM".
+    return "DOM$type *";

Shouldn't this use GetClassName() to special case DOMImplementation?

+            # Only generate 'dealloc' and 'finalize' methods for subclasses of DOMObject.

Comment should say "...for direct subclasses of DOMObject." or "immediate subclasses"

offsetTop and friends are still ObjCPrivate.

DOMElementExtensionsFIX should be removed or changed.  

Very close to an r+ from me!

[Timothy Hatcher]

Looking at the generated headers:

// Functions

Should that comment say //Methods

Or remove the comments?

[Timothy Hatcher]

Looking at the generated method names I see there is no whitespace added before the :.

- (NSString *)getAttributeNS:(NSString *)namespaceURI:(NSString *)localName;
- (void)setAttributeNS:(NSString *)namespaceURI:(NSString *)qualifiedName:(NSString *)value;

[Sam Weinig]

Created attachment 10245 [details]
DOMCore autogen (addressing more of Tim's comments)

[Timothy Hatcher]

Comment on attachment 10245 [details]
DOMCore autogen (addressing more of Tim's comments)

This is great!

Some formatting issues, other than that r=me.

-        attribute long scrollLeft;
+                 attribute [ObjCCatagory=DOMElementExtensions] long scrollLeft;
 
-        attribute long scrollTop;
+                 attribute [ObjCCatagory=DOMElementExtensions] long scrollTop;

These have some extra whitespace compared to the others.

Check your scripts for tabs, I see a few in there.

[Sam Weinig]

Created attachment 10261 [details]
patch addressing more of Tim's comments as well as comments from IRC

[Timothy Hatcher]

Comment on attachment 10261 [details]
patch addressing more of Tim's comments as well as comments from IRC

This looks good! Builds and works on my machine. r=me

[Sam Weinig]

Landed in r16068.