Bug 111728 – Implement support for nullable types in the bindings generator

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Peter Beverloo]

Implement support for nullable types in the bindings generator

[Peter Beverloo]

Created attachment 191992 [details]
Patch

[Peter Beverloo]

Hi Haraken-san!

Before implementing support for other generators as well, I'd like to know if I'm on the right direction.  I also have a few questions to ask:

  * IDLParser.pm takes the "old" code-path. I have updated parseAttributeRestOld, should I update parseAttributeRest as well?

  * The specification[1] lists a number of constraints: does not apply to "any" types, neither for certain union types, and so on.  Do we validate correctness of the IDL before generating code?

There are more places where nullable types can be used, i.e. constants.  I'll have to update parsing/code generation for those as well.  Also, I'll verify whether the the "TreatNullAs" directive works as expected.

[1] http://dev.w3.org/2006/webapi/WebIDL/#idl-nullable-type

[Adam Barth]

Comment on attachment 191992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191992&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1051
> +        if ($isNullable) {
> +            push(@implContentInternals, "    if (isNull)\n");
> +            push(@implContentInternals, "        return v8Null(info.GetIsolate());\n");
>          }

I don't understand.  We basically assume that all attributes are nullable in our implementation.  The way we implement non-nullable attributes is by never returning null from the implementation.

I would have expected this patch to be an ASSERT that we don't return null from non-nullable attributes.

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 191992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191992&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1051
> > +        if ($isNullable) {
> > +            push(@implContentInternals, "    if (isNull)\n");
> > +            push(@implContentInternals, "        return v8Null(info.GetIsolate());\n");
> >          }
> 
> I don't understand.  We basically assume that all attributes are nullable in our implementation.  The way we implement non-nullable attributes is by never returning null from the implementation.

This approach works for XXX* but won't work for double, int etc.

Our current context is how we can remove custom bindings for V8DeviceOrientationEvent::gammaAttrGetterCustom() (in preparation for adding more features around Device events).

  v8::Handle<v8::Value> V8DeviceOrientationEvent::gammaAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
  {
      v8::Handle<v8::Object> holder = info.Holder();
      DeviceOrientationEvent* imp = V8DeviceOrientationEvent::toNative(holder);
      if (!imp->orientation()->canProvideGamma())
          return v8::Null();
      return v8::Number::New(imp->orientation()->gamma());
  }

We cannot judge if we should return v8Null() or not by "if(imp->orientation()->gamma()==0)". Given that imp->orientation()->gamma() can return any double values, we need some other way to know whether we should return v8Null() or not. Beverloo introduced an isNull parameter for that purpose.

[Peter Beverloo]

Created attachment 192228 [details]
Patch

[Peter Beverloo]

Haraken's explanation is correct, which I also briefly discussed with Adam over IRC yesterday.

The updated patch contains modifications for all code generators except for ObjC, which is throwing an error for me.  I'll figure that out.  Open questions at this point:

  * IDLParser.pm takes the "old" code-path. I have updated parseAttributeRestOld, should I update parseAttributeRest as well?

  * The specification lists a number of constraints: does not apply to "any" types, neither for certain union types, and so on.  Do we validate correctness of the IDL before generating code?

  * The GObject, CPP and ObjC generators need to return a non-null value.  Do we ignore the isNull return value, or return a "default" value for the type (i.e. 0 for numeric types, false for bool, and so on).

  * Do I include the output each code generator creates for the TestNullableAttributes.idl file in the patch?

Thanks!

[Sam Weinig]

