[CMAKE][WK2] Add an option to build webkit2.

[Ryuan Choi]

This is split by Bug 61999 and covers platform independent CMake build script.

[Ryuan Choi]

Created attachment 96385 [details]
Patch

[Patrick R. Gansterer]

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

[Ryuan Choi]

Created attachment 96407 [details]
Patch

[Ryuan Choi]

(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.

[Patrick R. Gansterer]

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?

[Gyuyoung Kim]

Comment on attachment 96407 [details]
Patch

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

[Ryuan Choi]

Created attachment 96425 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

Comment on attachment 96425 [details]
Patch

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

[Ryuan Choi]

(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?

[Ryuan Choi]

(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

[Lucas De Marchi]

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.

[Ryuan Choi]

Created attachment 96563 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

Comment on attachment 96563 [details]
Patch

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

[Lucas De Marchi]

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?

[Ryuan Choi]

Created attachment 96671 [details]
Patch

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 96879 [details]
Patch

[Ryuan Choi]

(In reply to comment #19)
> Created an attachment (id=96879) [details]
> Patch

update patch to check ews.

[Eric Seidel (no email)]

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

[Ryuan Choi]

Created attachment 97074 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97074 [details]
Patch

Clearing flags on attachment: 97074

Committed r88991: <http://trac.webkit.org/changeset/88991>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.