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

• Component: WebKit Misc.
• Status: RESOLVED INVALID

[Luke Kenneth Casson Leighton]

this patch adds GDOMHTMLElementWrapperFactory which will eventually be replaced by an auto-generated set of matched files.  one key reason for not doing that right now is because exception support has been ripped out when compared to the file from which it is derived (JSHTMLElementWrapperFactory.[cpp/h].  another is simply time.
the files are being added under an agreement suggested by david to split #16401 into separate patches.

[Luke Kenneth Casson Leighton]

Created attachment 33052 [details]
adds gdomhtmlelementwrapperfactory.

these files have review comments on it (one key reason why they're submitted under a separate bugreport).

from memory, the questions are:

* why is there "shouting" in it?

  answer: because the original files, JSHTMLElementWrapperFactory.[cpp,h] from which they're copied, also have "shouting" in them.

* why is this not auto-generated?

  answer: because the auto-generator is written in a bug-ugly language called perl, and it was easier to do a first revision by cut/paste using the advanced editing features of vim than it was to mind-bend to yet another perl script [i did try].  also, the exception handling has been completely ripped out of CodeGeneratorGObject which makes this file quite different from its origins.

[Adam Treat]

Comment on attachment 33052 [details]
adds gdomhtmlelementwrapperfactory.

> [phone]  lkcl  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        WARNING: NO TEST CASES ADDED OR CHANGED
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=27425
> +
> +        * bindings/gdom/GDOMHTMLElementWrapperFactory.cpp: Added.  Copied from the auto-generated JSHTMLElementWrapperFactory.cpp 
> +        * bindings/gdom/GDOMHTMLElementWrapperFactory.h:  Added.  Copied from the auto-generated JSHTMLElementWrapperFactory.h

You are missing a description of what this patch actually does.  Please fill out some description for what this patch is meant to provide.

> +/* Note: This file is derived by hand from an automatically generated
> + file.
> + 
> + Keeping it up-to-date could potentially be done by adding a
> + make_names.pl generator, or by writing a separate generater which
> + takes JSHTMLElementWrapperFactory.h as input.
> +*/

Minor nit: s/generater/generator/

Regular nit: why is this generated file to be landed?  If it is to be updated in the future with a script, then why land it in the tree as static?  A placeholder until said script is written?

> +
> +
> +#include "config.h"
> +
> +#include "gdom/GdomNode.h"
> +#include "GDOMHTMLElementWrapperFactory.h"

Order of includes is wrong.   See the WebKit coding style guidelines for order of includes.

There a lot of commented out pieces of code.  We generally don't land patches with commented out pieces of code.

Beyond that, without a more detailed Changelog it makes it hard to review.

r- for the above reasons.

[Luke Kenneth Casson Leighton]

(In reply to comment #2)
> (From update of attachment 33052 [details])
> > [phone]  lkcl  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        WARNING: NO TEST CASES ADDED OR CHANGED
> > +
> > +        https://bugs.webkit.org/show_bug.cgi?id=27425
> > +
> > +        * bindings/gdom/GDOMHTMLElementWrapperFactory.cpp: Added.  Copied from the auto-generated JSHTMLElementWrapperFactory.cpp 
> > +        * bindings/gdom/GDOMHTMLElementWrapperFactory.h:  Added.  Copied from the auto-generated JSHTMLElementWrapperFactory.h
> 
> You are missing a description of what this patch actually does.  Please fill
> out some description for what this patch is meant to provide.

 ahh, ok.  done.

> > +/* Note: This file is derived by hand from an automatically generated
> > + file.
> > + 
> > + Keeping it up-to-date could potentially be done by adding a
> > + make_names.pl generator, or by writing a separate generater which
> > + takes JSHTMLElementWrapperFactory.h as input.
> > +*/
> 
> Minor nit: s/generater/generator/

 duh.
 
> Regular nit: why is this generated file to be landed?  If it is to be updated
> in the future with a script, then why land it in the tree as static?  A
> placeholder until said script is written?

 basically, yes.  i'm not a perl-ite.  nobody's yet volunteered to do the
 script yet, so the copied-from-autogenerated-javascript-version will have
 to do.  and it does actually work.

 also, there's some extra work that's needed (planned) - adding back in
 ExecState, which i removed (daftly) to simplify the gobject bindings.

 so i've delayed both because it's .... just.... too much extra work,
 and what's here now "does the job".


> > +#include "config.h"
> > +
> > +#include "gdom/GdomNode.h"
> > +#include "GDOMHTMLElementWrapperFactory.h"
> 
> Order of includes is wrong.

 drat, sorry. ah... ahh.... what's that doing in there?  that's
 from GDOMBinding.cpp ahh oops sorry, thank you for spotting that
 but GDOMBinding.cpp shouldn't be in this patch!

 i've corrected GDOMBinding.cpp of course, so thank you for that,
 but it won't be in the next revision.


> There a lot of commented out pieces of code.  We generally don't land patches
> with commented out pieces of code.

 ack.  well, that will be for GDOMBinding.cpp

[Luke Kenneth Casson Leighton]

Created attachment 33188 [details]
adds gdomhtmlelementwrapperfactory with better ChangeLog

revised patch, updated ChangeLog explaining why it's not autogenerated at the moment etc. corrected spelling.

[Eric Seidel (no email)]

Comment on attachment 33188 [details]
adds gdomhtmlelementwrapperfactory with better ChangeLog

We don't generally check-in autogenerated files.

[Eric Seidel (no email)]

Comment on attachment 33188 [details]
adds gdomhtmlelementwrapperfactory with better ChangeLog

Maybe I'm biased (I wrote much of this particular generator), but I really doubt it would be difficult to bend it to your needs.  Even if it's written in perl. ;)

[Eric Seidel (no email)]

Comment on attachment 33188 [details]
adds gdomhtmlelementwrapperfactory with better ChangeLog

It's not always required to do things the "right way" the first time, but in this instance, I think it's better to just do this right (modifying the generator) than checking in manual files.

[Luke Kenneth Casson Leighton]

(In reply to comment #6)
> (From update of attachment 33188 [details])
> Maybe I'm biased (I wrote much of this particular generator), but I really
> doubt it would be difficult to bend it to your needs.  Even if it's written in
> perl. ;)

 mmrmmmrmmm *grumble* *grumble* :)

[Luke Kenneth Casson Leighton]

ok.  done, but.... eurgh, i can't say it's much of an improvement.  certainly not in code size, i just cut/paste make_names.pl and called it make_gdom_names.pl - that's 1000 lines of perl, right there.  a diff -u make_names.pl make_gdom_names.pl might be more useful / indicative of what's been done, but hey...

the only advantage i can see is that some elements (video etc.) have been added, which weren't in the non-auto-generated file.

[Luke Kenneth Casson Leighton]

Created attachment 33977 [details]
blatant cut/paste copy of make_names.pl

it's terrible, it's hacked, it does the job - it's perl.  i haven't gone through it to remove functions to create HTMLNames.h,cpp - i just... *backing away from the keyboard*... :)

