Bug 61999 - [EFL][WK2] Add an option to build WebKit2.
Summary: [EFL][WK2] Add an option to build WebKit2.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
: 70802 (view as bug list)
Depends on:
Blocks: 61838 70802
  Show dependency treegraph
 
Reported: 2011-06-03 01:27 PDT by Ryuan Choi
Modified: 2011-12-22 02:48 PST (History)
9 users (show)

See Also:


Attachments
Patch (28.69 KB, patch)
2011-06-03 01:31 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
updated_patch (28.15 KB, patch)
2011-06-06 18:02 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
updated_patch (28.27 KB, patch)
2011-06-06 19:04 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
updated_patch (28.63 KB, patch)
2011-06-06 22:30 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
updated_patch (5.96 KB, patch)
2011-06-07 19:31 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
updated_patch (5.96 KB, patch)
2011-06-07 21:19 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (5.86 KB, patch)
2011-06-09 17:41 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (5.88 KB, patch)
2011-06-12 19:36 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (5.90 KB, patch)
2011-10-25 02:49 PDT, Tomasz Morawski
no flags Details | Formatted Diff | Diff
rebased (6.93 KB, patch)
2011-11-16 17:03 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
rebased (6.93 KB, patch)
2011-12-21 15:50 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2011-06-03 01:27:43 PDT
Add ENABLE_WEBKIT2 option in CMakeLists.txt.
If all of sources are landed, it will works.
Comment 1 Ryuan Choi 2011-06-03 01:31:31 PDT
Created attachment 95870 [details]
Patch
Comment 2 Gyuyoung Kim 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 ?
Comment 3 Leandro Pereira 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.
Comment 4 Ryuan Choi 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
Comment 5 Ryuan Choi 2011-06-06 18:02:14 PDT
Created attachment 96165 [details]
updated_patch
Comment 6 Ryuan Choi 2011-06-06 19:04:29 PDT
Created attachment 96174 [details]
updated_patch
Comment 7 Gyuyoung Kim 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.
Comment 8 Ryuan Choi 2011-06-06 22:30:02 PDT
Created attachment 96205 [details]
updated_patch
Comment 9 Ryuan Choi 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.
Comment 10 Leandro Pereira 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.
Comment 11 Ryuan Choi 2011-06-07 19:31:37 PDT
Created attachment 96358 [details]
updated_patch
Comment 12 Ryuan Choi 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.
Comment 13 Ryuan Choi 2011-06-07 21:19:53 PDT
Created attachment 96369 [details]
updated_patch
Comment 14 Leandro Pereira 2011-06-09 07:21:39 PDT
Comment on attachment 96369 [details]
updated_patch

LGTM.
Comment 15 Ryuan Choi 2011-06-09 17:41:37 PDT
Created attachment 96673 [details]
Patch
Comment 16 Ryuan Choi 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.
Comment 17 Gyuyoung Kim 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
Comment 18 Ryuan Choi 2011-06-12 19:36:03 PDT
Created attachment 96907 [details]
Patch
Comment 19 Ryuan Choi 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.
Comment 20 Tomasz Morawski 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.
Comment 21 Ryuan Choi 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.
Comment 22 Tomasz Morawski 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.
Comment 23 Tomasz Morawski 2011-10-25 02:47:43 PDT
*** Bug 70802 has been marked as a duplicate of this bug. ***
Comment 24 Tomasz Morawski 2011-10-25 02:49:58 PDT
Created attachment 112311 [details]
Patch
Comment 25 Jongseok Yang 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.
Comment 26 Ryuan Choi 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.
Comment 27 Ryuan Choi 2011-11-16 17:03:46 PST
Created attachment 115485 [details]
rebased
Comment 28 Ryuan Choi 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
Comment 29 Eric Seidel (no email) 2011-12-21 11:44:13 PST
Comment on attachment 115485 [details]
rebased

rs=me.
Comment 30 Ryuan Choi 2011-12-21 15:50:16 PST
Created attachment 120228 [details]
rebased
Comment 31 Ryuan Choi 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)
Comment 32 WebKit Review Bot 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>
Comment 33 WebKit Review Bot 2011-12-22 02:48:20 PST
All reviewed patches have been landed.  Closing bug.