Bug 150536 - Builtins generator should emit ENABLE(FEATURE) guards based on @conditional annotation
Summary: Builtins generator should emit ENABLE(FEATURE) guards based on @conditional a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-24 16:11 PDT by BJ Burg
Modified: 2015-10-28 13:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (41.55 KB, patch)
2015-10-24 16:16 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (48.51 KB, patch)
2015-10-26 21:41 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (63.02 KB, patch)
2015-10-27 23:03 PDT, BJ Burg
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-10-24 16:11:12 PDT
patch.
Comment 1 BJ Burg 2015-10-24 16:16:43 PDT
Created attachment 263985 [details]
Patch
Comment 2 WebKit Commit Bot 2015-10-24 16:18:50 PDT
This patch modifies the JS builtins code generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-builtins-generator-tests --reset-results`)
Comment 3 WebKit Commit Bot 2015-10-24 16:19:04 PDT
Attachment 263985 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_implementation.py:64:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 youenn fablet 2015-10-25 01:46:42 PDT
Just a quick note that I used @optional in streams built-in js, but maybe this is a poor name.
WebIDL is using "Conditional" keyword.
Generated code is using "ENABLE" macro.
Comment 5 BJ Burg 2015-10-25 10:23:43 PDT
(In reply to comment #4)
> Just a quick note that I used @optional in streams built-in js, but maybe
> this is a poor name.
> WebIDL is using "Conditional" keyword.
> Generated code is using "ENABLE" macro.

In other code generators, it is along the lines of 

// @conditional=ENABLE(STREAMS)

so that files can be conditionalized on PLATFORM(), USE(), etc.
It is easy to change this.
Comment 6 youenn fablet 2015-10-26 00:17:00 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Just a quick note that I used @optional in streams built-in js, but maybe
> > this is a poor name.
> > WebIDL is using "Conditional" keyword.
> > Generated code is using "ENABLE" macro.
> 
> In other code generators, it is along the lines of 
> 
> // @conditional=ENABLE(STREAMS)
> 
> so that files can be conditionalized on PLATFORM(), USE(), etc.
> It is easy to change this.

Sounds good to me
Comment 7 Yusuke Suzuki 2015-10-26 10:39:21 PDT
Comment on attachment 263985 [details]
Patch

r=me, annotation looks nice ;)
Comment 8 BJ Burg 2015-10-26 11:05:08 PDT
(In reply to comment #7)
> Comment on attachment 263985 [details]
> Patch
> 
> r=me, annotation looks nice ;)

Before landing, as discussed above, I'll change @optional to @conditional and include ENABLE() in all the existing guards. I'll put up another patch for EWS to catch any mistakes.
Comment 9 Yusuke Suzuki 2015-10-26 11:38:39 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 263985 [details]
> > Patch
> > 
> > r=me, annotation looks nice ;)
> 
> Before landing, as discussed above, I'll change @optional to @conditional
> and include ENABLE() in all the existing guards. I'll put up another patch
> for EWS to catch any mistakes.

@conditional looks preferable!
So your revised patch will include the existing stream JS code, right? (replacing @optional to @conditional)
Comment 10 BJ Burg 2015-10-26 11:40:20 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 263985 [details]
> > > Patch
> > > 
> > > r=me, annotation looks nice ;)
> > 
> > Before landing, as discussed above, I'll change @optional to @conditional
> > and include ENABLE() in all the existing guards. I'll put up another patch
> > for EWS to catch any mistakes.
> 
> @conditional looks preferable!
> So your revised patch will include the existing stream JS code, right?
> (replacing @optional to @conditional)

Yes.
Comment 11 BJ Burg 2015-10-26 21:41:48 PDT
Created attachment 264119 [details]
Proposed Fix
Comment 12 WebKit Commit Bot 2015-10-26 21:45:04 PDT
Attachment 264119 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_implementation.py:64:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 youenn fablet 2015-10-27 16:45:04 PDT
LGTM.
Just wondering whether we also want to test "@conditional=ENABLE(STREAMS_API) || USE(CF)" annotation?
Comment 14 BJ Burg 2015-10-27 22:41:31 PDT
(In reply to comment #13)
> LGTM.
> Just wondering whether we also want to test
> "@conditional=ENABLE(STREAMS_API) || USE(CF)" annotation?

Sure. The regex should accept any non '=' character as the annotation's value. But would be good to verify.
Comment 15 BJ Burg 2015-10-27 23:03:02 PDT
Created attachment 264197 [details]
Proposed Fix
Comment 16 WebKit Commit Bot 2015-10-27 23:05:04 PDT
Attachment 264197 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_implementation.py:64:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 BJ Burg 2015-10-28 09:45:07 PDT
Comment on attachment 264197 [details]
Proposed Fix

Setting cq+ with additional changes as discussed in bug.
Comment 18 WebKit Commit Bot 2015-10-28 09:48:24 PDT
Comment on attachment 264197 [details]
Proposed Fix

Rejecting attachment 264197 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 264197, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/349230
Comment 19 BJ Burg 2015-10-28 13:01:23 PDT
Committed r191687: <http://trac.webkit.org/changeset/191687>