(In reply to comment #6)

>   * Do I include the output each code generator creates for the TestNullableAttributes.idl file in the patch?

Please do.  It will make reviewing much easier.  When thinking about this in the past, I had considered adding a Nullable<>/OptionallyNull<> type to represent this possibility in WebCore C++ code.

[Kentaro Hara]

(In reply to comment #6)
>   * IDLParser.pm takes the "old" code-path. I have updated parseAttributeRestOld, should I update parseAttributeRest as well?

Yes, please.

>   * The specification lists a number of constraints: does not apply to "any" types, neither for certain union types, and so on.  Do we validate correctness of the IDL before generating code?

Not needed. We normally omit this kind of validation in the IDL parser (If we implemented all the validations, the parser would get extremely complicated:).

>   * The GObject, CPP and ObjC generators need to return a non-null value.  Do we ignore the isNull return value, or return a "default" value for the type (i.e. 0 for numeric types, false for bool, and so on).

Although in the future we want to support isNull in GObject, CPP and ObjC, for the time being it sounds reasonable to ignore the isNull return value. It is important not to change current behavior of those code generators.

>   * Do I include the output each code generator creates for the TestNullableAttributes.idl file in the patch?

Yes!

[Kentaro Hara]

Comment on attachment 192228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192228&action=review

> Source/WebCore/bindings/scripts/IDLParser.pm:2404
> +        my $type = $self->parseType();
> +        if ($type =~ /\?$/) {
> +            $newDataNode->signature->isNullable(1);
> +        } else {
> +            $newDataNode->signature->isNullable(0);
> +        }
> +        # Remove all "?" in the type declaration, e.g. "double?" -> "double".
> +        $type =~ s/\?//g;

The IDL parser is written strictly following the IDL grammar (http://www.w3.org/TR/WebIDL/#idl-grammar) and we don't want to add ad-hoc regular expressions. Would you write parseTypeSuffix() or parserNull() according to the grammar?

> Source/WebCore/bindings/scripts/test/TestNullableAttributes.idl:29
> +interface TestNullableAttributes {

You don't need to add a new test file. You can just add new attributes to TestObj.idl or TestInterface.idl.

[Peter Beverloo]

(In reply to comment #7)
> (In reply to comment #6)
> 
> >   * Do I include the output each code generator creates for the TestNullableAttributes.idl file in the patch?
> 
> Please do.  It will make reviewing much easier.  When thinking about this in the past, I had considered adding a Nullable<>/OptionallyNull<> type to represent this possibility in WebCore C++ code.

Ok, I've included the output.  I don't have a very strong preference for either an isNull attribute or adding a Nullable<> type so I've kept it as it is for now.

(In reply to comment #8)
> (In reply to comment #6)
> >   * IDLParser.pm takes the "old" code-path. I have updated parseAttributeRestOld, should I update parseAttributeRest as well?
> 
> Yes, please.

Done.
 
> >   * The specification lists a number of constraints: does not apply to "any" types, neither for certain union types, and so on.  Do we validate correctness of the IDL before generating code?
> 
> Not needed. We normally omit this kind of validation in the IDL parser (If we implemented all the validations, the parser would get extremely complicated:).

I see, ok.  The current parsing routines already seem to do a lot of checking in the existing Parse* functions.
 
> >   * The GObject, CPP and ObjC generators need to return a non-null value.  Do we ignore the isNull return value, or return a "default" value for the type (i.e. 0 for numeric types, false for bool, and so on).
> 
> Although in the future we want to support isNull in GObject, CPP and ObjC, for the time being it sounds reasonable to ignore the isNull return value. It is important not to change current behavior of those code generators.

I agree.  They do have to call the WebCore methods with the right signature, though.

> >   * Do I include the output each code generator creates for the TestNullableAttributes.idl file in the patch?
> 
> Yes!

Done.

(In reply to comment #9)
> (From update of attachment 192228 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192228&action=review
> 
> > Source/WebCore/bindings/scripts/IDLParser.pm:2404
> > +        my $type = $self->parseType();
> > +        if ($type =~ /\?$/) {
> > +            $newDataNode->signature->isNullable(1);
> > +        } else {
> > +            $newDataNode->signature->isNullable(0);
> > +        }
> > +        # Remove all "?" in the type declaration, e.g. "double?" -> "double".
> > +        $type =~ s/\?//g;
> 
> The IDL parser is written strictly following the IDL grammar (http://www.w3.org/TR/WebIDL/#idl-grammar) and we don't want to add ad-hoc regular expressions. Would you write parseTypeSuffix() or parserNull() according to the grammar?

The parseTypeSuffix() method already exists and is being called in parseNonAnyType(), which appends it to the type name.  Other methods, such as parseOptionalOrRequiredArgument, use this same regular expression to achieve the same goal.

These are the third and fourth occurrences of that structure in the code, each of which wants to separate the nullable ("?") suffix from the type's name.  A solution would be for parseType() to return a structure with the type name and suffix separated, but there are 30 callers so only 10% cares about the suffix.  We could generalize the regular expressions by moving them in two utility methods, which would change to code to read:

my $type = $self->parseType();
if (typeHasNullableSuffix($type)) {
    $newDataNode->signature->isNullable(1);
    $type = typeRemoveNullableSuffix($type);
} else {
    $newDataNode->signature->isNullable(0);
}

While it would increase readability, such utility methods are rare (only unquoteString exists right now) in the parser.  In either case, I'm unsure how to handle this in case of more complicated compositions such as "type?[][]?[]", which, per the grammar, are valid.  The existing code doesn't take this into account either.

> > Source/WebCore/bindings/scripts/test/TestNullableAttributes.idl:29
> > +interface TestNullableAttributes {
> 
> You don't need to add a new test file. You can just add new attributes to TestObj.idl or TestInterface.idl.

Ok, that significantly reduces the noise in the diff.

[Peter Beverloo]

Created attachment 193118 [details]
Patch

[WebKit Review Bot]

Attachment 193118 [details] did not pass style-queue:

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:936:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:938:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:939:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:940:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:943:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:945:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:947:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:948:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:949:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:952:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:954:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:956:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:957:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:958:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:959:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:961:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:963:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:964:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:965:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:966:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:968:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:970:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:971:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:972:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:975:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scriptsFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/IDLParser.pm', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.mm', u'Source/WebCore/bindings/scripts/test/TestObj.idl', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp']" exit_code: 1
/test/GObject/WebKitDOMTestObj.cpp:977:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:979:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:980:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:981:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:984:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 30 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Comment on attachment 193118 [details]
Patch

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!

[Peter Beverloo]

Created attachment 193303 [details]
Patch

[Peter Beverloo]

(In reply to comment #13)
> Nit: WebKit coding style requires:
> 
> if (A)
>   return X;
> if (B)
>   return Y;
> return Z;
>

We can address this in the patch for GObject/CPP and ObjC.

> 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.

The signature of the WebCore methods will change once they start using nullable types.  For example, a non-custom binding for:

readonly attribute double? myDouble;

will change the signature from "double myDouble() const;" to "double myDouble(bool& isNull) const;". I don't understand why we don't have to implement this now, as using this feature will now effectively break bindings.

I'll upload a separate patch to address these three code generators, only making sure that the calls to the WebCore methods have the correct signatures.

> Ditto.

Done..

[Peter Beverloo]

Created attachment 193306 [details]
Rebase

[Peter Beverloo]

Created attachment 193308 [details]
Fix ChangeLog

[Kentaro Hara]

(In reply to comment #15)
> readonly attribute double? myDouble;
> 
> will change the signature from "double myDouble() const;" to "double myDouble(bool& isNull) const;". I don't understand why we don't have to implement this now, as using this feature will now effectively break bindings.
> 
> I'll upload a separate patch to address these three code generators, only making sure that the calls to the WebCore methods have the correct signatures.

I'm very sorry about it... You're completely right. Would you again include the change to this patch?

[Peter Beverloo]

Created attachment 193318 [details]
Full patch

[Kentaro Hara]

Comment on attachment 193318 [details]
Full patch

LGTM. I'm very sorry I've asked you redundant work...

[Peter Beverloo]

(In reply to comment #20)
> (From update of attachment 193318 [details])
> LGTM. I'm very sorry I've asked you redundant work...

Done. No problem, and thank you for the review!!

[WebKit Review Bot]

Attachment 193318 [details] did not pass style-queue:

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:936:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:938:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:939:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:940:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:943:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:945:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:947:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:948:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:949:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:952:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:954:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:956:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:957:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:958:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:959:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:961:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:963:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:964:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:965:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:966:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:968:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:970:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:971:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:972:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:975:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scriptsFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/IDLParser.pm', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.mm', u'Source/WebCore/bindings/scripts/test/TestObj.idl', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp']" exit_code: 1
/test/GObject/WebKitDOMTestObj.cpp:977:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:979:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:980:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:981:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:984:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 30 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 193318 [details]
Full patch

Clearing flags on attachment: 193318

Committed r145907: <http://trac.webkit.org/changeset/145907>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

Comment on attachment 193318 [details]
Full patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193318&action=review

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:995
> +    JSValue result = jsNumber(impl->nullableDoubleAttribute(isNull));
> +    if (isNull)
> +        return jsNull();
> +    return result;
> +}

This seems quite inefficient.  You are creating the JSValue just to throw it out.  Please rework this.

[Peter Beverloo]

(In reply to comment #25)
> (From update of attachment 193318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193318&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:995
> > +    JSValue result = jsNumber(impl->nullableDoubleAttribute(isNull));
> > +    if (isNull)
> > +        return jsNull();
> > +    return result;
> > +}
> 
> This seems quite inefficient.  You are creating the JSValue just to throw it out.  Please rework this.

I only copied the semantics of what exceptions already do.  I'll upload a patch to rework this for JSC.

[Peter Beverloo]

Re-opened since this is blocked by bug 112711

[Peter Beverloo]

Sorry -- no need to reopen this.