Created attachment 114440 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input.

Comment on attachment 114440 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. View in context: https://bugs.webkit.org/attachment.cgi?id=114440&action=review > Source/WebCore/UseV8.cmake:229 > +SET(FEATURE_DEFINES_JAVASCRIPT "LANGUAGE_JAVASCRIPT=1") > +SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} V8_BINDING=1") Why not have these in a single call?

CC'ing Patrick in case he has anything to say.

Comment on attachment 114440 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. View in context: https://bugs.webkit.org/attachment.cgi?id=114440&action=review > Source/WebCore/UseV8.cmake:19 > + html/canvas/CanvasPixelArray.idl > + > + storage/IDBVersionChangeEvent.idl > + storage/IDBVersionChangeRequest.idl Are this IDL files part of V8? I don't think so. If you need them you should put them into the CMakeLists.txt > Source/WebCore/UseV8.cmake:23 > + bindings/generic/BindingSecurityBase.cpp If it's generic it should go into the CMakeLists.txt > Source/WebCore/UseV8.cmake:25 > + bindings/ScriptControllerBase.cpp already included in CMakeLists.txt > Source/WebCore/UseV8.cmake:240 > +ADD_CUSTOM_COMMAND( > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/DebuggerScriptSource.h > + MAIN_DEPENDENCY bindings/v8/DebuggerScript.js > + COMMAND ${PERL_EXECUTABLE} ${WEBCORE_DIR}/inspector/xxd.pl DebuggerScriptSource_js ${WEBCORE_DIR}/bindings/v8/DebuggerScript.js ${DERIVED_SOURCES_WEBCORE_DIR}/DebuggerScriptSource.h > + VERBATIM) Please add all other dependencies too, not only the MAIN_DEPENDENCY. > Source/WebCore/UseV8.cmake:256 > +ADD_CUSTOM_COMMAND( > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/V8ArrayBufferViewCustomScript.h > + MAIN_DEPENDENCY bindings/v8/custom/V8ArrayBufferViewCustomScript.js > + COMMAND ${PERL_EXECUTABLE} ${WEBCORE_DIR}/inspector/xxd.pl V8ArrayBufferViewCustomScript_js ${WEBCORE_DIR}/bindings/v8/custom/V8ArrayBufferViewCustomScript.js ${DERIVED_SOURCES_WEBCORE_DIR}/V8ArrayBufferViewCustomScript.h > + VERBATIM) Missing dependecies?

And CC'ing dbates too, as he's been doing CMake stuff lately.

