StrictTypeChecking extended attribute fails for methods with sequence<T>

[Antonio Gomes]

it tries to include JSSequence.h, which does not exist.

[Antonio Gomes]

Hey Chris, I have a simple patch for this that basically just adds a further if check, and works find.

See https://gist.github.com/tonikitoo/ca92684ace9f226d8060

I am wondering if you have feedback and what is the way to test it (if any)? TestObj.idl?

[Antonio Gomes]

Created attachment 211812 [details]
patch

[WebKit Commit Bot]

Attachment 211812 [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/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.h', u'Source/WebCore/bindings/scripts/test/TestObj.idl']" exit_code: 1
Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3047:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 1 in 5 files


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

[Antonio Gomes]

Created attachment 211825 [details]
actual patch

[Chris Dumez]

Comment on attachment 211825 [details]
actual patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2925
> +                if (!IsNativeType($argType) && !$codeGenerator->GetSequenceType($argType)) {

Based on the comment above, the type needs to be an interface. I believe the check we really want is:
if ($codeGenerator->IsWrapperType($argType))

This is exclude native types, sequences, arrays, basically everything that is not a wrapper type.
Also note that there is another place in the script where the check needs to be updated.

[Chris Dumez]

Comment on attachment 211825 [details]
actual patch

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

> Source/WebCore/bindings/scripts/test/TestObj.idl:227
> +    [StrictTypeChecking, RaisesException] bool strictFunctionWithSequence(TestObj objArg, sequence<unsigned long> a);

Please add a test with an array as well.

[Antonio Gomes]

Created attachment 212014 [details]
patch - addressing Chris' comments

Here is an updated patch. Chris, if you have any additional suggestion, please advice.

Otherwise, it should be good to land.

[Chris Dumez]

Comment on attachment 212014 [details]
patch - addressing Chris' comments

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

LGTM with a bit.

> Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp:-357
> -    if (exec->argumentCount() > 0 && !exec->argument(0).isUndefinedOrNull() && !exec->argument(0).inherits(JSlong[]::info()))

Ah, we fixed another bug :)

> Source/WebCore/bindings/scripts/test/TestObj.idl:227
> +    [StrictTypeChecking, RaisesException] bool strictFunctionWithSequence(TestObj objArg, sequence<unsigned long> a);

nit: It would be nice to add a method taking an array as well.

[Chris Dumez]

Comment on attachment 212014 [details]
patch - addressing Chris' comments

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

LGTM with a nit.

> Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp:-357
> -    if (exec->argumentCount() > 0 && !exec->argument(0).isUndefinedOrNull() && !exec->argument(0).inherits(JSlong[]::info()))

Ah, we fixed another bug :)

> Source/WebCore/bindings/scripts/test/TestObj.idl:227
> +    [StrictTypeChecking, RaisesException] bool strictFunctionWithSequence(TestObj objArg, sequence<unsigned long> a);

nit: It would be nice to add a method taking an array as well.

[Antonio Gomes]

 
> > Source/WebCore/bindings/scripts/test/TestObj.idl:227
> > +    [StrictTypeChecking, RaisesException] bool strictFunctionWithSequence(TestObj objArg, sequence<unsigned long> a);
> 
> nit: It would be nice to add a method taking an array as well.

Done: http://trac.webkit.org/changeset/156157

[Alexey Proskuryakov]

This broke bindings tests, <http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/13106/steps/bindings-generation-tests/logs/stdio>. Antonio, are you around to fix these now?

[Antonio Gomes]

(In reply to comment #11)
> This broke bindings tests, <http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/13106/steps/bindings-generation-tests/logs/stdio>. Antonio, are you around to fix these now?

Hi ap. After looking at the failures, they are non-JS especific results that need rebaseline. I on propose only updated the JS ones, because I fixed a bug in CodeGeneratorJS.pm only.

(GObject seems to generated odd results)

So, if the policy just rebaseline all binding generator results, even if it is still broken (see below), I can do that.

Please advice.


FAIL: (CPP) WebDOMTestObj.h
--- WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h	2013-07-11 08:13:02.000000000 -0700
+++ /var/folders/81/qcmw8mp11lvdf69qgyzyl2000000gn/T/tmpeC9T35/WebDOMTestObj.h	2013-09-20 08:38:22.000000000 -0700
@@ -41,6 +41,7 @@
 class WebDOMTestNode;
 class WebDOMTestObj;
 class WebDOMbool;
+class WebDOMlong[];

It looks wrong, but possibly because the code generator is buggy.

[Antonio Gomes]

Reopening to commit a temporary band-aid patch.

[Antonio Gomes]

Created attachment 212182 [details]
patch - make added tests JS-only

[WebKit Commit Bot]

Comment on attachment 212182 [details]
patch - make added tests JS-only

Clearing flags on attachment: 212182

Committed r156175: <http://trac.webkit.org/changeset/156175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.