Bug 108799 – [CPP,GObj,ObjC] Add checks for unsupported Web IDL features, remove #if V8/JS

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Nils Barth]

Currently IDLs (as in TestObj.idl) need to have an #if V8/JS around Web IDL features that aren't supported in [CPP,GObj,ObjC].
This patch adds checks in the code generators CodeGenerator{CPP,GObj,ObjC}.pm so these automatically skip these, avoiding generating incorrect code and allowing the IDL file itself to be clean.
It also removes a spurious check on a feature that actually is supported, and rebases tests to fix two mistakes in current test results.

Concretely, this adds the following checks:
1. Skip “Callback” extended attribute on operations

2. Remove #if around “CachedAttribute” extended attribute on attributes, as already supported, and rebase to add to tests:
./Source/WebCore/page/History.idl
./Source/WebCore/dom/MessageEvent.idl
./Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl

3. Skip overloaded operations, and rebase to remove accidentally included overloaded operation
CPP, GObject, ObjC manifestly don’t support overloads -- they don't call the LinkOverloadedFunctions function from CodeGenerator.pm -- but in TestObj.idl the overloadedMethod1 operation incorrectly isn't in an #if (unlike overloadedMethod, which was), so need to rebase.

4. Skip “DOMStringList” return type

5. [GObj] Skip array return type (to handle DOMString[])
Just missing check (has check for array in attributes, but not function return).

[Nils Barth]

Created attachment 186336 [details]
First draft

Makes changes as described in Description/first comment.    
Changes are quite simple, but there are a lot of them,
so I've put details in ChangeLog.
I don't believe that the changes make the CodeGenerators skip any 
code they can legitimately generate, so shouldn't change existing generated
code (or if it does, existing bindings may be broken), but someone
more familiar with them may want to check.
BTW, the check-webkit-style failures are b/c CodeGeneratorGObject.pm
generates non-compliant code for g_object_class_install_property
(unrelated issue).

[WebKit Review Bot]

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

Failed 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/CodeGeneratorObjC.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/ObjC/DOMTestObj.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.mm', u'Source/WebCore/bindings/scripts/test/TestObj.idl']" exit_code: 1
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:823:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:825:  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:826:  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:827:  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:828:  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:830:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:832:  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:833:  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:834:  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:835:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 10 in 11 files


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

[Kentaro Hara]

Comment on attachment 186336 [details]
First draft

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

> Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm:189
> +    if ($codeGenerator->GetArrayType($type)) {
> +        return 1;
> +    }
> +
> +    if ($codeGenerator->GetSequenceType($type)) {
> +        return 1;
> +    }

Are all array types and sequence types not supported in CodeGeneratorCPP? (not only DOMStringList and DOMString[])

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:260
> +    if ($codeGenerator->GetArrayType($functionReturnType)) {
> +        return 1;
> +    }

Ditto. Also, what about $codeGenerator->GetSequenceType() ?

> Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:438
> +    return 1 if $codeGenerator->GetSequenceType($type);
> +    return 1 if $codeGenerator->GetArrayType($type);

Ditto.

[Build Bot]

Comment on attachment 186336 [details]
First draft

Attachment 186336 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16369164

[Build Bot]

Comment on attachment 186336 [details]
First draft

Attachment 186336 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16375125

[Nils Barth]

To summarize:
* Most skip checks are existing checks, just mixed with new changes in diff.
* Check for array return on function in GObject *is* new, and took some looking.
Q: Proper order is Sequence then Array -- should I fix this in CPP? (minor)

