Bug 159921

Created attachment 284008 [details] Patch

Attachment 284008 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:86: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:87: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateImplementationGenerator' [pylint/E0602] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 284014 [details] Fixing CMake build

Attachment 284014 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:86: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:87: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateImplementationGenerator' [pylint/E0602] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 284014 [details] Fixing CMake build View in context: https://bugs.webkit.org/attachment.cgi?id=284014&action=review > Source/JavaScriptCore/ChangeLog:8 > + Updated built-in generator to generate only wrapper files when passed the--wrapper option. Typo: 'the --wrapper' > Source/JavaScriptCore/ChangeLog:10 > + WebCore separate test files. Not quite sure I follow this sentence. > Source/WebCore/ChangeLog:11 > + builtin generator is now called for each individula built-in file plus once for WebCore wrapper files. Typos, grammar > Source/JavaScriptCore/Scripts/generate-js-builtins.py:45 > + return framework_name + '-Wrappers-result' if generate_wrapper_files else os.path.basename(builtins_files[0]) + '-result' Please use block if-else, it's easier to read here where we have nontrivial branches. > Source/JavaScriptCore/Scripts/tests/builtins/expected/WebCore-Wrappers-result:1 > +### Begin File: WebCoreJSBuiltins.h I think the generated result file name should be WebCoreJSBuiltins.h-result, so its closer to the naming for other files. At the least it needs a valid extension before -result. > Source/WebCore/CMakeLists.txt:3761 > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.cpp Just so everyone is on the same page, you should explain in the changelog why it is desirable/necessary to create a combined builtins file. IIRC, we were trying to move *away* from this approach for JSC builtins but never got all the way there. > Source/WebCore/DerivedSources.make:-1322 > -# WebCore_BUILTINS_SOURCE_LIST = $(shell echo $(WebCore_BUILTINS_SOURCES) | perl -e 'print " " . join(" -I", split(" ", <>));') Sidenote: this use of Perl is unnecessary, the same can be achieved with $(patsubst %,-I%,$(WebCore_BUILTINS_SOURCES)) > Tools/Scripts/webkitpy/codegen/main.py:59 > + self.write_error_file(framework_name + "-Wrappers" if generate_wrappers else builtins_files[0], output_directory, stderr_output) Should have -error suffix like other results. > Tools/Scripts/webkitpy/codegen/main.py:112 > + work_directory = tempfile.mkdtemp() I recommend giving a mkdtemp directory a prefix or suffix, so it can be found by searching (i.e., using `find`) if necessary.

Created attachment 284036 [details] Patch

Attachment 284036 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:88: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:89: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateImplementationGenerator' [pylint/E0602] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 284103 [details] Rebasing

