Bug 150482 – For JSC builtins, generate-js-bindings.js should produce separate files per builtin

• Component: Tools / Tests
• Status: ASSIGNED

[Blaze Burg]

This will make the generator a lot smaller and reduce incremental build times.

I have a patch in progress.

[youenn fablet]

I added some additional tasks in bug 150496.
I will probably not have a lot of time to work on any of these in the next 2 weeks.

To be noted that the equivalent of JSCBuiltins.cpp and JSCBuiltins.h in WebCore is the set of individual XXBuiltins* files plus Source/WebCore/bindings/js/WebCoreJSBuiltins*

[Blaze Burg]

Created attachment 264013 [details]
WIP

[youenn fablet]

With a single JSCBuiltins.cpp/.h, it is not needed to edit xcode project file when adding a new JS built-in file. One just needs to edit CMakeList.txt and DerivedSources.make.
This is straightforward.
Can we keep the authoring as simple as it is now?

[Blaze Burg]

Created attachment 264030 [details]
Proposed Fix

[WebKit Commit Bot]

Attachment 264030 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:106:  blank line at end of file  [pep8/W391] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:44:  [BuiltinsFrameworkMacrosGenerator.output_filename] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:48:  [BuiltinsFrameworkMacrosGenerator.generate_output] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:50:  [BuiltinsFrameworkMacrosGenerator.generate_output] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:54:  [BuiltinsFrameworkMacrosGenerator.generate_output] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:68:  [BuiltinsFrameworkMacrosGenerator.generate_header_includes] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:74:  [BuiltinsFrameworkMacrosGenerator.generate_section_for_code_iteration_macro] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:81:  [BuiltinsFrameworkMacrosGenerator.generate_section_for_code_iteration_macro] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:92:  [BuiltinsFrameworkMacrosGenerator.generate_section_for_name_iteration_macro] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:97:  [BuiltinsFrameworkMacrosGenerator.generate_section_for_name_iteration_macro] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:69:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsFrameworkMacrosGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:73:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsObjectHeaderGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:74:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsObjectImplementationGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:77:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsObjectWrapperGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:49:  [BuiltinsObjectImplementationGenerator.macro_prefix] Instance of 'BuiltinsObjectImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:53:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:60:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:65:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'generate_embedded_code_string_section_for_function' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:66:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:68:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:99:  [BuiltinsObjectImplementationGenerator.generate_header_includes] Instance of 'BuiltinsObjectImplementationGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_wrapper.py:48:  [BuiltinsObjectWrapperGenerator.macro_prefix] Instance of 'BuiltinsObjectWrapperGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_wrapper.py:52:  [BuiltinsObjectWrapperGenerator.generate_output] Instance of 'BuiltinsObjectWrapperGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_wrapper.py:60:  [BuiltinsObjectWrapperGenerator.generate_output] Instance of 'BuiltinsObjectWrapperGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_wrapper.py:94:  [BuiltinsObjectWrapperGenerator.generate_header_includes] Instance of 'BuiltinsObjectWrapperGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:48:  [BuiltinsObjectHeaderGenerator.macro_prefix] Instance of 'BuiltinsObjectHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:52:  [BuiltinsObjectHeaderGenerator.generate_output] Instance of 'BuiltinsObjectHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:59:  [BuiltinsObjectHeaderGenerator.generate_output] Instance of 'BuiltinsObjectHeaderGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:88:  [BuiltinsObjectHeaderGenerator.generate_header_includes] Instance of 'BuiltinsObjectHeaderGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:146:  [BuiltinsObjectHeaderGenerator.generate_section_for_code_table_macro] Instance of 'BuiltinsObjectHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
Total errors found: 30 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