(In reply to comment #3)
> (From update of attachment 186336 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186336&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm:189
> > +    if ($codeGenerator->GetArrayType($type)) {
> > +        return 1;
> > +    }
> > +
> > +    if ($codeGenerator->GetSequenceType($type)) {
> > +        return 1;
> > +    }
> 
> Are all array types and sequence types not supported in CodeGeneratorCPP? (not only DOMStringList and DOMString[])

Yes, array an sequence are not supported; these are not new tests
(it just looks weird b/c I'm making a few changes).

I added
my $type = $function->signature->type
when testing for DOMStringList -- the new code is more legible,
but b/c I'm adding 2 other tests immediately below, the diff mixes the two.

Before:
    if ($codeGenerator->GetArrayType($function->signature->type)) {
        return 1;
    }

    if ($codeGenerator->GetSequenceType($function->signature->type)) {
        return 1;
    }


After:
    if ($codeGenerator->GetArrayType($type)) {
        return 1;
    }

    if ($codeGenerator->GetSequenceType($type)) {
        return 1;
    }

    if ($type eq "DOMStringList") {
        return 1;
    }

    if ($function->{overloads} && @{$function->{overloads}} > 1) {
        return 1;
    }

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:260
> > +    if ($codeGenerator->GetArrayType($functionReturnType)) {
> > +        return 1;
> > +    }
> 
> Ditto.

This check for array return on function in GObject *is* new;

This took a bit of looking at, and I noted it in the ChangeLog
(it was the last change I noticed and made):
5. [GObj] Skip array return type (to handle DOMString[])

GObject already does not allow array type on attributes, or sequence types anywhere -- the only time it checks for GetArrayType is to skip these, never to generate code, so I'm pretty sure this is just an omission, which I'm fixing.

> Also, what about $codeGenerator->GetSequenceType() ?

$codeGenerator->GetSequenceType() is above this code, hence not in patch.

Following Web IDL, one should have Sequence before Array, as per:
3.10.22. Sequences — sequence<T>
http://www.w3.org/TR/WebIDL/#idl-sequence
3.10.23. Arrays — T[]
http://www.w3.org/TR/WebIDL/#idl-array

GObject and ObjC correctly have sequence before array, but CPP flips them.
I didn't fix this order in CodeGeneratorCPP.pm (seemed a bit minor);
should I?

> > Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:438
> > +    return 1 if $codeGenerator->GetSequenceType($type);
> > +    return 1 if $codeGenerator->GetArrayType($type);
> 
> Ditto.

As above, not a new change -- just the $type = $function->signature->type getting mixed with the two new tests.

Before:
    return 1 if $codeGenerator->GetSequenceType($function->signature->type);
    return 1 if $codeGenerator->GetArrayType($function->signature->type);

After:
    return 1 if $codeGenerator->GetSequenceType($type);
    return 1 if $codeGenerator->GetArrayType($type);
    return 1 if $type eq "DOMStringList";

    if ($function->{overloads} && @{$function->{overloads}} > 1) {
        return 1;
    }

[Kentaro Hara]

(In reply to comment #6)
> Q: Proper order is Sequence then Array -- should I fix this in CPP? (minor)

I don't think the order matters. Let's make the order consistent for readability.

Please fix build errors on mac (ObjC changes would be the culprit).

[Nils Barth]

(In reply to comment #7)
> (In reply to comment #6)
> > Q: Proper order is Sequence then Array -- should I fix this in CPP? (minor)
> 
> I don't think the order matters. Let's make the order consistent for readability.

Ok -- I'll fix CPP and check consistent order.

> Please fix build errors on mac (ObjC changes would be the culprit).

Will do!
Looks like problem is in WebCore/dom/Document.idl
...or one of its dependencies.

[kov's GTK+ EWS bot]

Comment on attachment 186336 [details]
First draft

Attachment 186336 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16367217

[Nils Barth]

(In reply to comment #9)
> (From update of attachment 186336 [details])
> Attachment 186336 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/16367217

GObject binding build failure, looks like GObject does support (and requires) DOMStringList.
Will fix, and try building locally to avoid noise.

Exact failure looks like:

DerivedSources/webkitdom/WebKitDOMDOMSecurityPolicy.cpp: In function 'void webkit_dom_dom_security_policy_get_property(GObject*, guint, GValue*, GParamSpec*)':
DerivedSources/webkitdom/WebKitDOMDOMSecurityPolicy.cpp:135:56: error: no matching function for call to 'kit(WebCore::DOMStringList*)'
DerivedSources/webkitdom/WebKitDOMDOMSecurityPolicy.cpp:135:56: note: candidates are:
../../Source/WebCore/bindings/gobject/WebKitDOMBinding.h:41:16: note: WebKitDOMNode* WebKit::kit(WebCore::Node*)
../../Source/WebCore/bindings/gobject/WebKitDOMBinding.h:41:16: note:   no known conversion for argument 1 from 'WebCore::DOMStringList*' to 'WebCore::Node*'

...which seems pretty clearly DOMStringList.

[Nils Barth]

Comment on attachment 186336 [details]
First draft

(Clear review flag.)

[Nils Barth]

No longer relevant without V8, due to resulting larger changes.
(If want to remove checks for #if JS, please feel free to reopen/change.)