[Eric Seidel (no email)]

Comment on attachment 33977 [details]
blatant cut/paste copy of make_names.pl

It's unclear to me why we can't just modify the existing make_names.pl instead of copying it?  The whole point of my previous r- was that you were proposing checking in copy/paste code.  Now you're doing so again except in perl form. :)  Yes, make_names.pl is ugly.  You should feel free to re-write it in python if you like, but I think that would be a bit large of an undertaking.

If it would be a huge diff to make_names.pl to teach it about gdom then maybe we should just check in a separate script.  But I still think that's a mistake.

Checking in a hack like this is analogous to taking out a loan of money from a bank.  The loan will require continuous interest payments (keeping track of changes across the forked files) as well as eventually someone will have to re-pay the principle by un-forking the files!  You might not be the one who ends up paying that cost... so we as a project don't want to let you take out such a loan. :)

[Luke Kenneth Casson Leighton]

Created attachment 33999 [details]
diff for eric between make_names.pl an make_gdom_names

not a lot. 200 lines.
worth giving it a shot.

[Eric Seidel (no email)]

Comment on attachment 33999 [details]
diff for eric between make_names.pl an make_gdom_names

This totally seems doable.  It will be a bunch of re-working yes, but I think the right approach is to teach make_names how to make ElementWrapperFactories for languages other than JS.  Eventually I expect V8 will want to do something similar (if they don't already).

[Eric Seidel (no email)]

It looks like Chromium has already re-written at least part of this in python:
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/build/action_makenames.py?view=markup

[Luke Kenneth Casson Leighton]

Created attachment 34008 [details]
add --bindingType to make_names.pl

ouch.  300 line patch on a 1000-line file.  oh well.
confirmed that the output leaves {JS/GDOM}{SVG/HTML}ElementWrapperFactory.{cpp/h} the same.

[Eric Seidel (no email)]

Comment on attachment 34008 [details]
add --bindingType to make_names.pl

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". :)

[Luke Kenneth Casson Leighton]

(In reply to comment #16)
> (From update of attachment 34008 [details])
> This is *much* better!

 goood :)
 
> 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');

 because i don't have Gdom headers for anything that's DATAGRID based.

> In general this looks great though! r- mostly for the datagrid hack.

 ack.

>  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". :)

 ack.

 end-of-day.  will look at these again tomorrow.

[Eric Seidel (no email)]

Thank you for your perseverance on this issue Luke.

[Luke Kenneth Casson Leighton]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 34008 [details] [details])
> > This is *much* better!
> 
>  goood :)
> 
> > 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');
> 
>  because i don't have Gdom headers for anything that's DATAGRID based.

 ok - i gave this a shot - getting the CodeGeneratorGObject.pm to auto-generate GdomHTMLDataGridElement.cpp,h etc. and they depend on the existence of GdomDataGridDataSource.cpp,h.

 these files do not exist.

 and... they're not auto-generated, they must be created manually.

 i'm done with extra workload - adding extra functionality - until this series of patches is in, or unless there's a damn good reason.

 so, that means that i'm not willing to create GdomDataGridDataSource at this time (from looking at WebCore/bindings/JS/JSDataGridDataSource.cpp,h).

 therefore, to reduce workload, a small and simple hack is needed, to skip anything that is #ifdef DATAGRID.

 this simple hack will still allow people to create JS-based DATAGRID bindings, and a working web browser, but they will just not have gobject bindings to datagrid.

 DATAGRID can be looked at _after_ the series of gobject patches are landed.

