Bug 67666

Summary: Binding CodeGenerators don't support Conditional= on constants
Product: WebKit Reporter: Aaron Colwell <acolwell>
Component: New BugsAssignee: Erik Arvidsson <arv>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, arv, japhet, ojan, paroga, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
A patch that generates the errors
none
Errors generated by the patch.
none
Patch none

Aaron Colwell
Reported 2011-09-06 13:12:41 PDT
It appears that at least CodeGeneratorV8.pm does not honor the Conditional= directive on constants and functions specified in the IDL. I encountered this while trying to convert my MediaSource API methods from using an "#if defined(ENABLE_MEDIA_SOURCE)..." block to [Conditional=MEDIA_SOURCE] on each of the items in the block. I've attached a patch for the changes. When I apply this patch I get a bunch of compiler errors (see attached) that appear to indicate that the code generator completely ignored the Condition annotation on the functions and constants. I'd expect the code generator to not emit code for functions and constants if the Conditional value is 0 or not set like it does for attributes.
Attachments
A patch that generates the errors (2.99 KB, patch)
2011-09-06 13:13 PDT, Aaron Colwell 		no flags
DetailsFormatted DiffDiff
Errors generated by the patch. (7.58 KB, text/plain)
2011-09-06 13:14 PDT, Aaron Colwell 		no flags
Details
Patch (25.64 KB, patch)
2011-11-22 16:56 PST, Erik Arvidsson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Aaron Colwell
Comment 1 2011-09-06 13:13:35 PDT
Created attachment 106472 [details] A patch that generates the errors
Aaron Colwell
Comment 2 2011-09-06 13:14:33 PDT
Created attachment 106473 [details] Errors generated by the patch.
Erik Arvidsson
Comment 3 2011-11-21 16:59:11 PST
I can duplicate the issue with conditional consts but we have code that uses conditional functions so at least that should work: http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/xml/XMLHttpRequest.idl&exact_package=chromium&q=(%5Cs)*%5C%5BConditional=%20file:%5C.idl&type=cs&l=77
Erik Arvidsson
Comment 4 2011-11-22 16:56:29 PST
Created attachment 116296 [details] Patch
Adam Barth
Comment 5 2011-11-22 17:01:24 PST
Comment on attachment 116296 [details] Patch Nice patch. We should share more code like this.
WebKit Review Bot
Comment 6 2011-11-22 17:03:31 PST
Attachment 116296 [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/bindings/scripts/test/JS/JSTestObj.cpp:172: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1921: jsTestObjCONDITIONAL_CONST is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/JS/JSTestObj.h:266: jsTestObjCONDITIONAL_CONST is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1472: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 7 2011-11-23 14:03:37 PST
Comment on attachment 116296 [details] Patch Clearing flags on attachment: 116296 Committed r101102: <http://trac.webkit.org/changeset/101102>
WebKit Review Bot
Comment 8 2011-11-23 14:03:45 PST
All reviewed patches have been landed. Closing bug.
Patrick R. Gansterer
Comment 9 2011-11-23 14:50:33 PST
*** Bug 67450 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.