Bug 61999

Summary: [EFL][WK2] Add an option to build WebKit2.
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit2Assignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, js45.yang, kenneth, leandro, lucas.de.marchi, sangseok.lim, t.morawski, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838, 70802    
Attachments:
Description Flags
Patch
none
updated_patch
none
updated_patch
none
updated_patch
none
updated_patch
none
updated_patch
none
Patch
none
Patch
none
Patch
none
rebased
none
rebased none

Ryuan Choi
Reported 2011-06-03 01:27:43 PDT
Add ENABLE_WEBKIT2 option in CMakeLists.txt. If all of sources are landed, it will works.
Attachments
Patch (28.69 KB, patch)
2011-06-03 01:31 PDT, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
updated_patch (28.15 KB, patch)
2011-06-06 18:02 PDT, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
updated_patch (28.27 KB, patch)
2011-06-06 19:04 PDT, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
updated_patch (28.63 KB, patch)
2011-06-06 22:30 PDT, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
updated_patch (5.96 KB, patch)
2011-06-07 19:31 PDT, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
updated_patch (5.96 KB, patch)
2011-06-07 21:19 PDT, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
Patch (5.86 KB, patch)
2011-06-09 17:41 PDT, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
Patch (5.88 KB, patch)
2011-06-12 19:36 PDT, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
Patch (5.90 KB, patch)
2011-10-25 02:49 PDT, Tomasz Morawski 		no flags
DetailsFormatted DiffDiff
rebased (6.93 KB, patch)
2011-11-16 17:03 PST, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
rebased (6.93 KB, patch)
2011-12-21 15:50 PST, Ryuan Choi 		no flags
DetailsFormatted DiffDiff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.
Ryuan Choi
Comment 1 2011-06-03 01:31:31 PDT
Created attachment 95870 [details] Patch
Gyuyoung Kim
Comment 2 2011-06-03 01:46:02 PDT
Comment on attachment 95870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95870&action=review > Source/CMakeLists.txt:20 > +SET(DERIVED_SOURCES_WEBKIT2_DIR "${CMAKE_BINARY_DIR}/DerivedSources/WebKit2") How do you think WebKit 2 variables are only set when WEBKIT2 is enabled ?
Leandro Pereira
Comment 3 2011-06-03 10:03:07 PDT
Comment on attachment 95870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95870&action=review > Source/CMakeLists.txt:125 > ADD_SUBDIRECTORY(WebKit) > +IF (ENABLE_WEBKIT2) > + ADD_SUBDIRECTORY(WebKit2) > +ENDIF () WebKit always be built if one only wants WebKit2. Is this the expected behaviour? > Source/CMakeLists.txt:133 > +IF (ENABLE_WEBKIT2) > + INCLUDE_IF_EXISTS(${TOOLS_DIR}/MiniBrowser/${WEBKIT_PORT_DIR}/CMakeLists${PORT}.txt) > +ENDIF () I'd change Tools/CMakeListsEfl.txt instead.
Ryuan Choi
Comment 4 2011-06-06 17:39:03 PDT
(In reply to comment #2) > (From update of attachment 95870 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95870&action=review > > > Source/CMakeLists.txt:20 > > +SET(DERIVED_SOURCES_WEBKIT2_DIR "${CMAKE_BINARY_DIR}/DerivedSources/WebKit2") > > How do you think WebKit 2 variables are only set when WEBKIT2 is enabled ? If then, we should check webkit2 option at front of Source/CMakeLists.txt. cmake/WebKitFS.cmake should know that DERIVED_SOURCES_WEBKIT2_DIR. If it's not harmful, I like it as default. (In reply to comment #3) > (From update of attachment 95870 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95870&action=review > > > Source/CMakeLists.txt:125 > > ADD_SUBDIRECTORY(WebKit) > > +IF (ENABLE_WEBKIT2) > > + ADD_SUBDIRECTORY(WebKit2) > > +ENDIF () > > WebKit always be built if one only wants WebKit2. Is this the expected behaviour? > I'll change not to build webkit when WebKit2 was enabled. My first patch is exactly what I want like WebKit2/Gtk. But now, we don't have any requirements for that. So, only one library for webkit2 looks fine. > > Source/CMakeLists.txt:133 > > +IF (ENABLE_WEBKIT2) > > + INCLUDE_IF_EXISTS(${TOOLS_DIR}/MiniBrowser/${WEBKIT_PORT_DIR}/CMakeLists${PORT}.txt) > > +ENDIF () > > I'd change Tools/CMakeListsEfl.txt instead. Ok. If then, I'll remove it and it will be covered by Bug 61850
Ryuan Choi
Comment 5 2011-06-06 18:02:14 PDT
Created attachment 96165 [details] updated_patch
Ryuan Choi
Comment 6 2011-06-06 19:04:29 PDT
Created attachment 96174 [details] updated_patch
Gyuyoung Kim
Comment 7 2011-06-06 22:10:55 PDT
Comment on attachment 96174 [details] updated_patch View in context: https://bugs.webkit.org/attachment.cgi?id=96174&action=review > Source/CMakeLists.txt:85 > +SET(WebKit2_LIBRARY_NAME WebKit2) I think this setting also can be enabled when WEBKIT2 is enabled. Any problems ? > Source/CMakeLists.txt:103 > +SET(WebKit2_LIBRARY_TYPE SHARED) same.
Ryuan Choi
Comment 8 2011-06-06 22:30:02 PDT
Created attachment 96205 [details] updated_patch
Ryuan Choi
Comment 9 2011-06-06 22:30:41 PDT
(In reply to comment #7) > (From update of attachment 96174 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96174&action=review > > > Source/CMakeLists.txt:85 > > +SET(WebKit2_LIBRARY_NAME WebKit2) > > I think this setting also can be enabled when WEBKIT2 is enabled. Any problems ? > > > Source/CMakeLists.txt:103 > > +SET(WebKit2_LIBRARY_TYPE SHARED) > > same. No problem. I updated.
Leandro Pereira
Comment 10 2011-06-07 07:47:56 PDT
Comment on attachment 96205 [details] updated_patch On a quick glance, looks good; but again, I'm not familiar with WebKit2 internals so I can't properly review this. I'd split this into two patches: one for the platform-independent part, and another for the EFL-related part. Makes it easier to review.
Ryuan Choi
Comment 11 2011-06-07 19:31:37 PDT
Created attachment 96358 [details] updated_patch
Ryuan Choi
Comment 12 2011-06-07 19:46:26 PDT
(In reply to comment #10) > (From update of attachment 96205 [details]) > On a quick glance, looks good; but again, I'm not familiar with WebKit2 internals so I can't properly review this. > > I'd split this into two patches: one for the platform-independent part, and another for the EFL-related part. Makes it easier to review. Thanks. As your suggestion, I split patches and created Bug 62260 for platform independent part.
Ryuan Choi
Comment 13 2011-06-07 21:19:53 PDT
Created attachment 96369 [details] updated_patch
Leandro Pereira
Comment 14 2011-06-09 07:21:39 PDT
Comment on attachment 96369 [details] updated_patch LGTM.
Ryuan Choi
Comment 15 2011-06-09 17:41:37 PDT
Created attachment 96673 [details] Patch
Ryuan Choi
Comment 16 2011-06-09 17:44:12 PDT
(In reply to comment #15) > Created an attachment (id=96673) [details] > Patch I updated to follow changes of Bug 62260, Bug 62363.
Gyuyoung Kim
Comment 17 2011-06-10 06:13:44 PDT
Comment on attachment 96673 [details] Patch Attachment 96673 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8826394
Ryuan Choi
Comment 18 2011-06-12 19:36:03 PDT
Created attachment 96907 [details] Patch
Ryuan Choi
Comment 19 2011-06-12 19:36:52 PDT
(In reply to comment #17) > (From update of attachment 96673 [details]) > Attachment 96673 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/8826394 I can't understand this failure. I updated same patch for checking once more.
Tomasz Morawski
Comment 20 2011-10-24 23:43:48 PDT
Hello, I have made a patch that depends on your patch. It will be great if your patch could be merged.
Ryuan Choi
Comment 21 2011-10-24 23:53:15 PDT
(In reply to comment #20) > Hello, > I have made a patch that depends on your patch. It will be great if your patch could be merged. IMO, you'd better to upload new patch for this bug instead of creating new one.
Tomasz Morawski
Comment 22 2011-10-24 23:58:26 PDT
(In reply to comment #21) > (In reply to comment #20) > > Hello, > > I have made a patch that depends on your patch. It will be great if your patch could be merged. > > IMO, you'd better to upload new patch for this bug instead of creating new one. OK, I will join our patches.
Tomasz Morawski
Comment 23 2011-10-25 02:47:43 PDT
*** Bug 70802 has been marked as a duplicate of this bug. ***
Tomasz Morawski
Comment 24 2011-10-25 02:49:58 PDT
Created attachment 112311 [details] Patch
Jongseok Yang
Comment 25 2011-11-15 17:53:27 PST
r100362 removed WebCookieManagerEfl.cpp. (http://trac.webkit.org/changeset/100362) So, remove WebCookieManagerEfl.cpp from CMakeListEfl.txt and insert WebCookieManagerSoup.cpp, WebCookieManagerCurl.cpp If WTF_USE_SOUP is defined, WebCookieManagerSoup.cpp should be built. If WTF_USE_CURL, WebCookieManagerCurl.cpp should be built.
Ryuan Choi
Comment 26 2011-11-15 17:57:49 PST
(In reply to comment #25) > r100362 removed WebCookieManagerEfl.cpp. (http://trac.webkit.org/changeset/100362) > So, remove WebCookieManagerEfl.cpp from CMakeListEfl.txt and insert WebCookieManagerSoup.cpp, WebCookieManagerCurl.cpp > > If WTF_USE_SOUP is defined, WebCookieManagerSoup.cpp should be built. > If WTF_USE_CURL, WebCookieManagerCurl.cpp should be built. Right, and also we decide to use PlatformEfl.cmake for platform specific things. Tomasz, Can you rebase it? Otherwise I can do.
Ryuan Choi
Comment 27 2011-11-16 17:03:46 PST
Created attachment 115485 [details] rebased
Ryuan Choi
Comment 28 2011-11-16 17:45:46 PST
(In reply to comment #27) > Created an attachment (id=115485) [details] > rebased Apply Jongseok's change and Bug 56705's changes which decide to use Platform${Port}.cmake instead of CMakeLists${Port}.cmake
Eric Seidel (no email)
Comment 29 2011-12-21 11:44:13 PST
Comment on attachment 115485 [details] rebased rs=me.
Ryuan Choi
Comment 30 2011-12-21 15:50:16 PST
Created attachment 120228 [details] rebased
Ryuan Choi
Comment 31 2011-12-21 15:52:53 PST
Comment on attachment 120228 [details] rebased Thanks eric. I rebased patch for applying eunmi's patch (Bug 62444)
WebKit Review Bot
Comment 32 2011-12-22 02:48:12 PST
Comment on attachment 120228 [details] rebased Clearing flags on attachment: 120228 Committed r103512: <http://trac.webkit.org/changeset/103512>
WebKit Review Bot
Comment 33 2011-12-22 02:48:20 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.