(In reply to comment #3)
> With a single JSCBuiltins.cpp/.h, it is not needed to edit xcode project
> file when adding a new JS built-in file. One just needs to edit
> CMakeList.txt and DerivedSources.make.
> This is straightforward.
> Can we keep the authoring as simple as it is now?

I don't think that's possible, since it generates .cpp files. If it were just headers, then it would be fine, because those are found via VPATH or similar directory-search mechanism. But this isn't too big of a problem, as it's the same way for WebCore builtins.

[Blaze Burg]

Created attachment 264414 [details]
Rebased Patch

[Blaze Burg]

Created attachment 264425 [details]
Proposed Fix (CMake fix)

[WebKit Commit Bot]

Attachment 264425 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:106:  blank line at end of file  [pep8/W391] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:44:  [BuiltinsFrameworkMacrosGenerator.output_filename] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:48:  [BuiltinsFrameworkMacrosGenerator.generate_output] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:50:  [BuiltinsFrameworkMacrosGenerator.generate_output] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:54:  [BuiltinsFrameworkMacrosGenerator.generate_output] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:68:  [BuiltinsFrameworkMacrosGenerator.generate_header_includes] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:74:  [BuiltinsFrameworkMacrosGenerator.generate_section_for_code_iteration_macro] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:81:  [BuiltinsFrameworkMacrosGenerator.generate_section_for_code_iteration_macro] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:92:  [BuiltinsFrameworkMacrosGenerator.generate_section_for_name_iteration_macro] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:97:  [BuiltinsFrameworkMacrosGenerator.generate_section_for_name_iteration_macro] Instance of 'BuiltinsFrameworkMacrosGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:69:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsFrameworkMacrosGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:73:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsObjectHeaderGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:74:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsObjectImplementationGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:77:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsObjectWrapperGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/CMakeLists.txt:1268:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:49:  [BuiltinsObjectImplementationGenerator.macro_prefix] Instance of 'BuiltinsObjectImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:53:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:62:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:64:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:70:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'generate_embedded_code_string_section_for_function' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:71:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:73:  [BuiltinsObjectImplementationGenerator.generate_output] Instance of 'BuiltinsObjectImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_implementation.py:106:  [BuiltinsObjectImplementationGenerator.generate_secondary_header_includes] Instance of 'BuiltinsObjectImplementationGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_wrapper.py:48:  [BuiltinsObjectWrapperGenerator.macro_prefix] Instance of 'BuiltinsObjectWrapperGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_wrapper.py:52:  [BuiltinsObjectWrapperGenerator.generate_output] Instance of 'BuiltinsObjectWrapperGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_wrapper.py:62:  [BuiltinsObjectWrapperGenerator.generate_output] Instance of 'BuiltinsObjectWrapperGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_wrapper.py:100:  [BuiltinsObjectWrapperGenerator.generate_secondary_header_includes] Instance of 'BuiltinsObjectWrapperGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:48:  [BuiltinsObjectHeaderGenerator.macro_prefix] Instance of 'BuiltinsObjectHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:52:  [BuiltinsObjectHeaderGenerator.generate_output] Instance of 'BuiltinsObjectHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:61:  [BuiltinsObjectHeaderGenerator.generate_output] Instance of 'BuiltinsObjectHeaderGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:94:  [BuiltinsObjectHeaderGenerator.generate_secondary_header_includes] Instance of 'BuiltinsObjectHeaderGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:152:  [BuiltinsObjectHeaderGenerator.generate_section_for_code_table_macro] Instance of 'BuiltinsObjectHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
Total errors found: 32 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Youenn, this is ready for review. It might need another rebase after 150539 lands.

[youenn fablet]

Comment on attachment 264425 [details]
Proposed Fix (CMake fix)

Patch looks good to me, please find a preliminary review for the moment.
To ease the review, I would have made the script filename renaming in a separate patch.

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

> Source/JavaScriptCore/CMakeLists.txt:1254
> +    OUTPUT ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltinsMacros.cpp

JSCBuiltinsMacros.cpp should be removed.

> Source/JavaScriptCore/CMakeLists.txt:1268
> +    string(REGEX REPLACE "\\.Promise" "Promise" _objectName "${_objectName}")    

It seems "Constructor", ".Promise" and ".prototype" all serve the same purpose in different ways.
Should we make the filename consistent and simplify those rules?
I would go with removing "." followed by uppercase letter, as a preliminary patch maybe?

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6633
> +				0FEC84FF1BDACDAC0080FF74 /* B3ArgumentRegValue.h in Headers */,

These changes seem unrelated with this patch.
I guess this is due to editing the xcodeproj through XCode?

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:97
> +        unique_names = list(set([function.function_name for function in self.model().all_functions()]))

I guess @conditional is not supported here.
This might not be worth fixing it but we may still want to add a #FIXME.

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:84
> +enum class ConstructAbility : unsigned;

Are these two lines the only changes from the original separate_header.py?

> Source/JavaScriptCore/Scripts/tests/builtins/expected/JavaScriptCore-Builtin.prototype-Macros.js-result:44
> +    macro(forEach) \

This test is only based on one js file.
We probably do not test two js files with same names.
Can we add such test?

[Darin Adler]

Comment on attachment 264425 [details]
Proposed Fix (CMake fix)

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

> Source/JavaScriptCore/ChangeLog:3
> +        For JSC builtins, generate-js-bindings.js should produce separate files per builtin

Why? Does it make things build faster?

[youenn fablet]

(In reply to comment #12)
> Comment on attachment 264425 [details]
> Proposed Fix (CMake fix)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264425&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        For JSC builtins, generate-js-bindings.js should produce separate files per builtin
> 
> Why? Does it make things build faster?

It should (reduced rebuilding, parallel icecream) but the main reason to me is consistency.

It is more consistent with other generators, like IDL generator.

It allows consistency between WebCore and JSC. This reduces the code generator size and will simplify further developments that are beneficial for both JSC and WebCore.

For instance, handling of @conditional (compiling behind a flag) and @internals (functions that are not public JS API) annotation seems more natural with separate files.

[Blaze Burg]

Comment on attachment 264425 [details]
Proposed Fix (CMake fix)

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

I will post a new patch soon that doesn't rename + modify and addresses comments.

>>> Source/JavaScriptCore/ChangeLog:3
>>> +        For JSC builtins, generate-js-bindings.js should produce separate files per builtin
>> 
>> Why? Does it make things build faster?
> 
> It should (reduced rebuilding, parallel icecream) but the main reason to me is consistency.
> 
> It is more consistent with other generators, like IDL generator.
> 
> It allows consistency between WebCore and JSC. This reduces the code generator size and will simplify further developments that are beneficial for both JSC and WebCore.
> 
> For instance, handling of @conditional (compiling behind a flag) and @internals (functions that are not public JS API) annotation seems more natural with separate files.

I agree with Yoenn's reasons (not sure what icecream is though).

>> Source/JavaScriptCore/CMakeLists.txt:1254
>> +    OUTPUT ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltinsMacros.cpp
> 
> JSCBuiltinsMacros.cpp should be removed.

Oops, yeah.

>> Source/JavaScriptCore/CMakeLists.txt:1268
>> +    string(REGEX REPLACE "\\.Promise" "Promise" _objectName "${_objectName}")    
> 
> It seems "Constructor", ".Promise" and ".prototype" all serve the same purpose in different ways.
> Should we make the filename consistent and simplify those rules?
> I would go with removing "." followed by uppercase letter, as a preliminary patch maybe?

Maybe Yusuke can say more, but I am actually in favor of the current naming scheme. It matches the filename with the global binding for the builtin.

>> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6633
>> +				0FEC84FF1BDACDAC0080FF74 /* B3ArgumentRegValue.h in Headers */,
> 
> These changes seem unrelated with this patch.
> I guess this is due to editing the xcodeproj through XCode?

Yeah, I should just sort this separately. Looks like it hasn't been done since the B3 changes.

>> Source/JavaScriptCore/Scripts/builtins/builtins_generate_framework_macros.py:97
>> +        unique_names = list(set([function.function_name for function in self.model().all_functions()]))
> 
> I guess @conditional is not supported here.
> This might not be worth fixing it but we may still want to add a #FIXME.

I suppose this could consult the containing object and emit a guard if necessary for all uses. I am not sure how #ifdef will interact with X-macros.

>> Source/JavaScriptCore/Scripts/builtins/builtins_generate_object_header.py:84
>> +enum class ConstructAbility : unsigned;
> 
> Are these two lines the only changes from the original separate_header.py?

Yeah. I will split this so the renaming happens separately.

>> Source/JavaScriptCore/Scripts/tests/builtins/expected/JavaScriptCore-Builtin.prototype-Macros.js-result:44
>> +    macro(forEach) \
> 
> This test is only based on one js file.
> We probably do not test two js files with same names.
> Can we add such test?

Currently it's 1:1 input test file and multiple combined file outputs. We could have multiple builtins per file using a special test-only syntax like ### Begin File: XXX.js. But I think this is FIXME territory for now.

[youenn fablet]

> > It should (reduced rebuilding, parallel icecream) but the main reason to me is consistency.

http://linux.die.net/man/7/icecream

> > It seems "Constructor", ".Promise" and ".prototype" all serve the same purpose in different ways.
> > Should we make the filename consistent and simplify those rules?
> > I would go with removing "." followed by uppercase letter, as a preliminary patch maybe?
> 
> Maybe Yusuke can say more, but I am actually in favor of the current naming
> scheme. It matches the filename with the global binding for the builtin.

We currently have Promise.prototype.js, PromiseConstructor.js and Operations.Promise.js. Operations.Promise.js seems in particular not aligned.

I would tend to use PromisePrototype.js, PromiseConstructor.js and PromiseOperations.js (or PromiseInternals.js, as is done in WebCore).
This would directly match generated PromiseXX.h/.cpp

> > I guess @conditional is not supported here.
> > This might not be worth fixing it but we may still want to add a #FIXME.
> 
> I suppose this could consult the containing object and emit a guard if
> necessary for all uses. I am not sure how #ifdef will interact with X-macros.

Yes, documenting this issue is probably enough.

> > This test is only based on one js file.
> > We probably do not test two js files with same names.
> > Can we add such test?
> 
> Currently it's 1:1 input test file and multiple combined file outputs. We
> could have multiple builtins per file using a special test-only syntax like
> ### Begin File: XXX.js. But I think this is FIXME territory for now.

OK

[youenn fablet]

Also, there are some style warnings, should we update python source code or the style checker?

[Blaze Burg]

(In reply to comment #16)
> Also, there are some style warnings, should we update python source code or
> the style checker?

Most of the Python ones are unfixable, because the standard Python style checker that check-webkit-style defers to doesn't understand class inheritance or map literals.

[Blaze Burg]

(In reply to comment #15)
> > > It should (reduced rebuilding, parallel icecream) but the main reason to me is consistency.
> 
> http://linux.die.net/man/7/icecream
> 
> > > It seems "Constructor", ".Promise" and ".prototype" all serve the same purpose in different ways.
> > > Should we make the filename consistent and simplify those rules?
> > > I would go with removing "." followed by uppercase letter, as a preliminary patch maybe?
> > 
> > Maybe Yusuke can say more, but I am actually in favor of the current naming
> > scheme. It matches the filename with the global binding for the builtin.
> 
> We currently have Promise.prototype.js, PromiseConstructor.js and
> Operations.Promise.js. Operations.Promise.js seems in particular not aligned.
> 
> I would tend to use PromisePrototype.js, PromiseConstructor.js and
> PromiseOperations.js (or PromiseInternals.js, as is done in WebCore).
> This would directly match generated PromiseXX.h/.cpp

I filed this bug for the renaming, to avoid making this patch even more confusing.

https://bugs.webkit.org/show_bug.cgi?id=151549