[Luke Kenneth Casson Leighton]

Created attachment 34060 [details]
answers review comments.  

ok explanation for DATAGRID skip is in previous comment: summary - too much knock-on effect / consequences / increase in workload.  i made JS the default so that it was not necessary to modify GNUmakefile.am.  necessary modifications to GNUmakefile.am now included.

[Eric Seidel (no email)]

Comment on attachment 34060 [details]
answers review comments.  

I wasn't clear, sorry.  I agree that JS should be default.  I just wanted all the places where you assumed !JS was Gdom to turn into explicit GDom checks.

passing no --bindingType can/should default to JS.  Otherwise you'd have to change lots more files here. :)

327         if ($binding eq 'Gdom') {
 328             # skip DATAGRID for now
 329             next if ($conditional eq 'DATAGRID');
 330         }

Those can all go away if you just remove DATAGRID from the list of tags at the start, no?  Removing it once instead of skipping it in every for loop seems like a cleaner solution, or?  Also, don't you all have ENABLE_DATAGRID off anyway?  Won't make_names just print #defines around all the DATAGRID stuff which will be compiled out?

$F really should have a better name, per our style guidelines: http://webkit.org/coding/coding-style.html.  Since you're just moving existing code, I guess it's OK as is though.

So the GNUMakefile.am changes are fine, but not required if you restore the JS default.

