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).
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).
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.
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.
Comment on attachment 186336 [details] First draft Attachment 186336 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16369164
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
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; }
(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).
(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.
Comment on attachment 186336 [details] First draft Attachment 186336 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16367217
(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.
Comment on attachment 186336 [details] First draft (Clear review flag.)
No longer relevant without V8, due to resulting larger changes. (If want to remove checks for #if JS, please feel free to reopen/change.)