[webkit-reviews] review requested: [Bug 10685] ObjC DOM should have no unnamed parameters : [Attachment 10358] Implements this and other small tweaks

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has asked Darin Adler <darin at apple.com>
for review:
Bug 10685: ObjC DOM should have no unnamed parameters
http://bugzilla.opendarwin.org/show_bug.cgi?id=10685

Attachment 10358: Implements this and other small tweaks
http://bugzilla.opendarwin.org/attachment.cgi?id=10358&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
I mostly just have little nit comments:

I think this restriction could be eased using a ObjC specific keyword, if
necessary:
"Please don't change existing parameter names, or the ObjC API will change." 
Not something we need now, but could keep in mind for the future if another
language needs specific parameter names.


This seems a little confusing:
+    die "Skipping ObjC code generation, not needed on platforms other than Mac
OS." unless $ENV{"OS"} and $ENV{"OS"} eq "MACOS";

you're not actually skipping, you're dieing.  Maybe it should say.  "ObjC code
generation is not supported on your platform." or similar.

Personally I found the old style more readable:

-    if ($type eq "Counter"
-	     or $type eq "MediaList"
-	     or $type eq "CSSStyleSheet") {
+    if ($type eq "Counter" or $type eq "MediaList" or $type eq
"CSSStyleSheet") {
(actually in a couple places).

Another style comment:
+    $implIncludes{"DOMHTMLInternal.h"} = 1 if $type =~ /^HTML/;
One of the things we have started to do in our Ruby code (which also uses
one-line ifs all over) it to place an extra space between the code line and the
if, I find this really helps readability.  Perhaps WebKit would like to
consider doing the same.

Eventually this will not be true:
+    die "A class can't have more than one parent." if @{$dataNode->parents} >
1;
The others will just be categories.  WildFox just fixed this for JS.  You might
also consider saying "... more than one parent for ObjC."

I'm surprised the double-check is necessary here:
+	     if ($ENV{"MACOSX_DEPLOYMENT_TARGET"} and
$ENV{"MACOSX_DEPLOYMENT_TARGET"} >= 10.5) {

Why remove this?
-	     if ($hasGetterException) {
-		 die "We should not have any getter exceptions yet!";
-	     }
-	     
I don't see it used anywhere, but maybe it was already being used in code
before.

All in all, this looks fine.  I think at least darin should look over the API
changes.  I'm not sure simple argument names make the most sense for Obj-C
argument labels.  AppKit often uses prepositions with argument names, like "to"
or "with", etc.

I'm going to leave this ? for now.