[webkit-reviews] review denied: [Bug 27425] adding auto-generator support for GDOMHTMLElementWrapperFactory[.cpp/.h] : [Attachment 34008] add --bindingType to make_names.pl

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Luke Kenneth Casson Leighton
<lkcl at lkcl.net>'s request for review:
Bug 27425: adding auto-generator support for
GDOMHTMLElementWrapperFactory[.cpp/.h]
https://bugs.webkit.org/show_bug.cgi?id=27425

Attachment 34008: add --bindingType to make_names.pl
https://bugs.webkit.org/attachment.cgi?id=34008&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
This is *much* better!

1.  I think we need to validate that bindingType is either JS or GDOM and call
die when it's not one of those.

Why do we have this code at all?
 if ($binding eq 'JS') {
 78	printNamesHeaderFile("$namesBasePath.h");
 79	printNamesCppFile("$namesBasePath.cpp");
 80 }

Why is HTMLName.cpp compiled by inclusion in the element factory wrapper? That
seems wrong.  We should at least add a FIXME there.

Why are we skipping DATAGRID?
612	    # skip DATAGRID for now
 613	     next if ($conditional eq 'DATAGRID');

I think there has to be a better way to do it than 3-times copy/paste of that
code.

We should be checking explicitly for GDOM, but I don't think this DATAGRID hack
belongs here:
10	   if ($binding ne 'JS') {
 911		 # skip DATAGRID for now
 912		 next if ($conditional eq 'DATAGRID');
 913	     }

This block should probably be a subroutine:
 } else {
 934		     print F <<END
 935 static gpointer 

again here:
53	       } else {
 954		     print F <<END
 955 static gpointer 

and it shoudl be elif 'GDOM', not else.

AGain, checking for GDOM, not ne 'JS'
 982	 if ($binding ne 'JS') {

Again, probably should be subroutines:
11     } else {
 1012	      print F <<END
 1013 #include "CString.h"

You don't have to abstract the existing JS stuff, but if you put the gdom stuff
into nice little subroutines, someone else can come back and fix the JS stuff
later (or we'll fix it when we move to a python script in the future).

There are much cleaner ways to do this DATAGRID hack.  I'm still not sure why
it's needed:
1052	     if ($binding ne 'JS') {
 1053		  # skip DATAGRID for now
 1054		  next if ($conditional eq 'DATAGRID');
 1055	      }

In general this looks great though! r- mostly for the datagrid hack.  I would
much prefer that some of this is turned into subroutines instead of inline
ifdefs.  Also we need to explicitly check for GDOM, it can't be the "other
default". :)