[webkit-reviews] review denied: [Bug 111728] Implement support for nullable types in the bindings generator : [Attachment 193118] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Peter Beverloo
<peter at chromium.org>'s request for review:
Bug 111728: Implement support for nullable types in the bindings generator
https://bugs.webkit.org/show_bug.cgi?id=111728

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=193118&action=review


Looks good except for the changes in CodeGenerator{ObjC,CPP,GObject}.pm.

> Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm:567
>      if (!defined($returnType) or $returnType eq "void") {
> -	   $returnType = "";
> +	   return "";
>      } elsif ($codeGenerator->IsPrimitiveType($returnType)) {
> -	   $returnType = " 0";
> +	   return " 0";
>      } elsif ($returnType eq "bool") {
> -	   $returnType = " false";
> +	   return " false";
>      } else {
> -	   $returnType = " $returnType()";
> +	   return " $returnType()";
>      }

Nit: WebKit coding style requires:

if (A)
  return X;
if (B)
  return Y;
return Z;

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1015
> +    # FIXME: When creating a getter for a nullable attribute, should a
default value
> +    # be returned by the API is isNull=true?
> +    if ($function->signature->isNullable) {
> +	   push(@cBody, "    bool isNull = false;\n");
> +	   push(@callImplParams, "isNull");
> +    }
> +

I think you don't need to touch CodeGenerator{ObjC,CPP,GObject}.pm in this
patch. You can just ignore Nullable in these code generators so that generated
code is kept the same between before and after your patch.

I'm not intending to say that we want to ignore these code generators. For the
time being, it would be safer to keep the generated code the same than changing
their behavior with uncertainty (you are adding FIXMEs). Yu can change these
code generators in a separate patch.

> Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:1363
> +		   # FIXME: Should we return a "default" value for the return
type
> +		   # when isNull=true?

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:305
> +sub typeHasNullableSuffix
> +{
> +    my $type = shift;
> +    return $type =~ /\?$/;
> +}
> +
> +sub typeRemoveNullableSuffix
> +{
> +    my $type = shift;
> +    $type =~ s/\?//g;
> +    return $type;
> +}

Looks better!