Attachment 284103 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:88: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:89: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateImplementationGenerator' [pylint/E0602] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 284103 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=284103&action=review Great work. There are just a few things left to address: 1) EWS failures 2) WebCore/DerivedSources.make has some issues 3) Given that wrapper generation is now exclusive with other generation, I think the option should be named --wrappers-only. Sorry for being picky about the name, I think I did not understand the patch as well when I made the current suggestion. > Source/JavaScriptCore/ChangeLog:13 > + Added new built-in test file to cover the case of several concatenating several guards in generated WebCore wrapper files. Nit: extra 'several' > Source/WebCore/ChangeLog:12 > + WebCore wrapper files allow handling cthings like conditionally guarded features. Nit: 'things' > Source/JavaScriptCore/Scripts/generate-js-builtins.py:88 > + generators.append(BuiltinsSeparateHeaderGenerator(model, object)) Ok, so now I'm a little confused. Is --wrappers exclusive with generating normal builtins? It's not clear from the code and change log whether it's additive or subtractive. If it's subtractive, --wrappers-only might make more sense. > Source/WebCore/CMakeLists.txt:3762 > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.cpp This would be easier to follow if the output files for wrappers actually had Wrappers in the name somehow. > Source/WebCore/CMakeLists.txt:3768 > COMMAND ${PYTHON_EXECUTABLE} ${JavaScriptCore_SCRIPTS_DIR}/generate-js-builtins.py --wrappers --framework WebCore --output-directory ${DERIVED_SOURCES_WEBCORE_DIR} ${WebCore_BUILTINS_SOURCES} I see, so it would make sense to call it --wrappers-only, as you need to call it a second time. > Source/WebCore/DerivedSources.make:1328 > +WebCore_BUILTINS_WRAPPER : WebCore_BUILTINS_DEPENDENCIES_LIST Is WebCore_BUILTINS_WRAPPER a valid target? I don't see where it's defined. Shouldn't it be WebCoreBuiltinsWrappers.h or whatever? > Source/WebCore/DerivedSources.make:1334 > +all : $(notdir $(WebCore_BUILTINS_SOURCES:%.js=%Builtins.h)) WebCore_BUILTINS_WRAPPER Same comment as above. This will either not work at all, or expect a file called WebCore_BUILTINS_WRAPPER. > Tools/Scripts/webkitpy/codegen/main.py:107 > + def run_test(self, reference_directory, test_name, test_files, generate_builtin): Nit: I would add _callback suffix to generate_builtin. > Tools/Scripts/webkitpy/codegen/main.py:108 > + result = True Nit: I would rename result to something like 'success' since it's a return code not an object.

Thanks for the review. > 1) EWS failures Win did not succeed in applying the patch. I saw this error in another patch so I do not think this is related to this one. Same for gtk bot which has some issues these days. Note that elf-wk2 is also exercising CMake build. > 2) WebCore/DerivedSources.make has some issues OK > 3) Given that wrapper generation is now exclusive with other generation, I > think the option should be named --wrappers-only. Sorry for being picky > about the name, I think I did not understand the patch as well when I made > the current suggestion. OK > > Source/JavaScriptCore/ChangeLog:13 > > + Added new built-in test file to cover the case of several concatenating several guards in generated WebCore wrapper files. > > Nit: extra 'several' OK > > Source/WebCore/ChangeLog:12 > > + WebCore wrapper files allow handling cthings like conditionally guarded features. > > Nit: 'things' OK > > Source/JavaScriptCore/Scripts/generate-js-builtins.py:88 > > + generators.append(BuiltinsSeparateHeaderGenerator(model, object)) > > Ok, so now I'm a little confused. Is --wrappers exclusive with generating > normal builtins? It's not clear from the code and change log whether it's > additive or subtractive. If it's subtractive, --wrappers-only might make > more sense. It is exclusive, I updated the changelog. > > Source/WebCore/CMakeLists.txt:3762 > > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebCoreJSBuiltins.cpp > > This would be easier to follow if the output files for wrappers actually had > Wrappers in the name somehow. I am fine changing the name. I chose this name to be close to JSC names (JSCBuiltins.h/.cpp). I'd like to handle this in another patch if possible. > > Source/WebCore/CMakeLists.txt:3768 > > COMMAND ${PYTHON_EXECUTABLE} ${JavaScriptCore_SCRIPTS_DIR}/generate-js-builtins.py --wrappers --framework WebCore --output-directory ${DERIVED_SOURCES_WEBCORE_DIR} ${WebCore_BUILTINS_SOURCES} > > I see, so it would make sense to call it --wrappers-only, as you need to > call it a second time. OK > > Source/WebCore/DerivedSources.make:1328 > > +WebCore_BUILTINS_WRAPPER : WebCore_BUILTINS_DEPENDENCIES_LIST > > Is WebCore_BUILTINS_WRAPPER a valid target? I don't see where it's defined. > Shouldn't it be WebCoreBuiltinsWrappers.h or whatever? It is working but you are right, it is better to define it precisely. This will remove any potential future glitch. > > Tools/Scripts/webkitpy/codegen/main.py:107 > > + def run_test(self, reference_directory, test_name, test_files, generate_builtin): > > Nit: I would add _callback suffix to generate_builtin. OK > > Tools/Scripts/webkitpy/codegen/main.py:108 > > + result = True > > Nit: I would rename result to something like 'success' since it's a return > code not an object. I changed it to "passed" to be consistent with run_tests.

