Bug 61999 – [EFL][WK2] Add an option to build WebKit2.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Ryuan Choi]

Add ENABLE_WEBKIT2 option in CMakeLists.txt.
If all of sources are landed, it will works.

[Ryuan Choi]

Created attachment 95870 [details]
Patch

[Gyuyoung Kim]

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 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]

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

Created attachment 96165 [details]
updated_patch

[Ryuan Choi]

Created attachment 96174 [details]
updated_patch

[Gyuyoung Kim]

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]

Created attachment 96205 [details]
updated_patch

[Ryuan Choi]

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

Created attachment 96358 [details]
updated_patch

[Ryuan Choi]

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

Created attachment 96369 [details]
updated_patch

[Leandro Pereira]

Comment on attachment 96369 [details]
updated_patch

LGTM.

[Ryuan Choi]

Created attachment 96673 [details]
Patch

[Ryuan Choi]

(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 on attachment 96673 [details]
Patch

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

[Ryuan Choi]

Created attachment 96907 [details]
Patch

[Ryuan Choi]

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

Hello,
I have made a patch that depends on your patch. It will be great if your patch could be merged.

[Ryuan Choi]

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

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

*** Bug 70802 has been marked as a duplicate of this bug. ***

[Tomasz Morawski]

Created attachment 112311 [details]
Patch

[Jongseok Yang]

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]

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

Created attachment 115485 [details]
rebased

[Ryuan Choi]

(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 on attachment 115485 [details]
rebased

rs=me.

[Ryuan Choi]

Created attachment 120228 [details]
rebased

[Ryuan Choi]

Comment on attachment 120228 [details]
rebased

Thanks eric.

I rebased patch for applying eunmi's patch (Bug 62444)

[WebKit Review Bot]

Comment on attachment 120228 [details]
rebased

Clearing flags on attachment: 120228

Committed r103512: <http://trac.webkit.org/changeset/103512>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.