Attachment 115749[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/CMakeLists.txt', u'So..." exit_code: 1
ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 115750[details]
Add the configuration of cmake to select between JSC and V8
View in context: https://bugs.webkit.org/attachment.cgi?id=115750&action=review
How does this interact with WebKit, WebCore and Tools, all of which include JavaScriptCore directories and need its headers?
> Source/CMakeLists.txt:52
> +SET(JS_ENGINE "JSC" CACHE STRING "choose which javascrit engine to use (one of ${ALL_JAVASCRIPT_ENGINES})")
typo: "javascrit"
Comment on attachment 115750[details]
Add the configuration of cmake to select between JSC and V8
View in context: https://bugs.webkit.org/attachment.cgi?id=115750&action=review> ChangeLog:3
> + [CMAKE] Add the configuration of cmake to select between JSC and V8.
not importent, but cmake is usually written as CMake
>> Source/CMakeLists.txt:52
>> +SET(JS_ENGINE "JSC" CACHE STRING "choose which javascrit engine to use (one of ${ALL_JAVASCRIPT_ENGINES})")
>
> typo: "javascrit"
I'd prefer the long name JAVASCRIPT_ENGINE
> Source/CMakeLists.txt:56
> + SET(WTF_USE_V8 1)
If you set WTF_USE_V8, i think you should set WTF_USE_JSC too
> Source/CMakeLists.txt:146
> + ADD_SUBDIRECTORY(JavaScriptCore/wtf)
I think you should remove ADD_SUBDIRECTROY(wtf) from JavaScriptCore/CMakeLists.txt and always ADD_SUBDIRECTORY(JavaScriptCore/wtf) here
> Source/CMakeLists.txt:148
> + ADD_SUBDIRECTORY(JavaScriptCore)
should be IF (WTF_USE_JSC) then
> Source/CMakeLists.txt:174
> +IF (NOT WTF_USE_V8)
IF (WTF_USE_JSC) instead
> Source/cmake/WebKitMacros.cmake:88
> + SET(_outputfiles "${_outputfiles}" ${DERIVED_SOURCES_WEBCORE_DIR}/${_namespace}ElementFactory.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/${_namespace}ElementFactory.h ${DERIVED_SOURCES_WEBCORE_DIR}/${JAVASCRIPT_PREFIX}${_namespace}ElementWrapperFactory.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/${JAVASCRIPT_PREFIX}${_namespace}ElementWrapperFactory.h)
What about the calls to GENERATE_DOM_NAMES in WebCore/CMakeLists.txt? This will result in an error, since e.g. JSHTMLElementWrapperFactory.cpp won't get generated. Maybe there are more problems of this kind.
Comment on attachment 115750[details]
Add the configuration of cmake to select between JSC and V8
View in context: https://bugs.webkit.org/attachment.cgi?id=115750&action=review>>> Source/CMakeLists.txt:52
>>> +SET(JS_ENGINE "JSC" CACHE STRING "choose which javascrit engine to use (one of ${ALL_JAVASCRIPT_ENGINES})")
>>
>> typo: "javascrit"
>
> I'd prefer the long name JAVASCRIPT_ENGINE
ok, I will change it to "JAVASCRIPT_ENGINE" :)
>> Source/CMakeLists.txt:56>
> If you set WTF_USE_V8, i think you should set WTF_USE_JSC too
I will define WTF_USE_JSC and set it too.
>> Source/CMakeLists.txt:146
>> + ADD_SUBDIRECTORY(JavaScriptCore/wtf)
>
> I think you should remove ADD_SUBDIRECTROY(wtf) from JavaScriptCore/CMakeLists.txt and always ADD_SUBDIRECTORY(JavaScriptCore/wtf) here
ok... Regardless of javascript engine, I make ADD_SUBDIRECTORY(JavaScriptCore/wtf) default and remove it from JavaScriptCore/CMakeLists.txt
>> Source/CMakeLists.txt:174
>> +IF (NOT WTF_USE_V8)
>
> IF (WTF_USE_JSC) instead
ok.. I replace it with "WTF_USE_JSC"
>> Source/cmake/WebKitMacros.cmake:88
>> + SET(_outputfiles "${_outputfiles}" ${DERIVED_SOURCES_WEBCORE_DIR}/${_namespace}ElementFactory.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/${_namespace}ElementFactory.h ${DERIVED_SOURCES_WEBCORE_DIR}/${JAVASCRIPT_PREFIX}${_namespace}ElementWrapperFactory.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/${JAVASCRIPT_PREFIX}${_namespace}ElementWrapperFactory.h)
>
> What about the calls to GENERATE_DOM_NAMES in WebCore/CMakeLists.txt? This will result in an error, since e.g. JSHTMLElementWrapperFactory.cpp won't get generated. Maybe there are more problems of this kind.
hm.. I don't know exactly what you mean,,
so I will upload other patches for v8 in this bug... and then please review them.
Comment on attachment 116055[details]
Add the configuration of cmake to select between JSC and V8
View in context: https://bugs.webkit.org/attachment.cgi?id=116055&action=review> Source/WebCore/CMakeLists.txt:871
> - html/shadow/DetailsMarkerControl.cpp
> + html/shadow/DetailsMarkerControl.cpp
Unneeded change.
> Source/WebCore/CMakeLists.txt:2429
> -ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME})
> +IF (WTF_USE_V8)
> + ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${WTF_LIBRARY_NAME})
> +ELSEIF (WTF_USE_JSC)
> + ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME})
> +ENDIF()
I think this section is not needed -- TARGET_LINK_LIBRARIES will automatically create the dependencies between the targets.
Comment on attachment 116055[details]
Add the configuration of cmake to select between JSC and V8
View in context: https://bugs.webkit.org/attachment.cgi?id=116055&action=review>> Source/WebCore/CMakeLists.txt:871
>> + html/shadow/DetailsMarkerControl.cpp
>
> Unneeded change.
There are some spaces at the end of it.. I made a mistake,, :(
>> Source/WebCore/CMakeLists.txt:2429
>> +ENDIF()
>
> I think this section is not needed -- TARGET_LINK_LIBRARIES will automatically create the dependencies between the targets.
If only JavaScriptCore_LIBRARY_NAME is setted as TARGET_LINK_LIBRARIES when build with v8.
Undefined reference errors occurr related with WTF libraries. so I think that this change need to be included.
(In reply to comment #21)
> Why do you submit two patches to a Bug. Basically, we submit a patch to a Bug except for special case.
At first, I had added just one patch related to Source/CMakeLists.txt and Source/cmake/WebKitMacros.cmake. but I thought that WebCore's CMake patch is also needed for review. so I added it as another patch.
If you think that just one patch is better... I'll make them one patch.. tanks. :)
Comment on attachment 116490[details]
Add the configuration of CMake to select between JSC and V8
View in context: https://bugs.webkit.org/attachment.cgi?id=116490&action=review
It'd be good if you could submit the same patch (without flagging cq? or r?) setting V8 as the default engine and then manually submit it for EWS analysis, just so that we can check if the V8 code path builds fine.
> Source/CMakeLists.txt:8
> +IF (WTF_USE_JSC)
> + ADD_SUBDIRECTORY(JavaScriptCore)
> +ENDIF ()
> +
> +ADD_SUBDIRECTORY(JavaScriptCore/wtf)
It makes sense to build wtf first.
> Source/CMakeLists.txt:23
> +IF (WTF_USE_V8)
> + WEBKIT_SET_EXTRA_COMPILER_FLAGS(${WTF_LIBRARY_NAME})
WTF needs the extra compiler flags regardless of the engine chosen.
> Source/WebCore/CMakeLists.txt:2441
> -ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME})
> +IF (WTF_USE_V8)
> + ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${WTF_LIBRARY_NAME})
> +ELSEIF (WTF_USE_JSC)
> + ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME})
> +ENDIF()
> TARGET_LINK_LIBRARIES(${WebCore_LIBRARY_NAME} ${WebCore_LIBRARIES})
My point with this in a previous review is: WebCore_LIBRARIES is set to WTF_LIBRARY_NAME when V8 is chosen, and JavaScriptCore_LIBRARY_NAME when JSC is chosen. This means that TARGET_LINK_LIBRARIES() will create a dependency between WebCore_LIBRARY_NAME and WebCore_LIBRARIES, which makes the ADD_DEPENDENCIES() calls redundant. What errors do you get when you remove the ADD_DEPENDENCIES() block?
Comment on attachment 116490[details]
Add the configuration of CMake to select between JSC and V8
View in context: https://bugs.webkit.org/attachment.cgi?id=116490&action=review>> Source/CMakeLists.txt:8
>> +ADD_SUBDIRECTORY(JavaScriptCore/wtf)
>
> It makes sense to build wtf first.
I'll make WTF first.
>> Source/CMakeLists.txt:23
>> + WEBKIT_SET_EXTRA_COMPILER_FLAGS(${WTF_LIBRARY_NAME})
>
> WTF needs the extra compiler flags regardless of the engine chosen.
I'll make the WTF extra compiler flags to default option regardless of the engine.
>> Source/WebCore/CMakeLists.txt:2441
>> TARGET_LINK_LIBRARIES(${WebCore_LIBRARY_NAME} ${WebCore_LIBRARIES})
>
> My point with this in a previous review is: WebCore_LIBRARIES is set to WTF_LIBRARY_NAME when V8 is chosen, and JavaScriptCore_LIBRARY_NAME when JSC is chosen. This means that TARGET_LINK_LIBRARIES() will create a dependency between WebCore_LIBRARY_NAME and WebCore_LIBRARIES, which makes the ADD_DEPENDENCIES() calls redundant. What errors do you get when you remove the ADD_DEPENDENCIES() block?
OK... I understand what you mean. thanks
I remove ADD_DEPENDENCIES of WTF and JavaScirptCore
2011-11-17 23:52 PST, jaehoon jeong
2011-11-17 23:54 PST, jaehoon jeong
2011-11-21 02:13 PST, jaehoon jeong
2011-11-21 02:15 PST, jaehoon jeong
2011-11-22 21:01 PST, jaehoon jeong
2011-11-22 21:02 PST, jaehoon jeong
2011-11-22 21:04 PST, jaehoon jeong
2011-11-23 02:04 PST, jaehoon jeong
2011-11-23 02:04 PST, jaehoon jeong
2011-11-24 01:00 PST, jaehoon jeong
2011-11-24 01:01 PST, jaehoon jeong
2011-11-24 02:33 PST, jaehoon jeong
2011-11-29 02:15 PST, jaehoon jeong