[webkit-reviews] review denied: [Bug 88671] generated js bindings should include headers for argument conditionally. : [Attachment 146632] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied arno. <arno at renevier.net>'s
request for review:
Bug 88671: generated js bindings should include headers for argument
conditionally.
https://bugs.webkit.org/show_bug.cgi?id=88671

Attachment 146632: Patch
https://bugs.webkit.org/attachment.cgi?id=146632&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146632&action=review


>> Source/WebCore/ChangeLog:1
>> +2012-06-08	Arnaud Renevier  <a.renevier at sisa.samsung.com>
> 
> 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.