[webkit-reviews] review denied: [Bug 73162] Implement [Supplemental] IDL and support it in run-bindings-tests : [Attachment 116738] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kentaro Hara <haraken at chromium.org>'s
request for review:
Bug 73162: Implement [Supplemental] IDL and support it in run-bindings-tests
https://bugs.webkit.org/show_bug.cgi?id=73162

Attachment 116738: Patch
https://bugs.webkit.org/attachment.cgi?id=116738&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116738&action=review


> Source/WebCore/bindings/scripts/CodeGenerator.pm:575
>      if ($conditional =~ /&/) {
> -	   return "ENABLE(" . join(") && ENABLE(", split(/&/, $conditional)) .
")";
> +	   # Avoid duplicated conditions.
> +	   my %conditions;
> +	   map { $conditions{$_} = 1 } split(/&/, $conditional);
> +	   return "ENABLE(" . join(") && ENABLE(", sort keys %conditions) .
")";
>      } elsif ($conditional =~ /\|/) {
> -	   return "ENABLE(" . join(") || ENABLE(", split(/\|/, $conditional)) .
")";
> +	   # Avoid duplicated conditions.
> +	   my %conditions;
> +	   map { $conditions{$_} = 1 } split(/\|/, $conditional);
> +	   return "ENABLE(" . join(") || ENABLE(", sort keys %conditions) .
")";

This feels copy-pasted.  Should we break this out into a helper function?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1654
> +			   push(@implContent, "    return
${implementedBy}::$implGetterFunctionName(castedThis, exec);\n");

This doesn't seem quite right.	$implementedBy will be a WebCore-proper class,
which means it shouldn't receive an ExecState.	Perhaps we should write
"JS${implementedBy}::$implGetterFunctionName(" ?  (We can also make Custom and
ImplementedBy an error to use together for now.)

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1948
>		   }

Looks like you're only handling getters and setters.  It seems like we should
handle normal member functions as well.  We can do that in this patch or the
next patch, whichever you prefer.

> Source/WebCore/bindings/scripts/generate-bindings.pl:141
> +		       # Record that this attribute is implemented by
$interfaceName.
> +		      
$attribute->signature->extendedAttributes->{"ImplementedBy"} = $interfaceName;

Should we move this outside the inner loop?  It seems like we'll end up make
this assignment many times redundantly.

> Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterface.mm:76
> +    WebCore::JSMainThreadNullState state;
> +    return IMPL->str1();

This won't compile, right?  We might need to modify the other code generators
as well.