[webkit-reviews] review denied: [Bug 27425] adding auto-generator support for GDOMHTMLElementWrapperFactory[.cpp/.h] : [Attachment 34262] version that works.

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied	review:
Bug 27425: adding auto-generator support for
GDOMHTMLElementWrapperFactory[.cpp/.h]
https://bugs.webkit.org/show_bug.cgi?id=27425

Attachment 34262: version that works.
https://bugs.webkit.org/attachment.cgi?id=34262&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> +#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-.