Created attachment 284250 [details] Fixing according comments

Attachment 284250 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:87: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:88: [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateImplementationGenerator' [pylint/E0602] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 284250 [details] Fixing according comments View in context: https://bugs.webkit.org/attachment.cgi?id=284250&action=review r=me pending EWS and minor DerivedSources.make fixes. > Source/JavaScriptCore/ChangeLog:9 > + When this option is used, wrqpper files are generated but no individual file is generated. Nit: 'wrapper' > Source/WebCore/DerivedSources.make:1335 > +WebCore_BUILTINS_WRAPPERS : WebCore_BUILTINS_DEPENDENCIES_LIST This will probably not do what you want. It will call the generator once per file in WebCore_BUILTINS_WRAPPERS. The fix we have been using is to do $(firstword $(WebCore_BUILTINS_WRAPPERS)) when listing it as a target to be made. It will only be made once but all the files will be produced nonetheless. (Don't you wish we could just stick to CMake? ;)) > Source/WebCore/DerivedSources.make:1341 > +all : $(notdir $(WebCore_BUILTINS_SOURCES:%.js=%Builtins.h)) WebCore_BUILTINS_WRAPPERS Unlike the dependencies list, this is not a file. Use $(WebCore_BUILTINS_WRAPPERS).

Thanks for the r+ > > Source/JavaScriptCore/ChangeLog:9 > > + When this option is used, wrqpper files are generated but no individual file is generated. > > Nit: 'wrapper' OK > > Source/WebCore/DerivedSources.make:1335 > > +WebCore_BUILTINS_WRAPPERS : WebCore_BUILTINS_DEPENDENCIES_LIST > > This will probably not do what you want. It will call the generator once per > file in WebCore_BUILTINS_WRAPPERS. The fix we have been using is to do > $(firstword $(WebCore_BUILTINS_WRAPPERS)) when listing it as a target to be > made. It will only be made once but all the files will be produced > nonetheless. I'll change that but cannot it cause an issue if the first file is ok but one of the other file needs to be regenerated (like if it is accidentally deleted)? > (Don't you wish we could just stick to CMake? ;)) Definitely... 8-)

Created attachment 284315 [details] Patch for landing

Comment on attachment 284315 [details] Patch for landing Clearing flags on attachment: 284315 Committed r203554: <http://trac.webkit.org/changeset/203554>

All reviewed patches have been landed. Closing bug.

(In reply to comment #15) > Thanks for the r+ > > > > Source/JavaScriptCore/ChangeLog:9 > > > + When this option is used, wrqpper files are generated but no individual file is generated. > > > > Nit: 'wrapper' > > OK > > > > Source/WebCore/DerivedSources.make:1335 > > > +WebCore_BUILTINS_WRAPPERS : WebCore_BUILTINS_DEPENDENCIES_LIST > > > > This will probably not do what you want. It will call the generator once per > > file in WebCore_BUILTINS_WRAPPERS. The fix we have been using is to do > > $(firstword $(WebCore_BUILTINS_WRAPPERS)) when listing it as a target to be > > made. It will only be made once but all the files will be produced > > nonetheless. > > I'll change that but cannot it cause an issue if the first file is ok but > one of the other file needs to be regenerated (like if it is accidentally > deleted)? Unfortunately we are stuck with this problem as long as we use Make. AFAIK there is no good workaround for multiple files derived from one invocation. If you delete the .mm or .cpp from JS DOM bindings, it will also not get regenerated. > > > (Don't you wish we could just stick to CMake? ;)) > > Definitely... 8-)