Bug 62260

Created attachment 96385 [details] Patch

Comment on attachment 96385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96385&action=review > Source/CMakeLists.txt:128 > +IF (NOT ENABLE_WEBKIT2) Why not building both at the same time? I'd prefere a ENABLE_WEBKIT1 and a second ENABLE_WEBKIT2

Created attachment 96407 [details] Patch

(In reply to comment #2) > (From update of attachment 96385 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96385&action=review > > > Source/CMakeLists.txt:128 > > +IF (NOT ENABLE_WEBKIT2) > > Why not building both at the same time? > I'd prefere a ENABLE_WEBKIT1 and a second ENABLE_WEBKIT2 How about ENABLE_WEBKIT? I add ENABLE_WEBKIT for legacy and ENABLE_WEBKIT2 for webkit2. if user choose both, we can get both. As a default, ENABLE_WEBKIT will be enabled. Thanks.

Comment on attachment 96407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96407&action=review > Source/CMakeLists.txt:16 > + SET(ENABLE_WEBKIT ) IMHO a MESSAGE(FATAL_ERROR "config error....") whould be better here. > Source/CMakeLists.txt:29 > + SET(DERIVED_SOURCES_DIR "${CMAKE_BINARY_DIR}/DerivedSources") I don't understand this line. How it related to WK2? > Source/CMakeLists.txt:101 > +IF (ENABLE_WEBKIT) > + SET(WebKit_LIBRARY_NAME WebKit) > +ENDIF () > + > +IF (ENABLE_WEBKIT2) > + SET(WebKit2_LIBRARY_NAME WebKit2) > +ENDIF () I don't see a problem in setting this variables all the time. This will make the configuration much simpler. (the same applies to the other similar changes too) > Source/WebKit2/CMakeLists.txt:94 > +SET(WebKit2_SOURCES Please add an empty line when the containig folder changes like we do at the other files: e.g. after "Platform/CoreIPC" to "Plaform", or "Platform" to "PluginProcess". This makes the long lines a little bit easier to read. > Source/WebKit2/CMakeLists.txt:410 > +SET (WebProcess_NAME ../Programs/WebProcess) > +SET (WebProcess_SOURCES "") > + > +SET (WebProcess_LIBRARIES Please removte the space after SET > Source/WebKit2/CMakeLists.txt:441 > + Is there an unneded empty line at the end?

Comment on attachment 96407 [details] Patch Attachment 96407 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8777630

Created attachment 96425 [details] Patch

(In reply to comment #5) > (From update of attachment 96407 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96407&action=review > > > Source/CMakeLists.txt:16 > > + SET(ENABLE_WEBKIT ) > > IMHO a MESSAGE(FATAL_ERROR "config error....") whould be better here. > Hmm, Is it better to keep Webkit as a default for convenience? IMO, Other engineers, who may not know webkit2, doesn't need to learn new option to build. > > Source/CMakeLists.txt:29 > > + SET(DERIVED_SOURCES_DIR "${CMAKE_BINARY_DIR}/DerivedSources") > > I don't understand this line. How it related to WK2? > I'll check once more. When I started, WebKit2 required it. Now I'm not sure. > > Source/CMakeLists.txt:101 > > +IF (ENABLE_WEBKIT) > > + SET(WebKit_LIBRARY_NAME WebKit) > > +ENDIF () > > + > > +IF (ENABLE_WEBKIT2) > > + SET(WebKit2_LIBRARY_NAME WebKit2) > > +ENDIF () > > I don't see a problem in setting this variables all the time. This will make the configuration much simpler. > (the same applies to the other similar changes too) Done. > > > Source/WebKit2/CMakeLists.txt:94 > > +SET(WebKit2_SOURCES > > Please add an empty line when the containig folder changes like we do at the other files: e.g. after "Platform/CoreIPC" to "Plaform", or "Platform" to "PluginProcess". > This makes the long lines a little bit easier to read. Thank you, I did. > > > Source/WebKit2/CMakeLists.txt:410 > > +SET (WebProcess_NAME ../Programs/WebProcess) > > +SET (WebProcess_SOURCES "") > > + > > +SET (WebProcess_LIBRARIES > > Please removte the space after SET Done. > > > Source/WebKit2/CMakeLists.txt:441 > > + > > Is there an unneded empty line at the end? Done.

Comment on attachment 96425 [details] Patch Attachment 96425 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8777664

(In reply to comment #9) > (From update of attachment 96425 [details]) > Attachment 96425 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/8777664 I can't understand why it was failed. It looks fine in my pc. Gyuyoung, could you check this?

(In reply to comment #8) > (In reply to comment #5) > > > Source/CMakeLists.txt:29 > > > + SET(DERIVED_SOURCES_DIR "${CMAKE_BINARY_DIR}/DerivedSources") > > > > I don't understand this line. How it related to WK2? > > > I'll check once more. > When I started, WebKit2 required it. > Now I'm not sure. > To build InjectedBundleNodeHandle.cpp, DERIVED_SOURCES_DIR is needed. I think that it looks a bug. however, it's not related with this bug. I checked WebKit2/Gtk and it was same. I'll investigate other ports and create different bug if needed. below is error message without it. /workspace/webkits/WebKit2/Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.cpp:37:31: fatal error: WebCore/HTMLNames.h: No such file or directory compilation terminated. make[2]: *** [WebKit2/CMakeFiles/WebKit2.dir/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.cpp.o] 오류 1 make[2]: *** 끝나지 않은 작업을 기다리고 있습니다.... /workspace/webkits/WebKit2/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:48:33: fatal error: WebCore/JSDOMWindow.h: No such file or directory compilation terminated. make[2]: *** [WebKit2/CMakeFiles/WebKit2.dir/WebProcess/InjectedBundle/InjectedBundle.cpp.o] 오류 1 make[1]: *** [WebKit2/CMakeFiles/WebKit2.dir/all] 오류 2 make: *** [all] 오류 2

Comment on attachment 96425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96425&action=review > Source/WebKit2/CMakeLists.txt:427 > + LIST(APPEND WebKit2_SOURCES ${DERIVED_SOURCES_WEBKIT2_DIR}/${_name}Messages.h) You don't need to append the header to the sources list.

Created attachment 96563 [details] Patch

(In reply to comment #12) > (From update of attachment 96425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96425&action=review > > > Source/WebKit2/CMakeLists.txt:427 > > + LIST(APPEND WebKit2_SOURCES ${DERIVED_SOURCES_WEBKIT2_DIR}/${_name}Messages.h) > > You don't need to append the header to the sources list. Thanks, I removed it.

Comment on attachment 96563 [details] Patch Attachment 96563 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8812631

Comment on attachment 96563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96563&action=review > Source/CMakeLists.txt:29 > +IF (ENABLE_WEBKIT2) > + SET(WEBKIT2_DIR "${CMAKE_SOURCE_DIR}/WebKit2") > + SET(DERIVED_SOURCES_DIR "${CMAKE_BINARY_DIR}/DerivedSources") This is needed in order to build Gtk too. Mind setting this unconditionally and setting the others using this var? Like this: SET(DERIVED_SOURCES_DIR "${CMAKE_BINARY_DIR}/DerivedSources") SET(DERIVED_SOURCES_WEBCORE_DIR ${DERIVED_SOURCES_DIR/WebCore) SET(DERIVED_SOURCES_JAVASCRIPTCORE_DIR ${DERIVED_SOURCES_DIR/JavaScriptCore) and so on > Source/WebKit2/CMakeLists.txt:410 > +SET(WebKit2_MESSAGES_IN_FILES > + ${WEBKIT2_DIR}/UIProcess/WebMediaCacheManagerProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebCookieManagerProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/Plugins/PluginProcessProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebInspectorProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebFullScreenManagerProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebDatabaseManagerProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebPageProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebIconDatabase.messages.in > + ${WEBKIT2_DIR}/UIProcess/Downloads/DownloadProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebContext.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebGeolocationManagerProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/DrawingAreaProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebKeyValueStorageManagerProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebResourceCacheManagerProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebProcessProxy.messages.in > + ${WEBKIT2_DIR}/UIProcess/WebApplicationCacheManagerProxy.messages.in > + ${WEBKIT2_DIR}/PluginProcess/WebProcessConnection.messages.in > + ${WEBKIT2_DIR}/PluginProcess/PluginControllerProxy.messages.in > + ${WEBKIT2_DIR}/PluginProcess/PluginProcess.messages.in > + ${WEBKIT2_DIR}/Shared/Plugins/NPObjectMessageReceiver.messages.in > + ${WEBKIT2_DIR}/WebProcess/ApplicationCache/WebApplicationCacheManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/MediaCache/WebMediaCacheManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/Plugins/PluginProxy.messages.in > + ${WEBKIT2_DIR}/WebProcess/WebProcess.messages.in > + ${WEBKIT2_DIR}/WebProcess/Geolocation/WebGeolocationManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/WebCoreSupport/WebDatabaseManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/IconDatabase/WebIconDatabaseProxy.messages.in > + ${WEBKIT2_DIR}/WebProcess/Authentication/AuthenticationManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/ResourceCache/WebResourceCacheManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/FullScreen/WebFullScreenManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/Cookies/WebCookieManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/KeyValueStorage/WebKeyValueStorageManager.messages.in > + ${WEBKIT2_DIR}/WebProcess/WebPage/WebPage.messages.in > + ${WEBKIT2_DIR}/WebProcess/WebPage/WebInspector.messages.in > + ${WEBKIT2_DIR}/WebProcess/WebPage/DrawingArea.messages.in > +) Since we are already in WebKit2 subdir, I think it's better not to keep the ${WEBKIT2_DIR} prefix. If you need it after in ADD_CUSTOM_COMMAND, use the WORKING_DIRECTORY setting. > Source/WebKit2/CMakeLists.txt:436 > +SET(WebProcess_NAME ../Programs/WebProcess) > +SET(WebProcess_SOURCES "") > + > +SET(WebProcess_LIBRARIES > + ${JavaScriptCore_LIBRARY_NAME} > + ${WebCore_LIBRARY_NAME} > + ${WebKit2_LIBRARY_NAME} > +) Did you forget to add the sources? > Source/WebKit2/CMakeLists.txt:452 > +ADD_DEFINITIONS(-DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1) > + Shouldn't this be specific to EFL?

Created attachment 96671 [details] Patch

(In reply to comment #16) > (From update of attachment 96563 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96563&action=review > > > Source/CMakeLists.txt:29 > > +IF (ENABLE_WEBKIT2) > > + SET(WEBKIT2_DIR "${CMAKE_SOURCE_DIR}/WebKit2") > > + SET(DERIVED_SOURCES_DIR "${CMAKE_BINARY_DIR}/DerivedSources") > > This is needed in order to build Gtk too. Mind setting this unconditionally and setting the others using this var? Like this: > SET(DERIVED_SOURCES_DIR "${CMAKE_BINARY_DIR}/DerivedSources") > SET(DERIVED_SOURCES_WEBCORE_DIR ${DERIVED_SOURCES_DIR/WebCore) > SET(DERIVED_SOURCES_JAVASCRIPTCORE_DIR ${DERIVED_SOURCES_DIR/JavaScriptCore) > > and so on I removed IF condition. > > > Source/WebKit2/CMakeLists.txt:410 > > +SET(WebKit2_MESSAGES_IN_FILES > > + ${WEBKIT2_DIR}/UIProcess/WebMediaCacheManagerProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebCookieManagerProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/Plugins/PluginProcessProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebInspectorProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebFullScreenManagerProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebDatabaseManagerProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebPageProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebIconDatabase.messages.in > > + ${WEBKIT2_DIR}/UIProcess/Downloads/DownloadProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebContext.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebGeolocationManagerProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/DrawingAreaProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebKeyValueStorageManagerProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebResourceCacheManagerProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebProcessProxy.messages.in > > + ${WEBKIT2_DIR}/UIProcess/WebApplicationCacheManagerProxy.messages.in > > + ${WEBKIT2_DIR}/PluginProcess/WebProcessConnection.messages.in > > + ${WEBKIT2_DIR}/PluginProcess/PluginControllerProxy.messages.in > > + ${WEBKIT2_DIR}/PluginProcess/PluginProcess.messages.in > > + ${WEBKIT2_DIR}/Shared/Plugins/NPObjectMessageReceiver.messages.in > > + ${WEBKIT2_DIR}/WebProcess/ApplicationCache/WebApplicationCacheManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/MediaCache/WebMediaCacheManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/Plugins/PluginProxy.messages.in > > + ${WEBKIT2_DIR}/WebProcess/WebProcess.messages.in > > + ${WEBKIT2_DIR}/WebProcess/Geolocation/WebGeolocationManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/WebCoreSupport/WebDatabaseManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/IconDatabase/WebIconDatabaseProxy.messages.in > > + ${WEBKIT2_DIR}/WebProcess/Authentication/AuthenticationManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/ResourceCache/WebResourceCacheManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/FullScreen/WebFullScreenManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/Cookies/WebCookieManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/KeyValueStorage/WebKeyValueStorageManager.messages.in > > + ${WEBKIT2_DIR}/WebProcess/WebPage/WebPage.messages.in > > + ${WEBKIT2_DIR}/WebProcess/WebPage/WebInspector.messages.in > > + ${WEBKIT2_DIR}/WebProcess/WebPage/DrawingArea.messages.in > > +) > > Since we are already in WebKit2 subdir, I think it's better not to keep the ${WEBKIT2_DIR} prefix. > > If you need it after in ADD_CUSTOM_COMMAND, use the WORKING_DIRECTORY setting. Thanks, I did. > > > Source/WebKit2/CMakeLists.txt:436 > > +SET(WebProcess_NAME ../Programs/WebProcess) > > +SET(WebProcess_SOURCES "") > > + > > +SET(WebProcess_LIBRARIES > > + ${JavaScriptCore_LIBRARY_NAME} > > + ${WebCore_LIBRARY_NAME} > > + ${WebKit2_LIBRARY_NAME} > > +) > > Did you forget to add the sources? No, WebProcess only requires platform specific one files. > > > Source/WebKit2/CMakeLists.txt:452 > > +ADD_DEFINITIONS(-DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1) > > + > > Shouldn't this be specific to EFL? Right, I removed. Thanks.

Created attachment 96879 [details] Patch

(In reply to comment #19) > Created an attachment (id=96879) [details] > Patch update patch to check ews.

I'm happy to rs=me this, assuming it builds on all EWS bots.

Created attachment 97074 [details] Patch

Comment on attachment 97074 [details] Patch Clearing flags on attachment: 97074 Committed r88991: <http://trac.webkit.org/changeset/88991>

All reviewed patches have been landed. Closing bug.