Bug 67666 - Binding CodeGenerators don't support Conditional= on constants
Summary: Binding CodeGenerators don't support Conditional= on constants
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
: 67450 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-09-06 13:12 PDT by Aaron Colwell
Modified: 2011-11-23 14:50 PST (History)
6 users (show)

See Also:


Attachments
A patch that generates the errors (2.99 KB, patch)
2011-09-06 13:13 PDT, Aaron Colwell
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Colwell 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.
Comment 1 Aaron Colwell 2011-09-06 13:13:35 PDT
Created attachment 106472 [details]
A patch that generates the errors
Comment 2 Aaron Colwell 2011-09-06 13:14:33 PDT
Created attachment 106473 [details]
Errors generated by the patch.
Comment 3 Erik Arvidsson 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
Comment 4 Erik Arvidsson 2011-11-22 16:56:29 PST
Created attachment 116296 [details]
Patch
Comment 5 Adam Barth 2011-11-22 17:01:24 PST
Comment on attachment 116296 [details]
Patch

Nice patch.  We should share more code like this.
Comment 6 WebKit Review Bot 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.
Comment 7 WebKit Review Bot 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>
Comment 8 WebKit Review Bot 2011-11-23 14:03:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Patrick R. Gansterer 2011-11-23 14:50:33 PST
*** Bug 67450 has been marked as a duplicate of this bug. ***