generated js bindings should include headers for argument conditionally.

[arno.]

Hi,
If I have this idl file:

module mymodule {
    interface myIface {
        void foo();
        [Conditional=WEBGL] void bar(in WebGLBuffer bufobj);
    };
}

the bar property will be defined conditionally in the generated js binding.
But the  JSWebGLBuffer.h will still be included unconditionally.

JSWebGLBuffer.h should be included only #if ENABLE(WEBGL) is true.

[arno.]

Created attachment 146632 [details]
Patch

patch proposal

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:7:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 2 files


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

[Kentaro Hara]

Comment on attachment 146632 [details]
Patch

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

>> Source/WebCore/ChangeLog:1
>> [phone]  Arnaud Renevier  <[e-mail]>
> 
> ChangeLog entry has no bug number  [changelog/bugnumber] [5]

The ChangeLog format is wrong. Please refer to other ChangeLogs. (e.g. https://bugs.webkit.org/attachment.cgi?id=139292&action=review)

> Source/WebCore/ChangeLog:4
> +

But URL is needed.

> Source/WebCore/ChangeLog:6
> +

Please explain what this patch is changing.

>> Source/WebCore/ChangeLog:7
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

Your test case is covered by bindings/scripts/test/TestObj.idl (conditionalMethod1(), conditionalMethod2(), conditionalMethod3()). So this line should be

Test: bindings/scripts/test/TestObj.idl

You need to update the test result of TestObj.idl. run-bindings-tests will update the result (See https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests).

> Source/WebCore/bindings/scripts/IDLParser.pm:256
> +        if (!$paramDataNode->extendedAttributes->{"Conditional"} and $newDataNode->signature->extendedAttributes->{"Conditional"}) {
> +            $paramDataNode->extendedAttributes->{"Conditional"} = $newDataNode->signature->extendedAttributes->{"Conditional"};
> +        }

I do not think this is the right way to fix the issue. I guess that the right fix would be to change bindings/scripts/CodeGeneratorJS.pm so that it generates the Conditional string for conditional methods.

[Alexey Proskuryakov]

Can you explain any this is something that needs to be fixed?

We have a huge mix of styles in this respect, but I don't think that we ever saw a strong argument that #ifdefs should be at include side.

For generated files, the solution is just to generate them.

[arno.]

My use case was that, when compiled without WEBGL, the header was still included, and the build did not work when webgl headers were not present on the machine.
But maybe, it's better to just use 


#if defined(WEBGL)
       void bar(in WebGLBuffer bufobj);
#endif

in the idl file

[Alexey Proskuryakov]

What I'm saying is that it's best to have JSWebGLBuffer.h generated even if the platform doesn't enable WebGL.