[Webkit-unassigned] [Bug 27425] adding auto-generator support for GDOMHTMLElementWrapperFactory[.cpp/.h]

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27425


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34008|review?(eric at webkit.org)    |review-
               Flag|                            |




--- Comment #16 from Eric Seidel <eric at webkit.org>  2009-08-03 15:54:58 PDT ---
(From update of attachment 34008)
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". :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.