This in general looks great.  I think we still can fix the DATAGRID with a cleaner hack.

[Luke Kenneth Casson Leighton]

(In reply to comment #21)
> (From update of attachment 34060 [details])
> I wasn't clear, sorry.  I agree that JS should be default.  I just wanted all
> the places where you assumed !JS was Gdom to turn into explicit GDom checks.

 done.  yep.

> passing no --bindingType can/should default to JS.  Otherwise you'd have to
> change lots more files here. :)

 ack.  whew.

> 327         if ($binding eq 'Gdom') {
>  328             # skip DATAGRID for now
>  329             next if ($conditional eq 'DATAGRID');
>  330         }
> 
> Those can all go away if you just remove DATAGRID from the list of tags at the
> start, no? 

 yes - good idea, but my perl skills don't extend to doing that.

 cut-and-paste if it's already in the same perl file (or in any other one i've encountered, which aren't many) i can manage.  i even managed to look up how to do includes recently...

 ... but that's about it, really.  the only reason CodeGeneratorGObject.pm works is because the other CodeGenerator*.pm files were complex enough to contain everything that i needed.


> Removing it once instead of skipping it in every for loop seems
> like a cleaner solution, or?  Also, don't you all have ENABLE_DATAGRID off
> anyway? 

 no - it appears to be on by default.  and, anyone enabling it will end up with a compile-time error, because the CodeGeneratorGObject.pm isn't equipped to auto-generate GdomHTMLDataGrid{Row,Col,Element}.{cpp,h}, and neither is the infrastructure there to support it.

> Won't make_names just print #defines around all the DATAGRID stuff
> which will be compiled out?

 yes - but then suppose someone _does_ want JS-based DATAGRID support, and doesn't care about GObject or doesn't care about DATAGRID not being in GObject bindings?


> $F really should have a better name, per our style guidelines:
> http://webkit.org/coding/coding-style.html.  Since you're just moving existing
> code, I guess it's OK as is though.

 ack.  whew.
 
> So the GNUMakefile.am changes are fine, but not required if you restore the JS
> default.

 going.  prefer not to hack GNUmakefile.am right now, i have a number of other mods to it that i'm keeping track of.
 
> This in general looks great.  I think we still can fix the DATAGRID with a
> cleaner hack.

 not with my level of perl skills, though.

[Eric Seidel (no email)]

Is DATAGRID off for the gtk port?

What happens when you try to generate DATAGRID files?  Why are they different from other HTML elements?

[Luke Kenneth Casson Leighton]

> > 327         if ($binding eq 'Gdom') {
> >  328             # skip DATAGRID for now
> >  329             next if ($conditional eq 'DATAGRID');
> >  330         }
> > 
> > Those can all go away if you just remove DATAGRID from the list of tags at the
> > start, no? 
> 
>  yes - good idea, but my perl skills don't extend to doing that.

 looking at tagHandler, it looks like this gets called several times per tag
 e.g. once for interfaceName, another time for conditional.

 so... you would only discover that you shouldn't _have_ the tag _after_
 it's been created (once you encounter conditional = 'DATAGRID'.

 which means iterating, after the fact, over $tags and re-creating $realtags excluding anything conditional=DATAGRID.

 ... yuk?

[Luke Kenneth Casson Leighton]