(In reply to comment #2) > (From update of attachment 114440 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114440&action=review > > > Source/WebCore/UseV8.cmake:229 > > +SET(FEATURE_DEFINES_JAVASCRIPT "LANGUAGE_JAVASCRIPT=1") > > +SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} V8_BINDING=1") > > Why not have these in a single call? I will replace these with belows +SET(FEATURE_DEFINES_JAVASCRIPT "LANGUAGE_JAVASCRIPT=1") +FOREACH (_feature ${FEATURE_DEFINES}) + SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} ${_feature}") + SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} V8_BINDING=1") +ENDFOREACH ()

(In reply to comment #6) > (In reply to comment #2) > > (From update of attachment 114440 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=114440&action=review > > > > > Source/WebCore/UseV8.cmake:229 > > > +SET(FEATURE_DEFINES_JAVASCRIPT "LANGUAGE_JAVASCRIPT=1") > > > +SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} V8_BINDING=1") > > > > Why not have these in a single call? > > I will replace these with belows > > +SET(FEATURE_DEFINES_JAVASCRIPT "LANGUAGE_JAVASCRIPT=1") > +FOREACH (_feature ${FEATURE_DEFINES}) > + SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} ${_feature}") > + SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} V8_BINDING=1") > +ENDFOREACH () Why do you add V8_BINDING=1 not only once?

(In reply to comment #4) > (From update of attachment 114440 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114440&action=review > > > Source/WebCore/UseV8.cmake:19 > > + html/canvas/CanvasPixelArray.idl > > + > > + storage/IDBVersionChangeEvent.idl > > + storage/IDBVersionChangeRequest.idl > > Are this IDL files part of V8? I don't think so. If you need them you should put them into the CMakeLists.txt > > > Source/WebCore/UseV8.cmake:23 > > + bindings/generic/BindingSecurityBase.cpp > > If it's generic it should go into the CMakeLists.txt > > > Source/WebCore/UseV8.cmake:25 > > + bindings/ScriptControllerBase.cpp > > already included in CMakeLists.txt > > > Source/WebCore/UseV8.cmake:240 > > +ADD_CUSTOM_COMMAND( > > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/DebuggerScriptSource.h > > + MAIN_DEPENDENCY bindings/v8/DebuggerScript.js > > + COMMAND ${PERL_EXECUTABLE} ${WEBCORE_DIR}/inspector/xxd.pl DebuggerScriptSource_js ${WEBCORE_DIR}/bindings/v8/DebuggerScript.js ${DERIVED_SOURCES_WEBCORE_DIR}/DebuggerScriptSource.h > > + VERBATIM) > > Please add all other dependencies too, not only the MAIN_DEPENDENCY. > > > Source/WebCore/UseV8.cmake:256 > > +ADD_CUSTOM_COMMAND( > > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/V8ArrayBufferViewCustomScript.h > > + MAIN_DEPENDENCY bindings/v8/custom/V8ArrayBufferViewCustomScript.js > > + COMMAND ${PERL_EXECUTABLE} ${WEBCORE_DIR}/inspector/xxd.pl V8ArrayBufferViewCustomScript_js ${WEBCORE_DIR}/bindings/v8/custom/V8ArrayBufferViewCustomScript.js ${DERIVED_SOURCES_WEBCORE_DIR}/V8ArrayBufferViewCustomScript.h > > + VERBATIM) > > Missing dependecies? Thanks for your feedback. I have check it, whether there are missing dependencies in bindings/v8 but there are no other dependencies related with generating DebuggerScriptSource.h and V8ArrayBufferViewCustomScript.h. so they have just one dependency. Belows are not part of V8. I will put them into CMakeLists.txt. Source/WebCore/UseV8.cmake:19 + html/canvas/CanvasPixelArray.idl Source/WebCore/UseV8.cmake:23 + bindings/generic/BindingSecurityBase.cpp ScriptControllerBase.cpp is already included in CMakeLists.txt and IDBVersion*.idl files are not necessary. so I will remove belows from UseV8.cmake Source/WebCore/UseV8.cmake:23 + storage/IDBVersionChangeEvent.idl + storage/IDBVersionChangeRequest.idl Source/WebCore/UseV8.cmake:25 + bindings/ScriptControllerBase.cpp

Created attachment 114821 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input.

Attachment 114821 [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/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 114821 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. View in context: https://bugs.webkit.org/attachment.cgi?id=114821&action=review > Source/WebCore/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=XXXXX Pleas add a VALID bug number! ;-) > Source/WebCore/UseV8.cmake:220 > + SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} V8_BINDING=1") why do you add the in the loop? if you really need to add it for every feature you can add it the the set statement in the line before. > Source/WebCore/UseV8.cmake:228 > +ADD_CUSTOM_COMMAND( > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/DebuggerScriptSource.h > + MAIN_DEPENDENCY bindings/v8/DebuggerScript.js > + COMMAND ${PERL_EXECUTABLE} ${WEBCORE_DIR}/inspector/xxd.pl DebuggerScriptSource_js ${WEBCORE_DIR}/bindings/v8/DebuggerScript.js ${DERIVED_SOURCES_WEBCORE_DIR}/DebuggerScriptSource.h > + VERBATIM) you need to add DEPENDS ${WEBCORE_DIR}/inspector/xxd.pl (and all file it maybe includes) if you want to regenerate the output when xxd.pl changes > Source/WebCore/UseV8.cmake:244 > +ADD_CUSTOM_COMMAND( > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/V8ArrayBufferViewCustomScript.h > + MAIN_DEPENDENCY bindings/v8/custom/V8ArrayBufferViewCustomScript.js > + COMMAND ${PERL_EXECUTABLE} ${WEBCORE_DIR}/inspector/xxd.pl V8ArrayBufferViewCustomScript_js ${WEBCORE_DIR}/bindings/v8/custom/V8ArrayBufferViewCustomScript.js ${DERIVED_SOURCES_WEBCORE_DIR}/V8ArrayBufferViewCustomScript.h > + VERBATIM) missing dependency to xxd.pl

Created attachment 114824 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input.

Comment on attachment 114824 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. View in context: https://bugs.webkit.org/attachment.cgi?id=114824&action=review > Source/WebCore/UseV8.cmake:218 > +SET(FEATURE_DEFINES_JAVASCRIPT "LANGUAGE_JAVASCRIPT=1") > +SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} V8_BINDING=1") Why not SET(FEATURE_DEFINES_JAVASCRIPT "LANGUAGE_JAVASCRIPT=1 V8_BINDING=1") ? > Source/WebCore/UseV8.cmake:227 > + DEPENDS ${WEBCORE_DIR}/inspector/xxd.pl ${WEBCORE_DIR}/bindings/v8/DebuggerScript.js You don't need to add DebuggerScript.js, since it's already the MAIN_DEPENDENCY > Source/WebCore/UseV8.cmake:244 > + DEPENDS ${WEBCORE_DIR}/inspector/xxd.pl ${WEBCORE_DIR}/bindings/v8/custom/V8ArrayBufferViewCustomScript.js no need for V8ArrayBufferViewCustomScript.js

(In reply to comment #11) > (From update of attachment 114821 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114821&action=review > > > Source/WebCore/ChangeLog:6 > > + https://bugs.webkit.org/show_bug.cgi?id=XXXXX > > Pleas add a VALID bug number! ;-) > > > Source/WebCore/UseV8.cmake:220 > > + SET(FEATURE_DEFINES_JAVASCRIPT "${FEATURE_DEFINES_JAVASCRIPT} V8_BINDING=1") > > why do you add the in the loop? if you really need to add it for every feature you can add it the the set statement in the line before. > > > Source/WebCore/UseV8.cmake:228 > > +ADD_CUSTOM_COMMAND( > > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/DebuggerScriptSource.h > > + MAIN_DEPENDENCY bindings/v8/DebuggerScript.js > > + COMMAND ${PERL_EXECUTABLE} ${WEBCORE_DIR}/inspector/xxd.pl DebuggerScriptSource_js ${WEBCORE_DIR}/bindings/v8/DebuggerScript.js ${DERIVED_SOURCES_WEBCORE_DIR}/DebuggerScriptSource.h > > + VERBATIM) > > you need to add DEPENDS ${WEBCORE_DIR}/inspector/xxd.pl (and all file it maybe includes) if you want to regenerate the output when xxd.pl changes > > > Source/WebCore/UseV8.cmake:244 > > +ADD_CUSTOM_COMMAND( > > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/V8ArrayBufferViewCustomScript.h > > + MAIN_DEPENDENCY bindings/v8/custom/V8ArrayBufferViewCustomScript.js > > + COMMAND ${PERL_EXECUTABLE} ${WEBCORE_DIR}/inspector/xxd.pl V8ArrayBufferViewCustomScript_js ${WEBCORE_DIR}/bindings/v8/custom/V8ArrayBufferViewCustomScript.js ${DERIVED_SOURCES_WEBCORE_DIR}/V8ArrayBufferViewCustomScript.h > > + VERBATIM) > > missing dependency to xxd.pl sorry.. I make a mistake for adding a wrong patch :( I add "DEPENDS ${WEBCORE_DIR}/inspector/xxd.pl" and modify other misc.

Created attachment 114827 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. remove DebuggerScript.js, V8ArrayBufferViewCustomScript.js from DEPENDS. and change settings of FEATURE_DEFINES_JAVASCRIPT

Comment on attachment 114827 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. Since I don't know all of the V8 stuff, I can't tell if there are any files missing or wrong. So if this patch works: LGTM

Comment on attachment 114827 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. View in context: https://bugs.webkit.org/attachment.cgi?id=114827&action=review This looks sane to me. I have some minor nits. > Source/WebCore/UseV8.cmake:15 > +LIST(APPEND WebCore_SOURCES Please sort the entires in this list so that they appear in sorted order as produced by the Unix sort command. > Source/WebCore/UseV8.cmake:167 > +LIST(APPEND WebCore_SOURCES > + ${JAVASCRIPTCORE_DIR}/yarr/YarrPattern.cpp > + ${JAVASCRIPTCORE_DIR}/yarr/YarrInterpreter.cpp > + ${JAVASCRIPTCORE_DIR}/yarr/YarrJIT.cpp > + ${JAVASCRIPTCORE_DIR}/yarr/YarrSyntaxChecker.cpp Ditto.

Created attachment 114876 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input.

Created attachment 114880 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. I modified the entries of Yarr* and others as a sorted order Source/WebCore/UseV8.cmake:167 +LIST(APPEND WebCore_SOURCES + ${JAVASCRIPTCORE_DIR}/yarr/YarrPattern.cpp + ${JAVASCRIPTCORE_DIR}/yarr/YarrInterpreter.cpp + ${JAVASCRIPTCORE_DIR}/yarr/YarrJIT.cpp + ${JAVASCRIPTCORE_DIR}/yarr/YarrSyntaxChecker.cpp

Comment on attachment 114880 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. View in context: https://bugs.webkit.org/attachment.cgi?id=114880&action=review > Source/WebCore/UseV8.cmake:15 > +LIST(APPEND WebCore_SOURCES The first two sections of this list aren't in sorted order as produced by the Unix sort command. The first two sections should look like <http://pastie.org/2860330>.

Created attachment 114907 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. The entries of v8 bindings are listed in order as you mentioned (the Unix sort commend)

Comment on attachment 114907 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. I checked Daniel's last comment and it seems well ordered. BTW, you shouldn't check r+ by yourself although reviewer already gave it.

Comment on attachment 114907 [details] Add the UseV8.cmake to WebCore to build bindings of V8 and generate JavaScript codes given IDL input. Clearing flags on attachment: 114907 Committed r100126: <http://trac.webkit.org/changeset/100126>

All reviewed patches have been landed. Closing bug.