Bug 150539 - Builtins generator should put WebCore-only wrappers in the per-builtin header
Summary: Builtins generator should put WebCore-only wrappers in the per-builtin header
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: 150496
  Show dependency treegraph
 
Reported: 2015-10-24 16:52 PDT by BJ Burg
Modified: 2015-10-31 09:53 PDT (History)
4 users (show)

See Also:


Attachments
Proposed Fix (71.37 KB, patch)
2015-10-24 16:56 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (75.77 KB, patch)
2015-10-24 17:00 PDT, BJ Burg
youennf: review+
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:52:46 PDT
No point in making this a separate header.
Comment 1 BJ Burg 2015-10-24 16:56:50 PDT
Created attachment 263989 [details]
Proposed Fix
Comment 2 WebKit Commit Bot 2015-10-24 16:58:51 PDT
Attachment 263989 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:71:  [BuiltinsSeparateHeaderGenerator.generate_output] Instance of 'BuiltinsSeparateHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 BJ Burg 2015-10-24 17:00:33 PDT
Created attachment 263991 [details]
Proposed Fix
Comment 4 WebKit Commit Bot 2015-10-24 17:01:57 PDT
Attachment 263991 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:71:  [BuiltinsSeparateHeaderGenerator.generate_output] Instance of 'BuiltinsSeparateHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 youenn fablet 2015-10-25 00:09:08 PDT
Comment on attachment 263991 [details]
Proposed Fix

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

Thanks for moving that forward. Obviously, I like it.

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:86
> +                ("WebCore", "%sBuiltins.h" % self.object.object_name),

We probably do not need that include.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:138
>      """class ${objectName}BuiltinsWrapper : private JSC::WeakHandleOwner {

${objectName}BuiltinsWrapper could be renamed ${objectName}Builtins later on.
Hopefully, only Source/WebCore/bindings/js/WebCoreJSBuiltins.h and WebCoreJSBuiltinInternals.h should need to be updated.

We might also want to clean a little bit  ${objectName}BuiltinFunctions in the future.
Comment 6 BJ Burg 2015-10-25 10:26:25 PDT
Comment on attachment 263991 [details]
Proposed Fix

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

>> Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:86
>> +                ("WebCore", "%sBuiltins.h" % self.object.object_name),
> 
> We probably do not need that include.

OK.

>> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:138
>>      """class ${objectName}BuiltinsWrapper : private JSC::WeakHandleOwner {
> 
> ${objectName}BuiltinsWrapper could be renamed ${objectName}Builtins later on.
> Hopefully, only Source/WebCore/bindings/js/WebCoreJSBuiltins.h and WebCoreJSBuiltinInternals.h should need to be updated.
> 
> We might also want to clean a little bit  ${objectName}BuiltinFunctions in the future.

Sure, this can be a followup.
Comment 7 BJ Burg 2015-10-30 10:45:04 PDT
Youenn, can you review this patch? I don't want it to get stale.
Comment 8 BJ Burg 2015-10-31 09:53:53 PDT
Committed r191842: <http://trac.webkit.org/changeset/191842>