(In reply to comment #23)
> Is DATAGRID off for the gtk port?
> 
> What happens when you try to generate DATAGRID files?  Why are they different
> from other HTML elements?

 they inherit from / have-as-members a base class / DOM object which is neither auto-generated nor already existing in the Gdom* codebase: DataGridDataSource.

 in the JS case, this has been catered for, in the past ten months (since the original work was done in nov 2008) by creating WebCore/bindings/js/JSDataGridDataSource.{cpp,h}, and by adding WebCore/bindings/js/JSDataGridColumnListCustom.cpp and much more.

none of which i want to duplicate / get involved with, right now.

all of this is one of the key reasons behind submitting a static manually-generated file.

at least then, as the rest of webkit continues to move forward, leaving the GObject bindings behind, i don't have so much work to do to keep up.

[Luke Kenneth Casson Leighton]

Created attachment 34164 [details]
tidier workaround for skipping DataGrid

hiya eric, ok what do you think of this, i put in an explanation and a FIXME to say it's got to go at some point, but the main thing is that the skip test is just a single function, not a dog's dinner cut/paste job.

[Eric Seidel (no email)]

Comment on attachment 34164 [details]
tidier workaround for skipping DataGrid

OK, looks fine.

[Luke Kenneth Casson Leighton]

great.  will add a TODO for #ifdef DATAGRID.

[Adam Barth]

Comment on attachment 34164 [details]
tidier workaround for skipping DataGrid

This patch fails to build:

perl -I WebCore/bindings/scripts WebCore/dom/make_names.pl --tags WebCore/html/HTMLTagNames.in --attrs WebCore/html/HTMLAttributeNames.in --factory --wrapperFactory --extraDefines " ENABLE_VIDEO=1 ENABLE_RUBY=1"
"my" variable $name masks earlier declaration in same scope at WebCore/dom/make_names.pl line 540.
"my" variable $name masks earlier declaration in same scope at WebCore/dom/make_names.pl line 551.
"my" variable $name masks earlier declaration in same scope at WebCore/dom/make_names.pl line 556.
"my" variable $name masks earlier declaration in same scope at WebCore/dom/make_names.pl line 540.
"my" variable $name masks earlier declaration in same scope at WebCore/dom/make_names.pl line 551.
"my" variable $name masks earlier declaration in same scope at WebCore/dom/make_names.pl line 556.
"my" variable $name masks earlier declaration in same scope at WebCore/dom/make_names.pl line 674.
"my" variable $name masks earlier declaration in same scope at WebCore/dom/make_names.pl line 674.
Global symbol "%tag" requires explicit package name at WebCore/dom/make_names.pl line 317.
syntax error at WebCore/dom/make_names.pl line 345, near ");"
syntax error at WebCore/dom/make_names.pl line 380, near ");"
syntax error at WebCore/dom/make_names.pl line 610, near ");"
syntax error at WebCore/dom/make_names.pl line 625, near ");"
syntax error at WebCore/dom/make_names.pl line 922, near ");"
Global symbol "$tagName" requires explicit package name at WebCore/dom/make_names.pl line 1084.
syntax error at WebCore/dom/make_names.pl line 1084, near ");"
Execution of WebCore/dom/make_names.pl aborted due to compilation errors.
Global symbol "%tag" requires explicit package name at WebCore/dom/make_names.pl line 317.
syntax error at WebCore/dom/make_names.pl line 345, near ");"
syntax error at WebCore/dom/make_names.pl line 380, near ");"
syntax error at WebCore/dom/make_names.pl line 610, near ");"
syntax error at WebCore/dom/make_names.pl line 625, near ");"
syntax error at WebCore/dom/make_names.pl line 922, near ");"
Global symbol "$tagName" requires explicit package name at WebCore/dom/make_names.pl line 1084.
syntax error at WebCore/dom/make_names.pl line 1084, near ");"
Execution of WebCore/dom/make_names.pl aborted due to compilation errors.
make: *** [HTMLNames.cpp] Error 255
make: *** Waiting for unfinished jobs....
make: *** [HTMLElementFactory.cpp] Error 255

[Luke Kenneth Casson Leighton]

(In reply to comment #29)
> syntax error at WebCore/dom/make_names.pl line 345, near ");"
> syntax error at WebCore/dom/make_names.pl line 380, near ");"
> syntax error at WebCore/dom/make_names.pl line 610, near ");"
> syntax error at WebCore/dom/make_names.pl line 625, near ");"
> syntax error at WebCore/dom/make_names.pl line 922, near ");"

 nuts.  yep.  missed out a bracket.

 sorry about that - late night.  missed out a compile-test before submitting.

[Luke Kenneth Casson Leighton]

Created attachment 34262 [details]
version that works.

sooorryy, eric, andrew.  this version i've double-checked this time, and also confirmed that the output it auto-generates is identical (JS{HTML,SVG}ElementWrapperFactory.cpp,h)

[Eric Seidel (no email)]

Comment on attachment 34262 [details]
version that works.

Still looks OK.

[Adam Barth]

Comment on attachment 34262 [details]
version that works.

commit-queue can't land this because the ChangeLog entry isn't at the top of the file.

[Luke Kenneth Casson Leighton]

(In reply to comment #33)
> (From update of attachment 34262 [details])
> commit-queue can't land this because the ChangeLog entry isn't at the top of
> the file.

 i'm manually maintaining 15 separate patches to deal with this functionality, and that's about to go up to about 25 very shortly.

 in order to stop the patches conflicting, i'm interspersing the ChangeLog entries at different points, so that the [currently] 15 entries don't munge into one big globular commit.

 can you simply move the entry to the top?

 do you have a script which does that, already, surely?

[Gour]

Created attachment 34472 [details]
uploaded

[Eric Seidel (no email)]

Why does hte ChangeLog diff have no context?  Also you should obsolete old patches if you're posting a new one.

[Eric Seidel (no email)]

Comment on attachment 34472 [details]
uploaded

See my comments in the mail to you.

[Mark Rowe (bdash)]

Comment on attachment 34262 [details]
version that works.

> +#die "You must specify a --bindingType <type> e.g. Gdom or JS" unless (length($binding));
> +

We don't land commented-out code.

> +# FIXME: make_names is being utilised for six different purposes,
> +# with generation of HTMLNames.cpp/h being done at the same time
> +# as the JS bindings wrapper factories.

It's not clear how this comment relates to the code around it.

> +# FIXME: this is here because the gobject bindings were created in
> +# november 2008, when #ifdef DATAGRID did not exist.  to avoid
> +# significant extra work in getting the gobject bindings landed,
> +# and to also ensure that webkit-gtk will still compile even when
> +# DATAGRID is enabled, this workaround skips the #include
> +# of headers which the CodeGeneratorGObject.pm does not yet create.

Rather than adding a hack like this to the script we should just fix the related issues.

> @@ -313,6 +342,8 @@
>  
>          $uniqueTags{$interfaceName} = '1';
>  
> +        next if (temporaryWorkaroundSkipTag($tagName));
> +

There are redundant braces here.

>          my $conditional = $tags{$tagName}{"conditional"};
>          if ($conditional) {
>              my $conditionalString = "ENABLE(" . join(") && ENABLE(", split(/&/, $conditional)) . ")";
> @@ -346,6 +377,8 @@
>  
>      for my $tagName (sort keys %tagConstructorMap) {
>  
> +        next if (temporaryWorkaroundSkipTag($tagName));
> +

And here.  And in other similar places.

> +sub printWrapperFunctionGdomInterface
> +{
> +    my $F = shift;
> +    my $JSInterfaceName = shift;

This seems like a poor choice of variable name.

> +            } 
> +            if ($binding eq 'Gdom') {
> +                printWrapperFunctionGdomInterface($F, $JSInterfaceName);
> +            }

As does this.

> +            if ($binding eq 'Gdom') {
> +                printWrapperFunctionGdomInterface($F, $JSInterfaceName);
> +            }
>          }


In general I don't think that a bunch of "if ($binding eq 'Foo')" is a good way to structure this code.  Given the number of issues I've mentioned so far I'm marking this as r-.

[Eric Seidel (no email)]

Given the history and controversy surrounding this bug, I am closing it, and I recommend someone wishing to continue with this work start again with a new bug.  See my email to webkit-dev entitled "GDOM patch spam".