[webkit-reviews] review denied: [Bug 42286] [EFL] Add support for using libcurl network backend. : [Attachment 61722] New patch that behaves correctly with cookie functions.

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Rafael Antognolli
<antognolli at profusion.mobi>'s request for review:
Bug 42286: [EFL] Add support for using libcurl network backend.
https://bugs.webkit.org/show_bug.cgi?id=42286

Attachment 61722: New patch that behaves correctly with cookie functions.
https://bugs.webkit.org/attachment.cgi?id=61722&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
Looks good to me. I am saying r- now to fix the typos and get the questions
answered, specially the ones related to gthread.

> +	   Instead of just libsoup, the EFL port now can use libcurl backend.
> +	   This is a step in the direction of removing dependency on glib. Just

> +	   need to pass the option -DNETWORK_BACKEND=curl to camke in order to
> +	   enable it.

typo: camke (in the changelog too)


> @@ -133,9 +157,20 @@ LIST(APPEND WebCore_INCLUDE_DIRECTORIES
>    ${Glib_INCLUDE_DIRS}
>    ${GTK_INCLUDE_DIRS}
>    ${ICU_INCLUDE_DIRS}
> -  ${LIBSOUP24_INCLUDE_DIRS}
> -  ${LIBXML2_INCLUDE_DIRS}
> +  ${LIBXML2_INCLUDE_DIR}

You are changing from DIRS to DIR. Is that intentional?

> @@ -80,7 +88,7 @@ SET(EWebLauncher_LIBRARIES
>      ${EVAS_LIBRARIES}
>      ${Gdk_LIBRARIES}
>      ${Glib_LIBRARIES}
> -    ${LIBSOUP24_LIBRARIES}
> +    ${GTHREAD_LIBRARIES}

If you are trying to make glib optional, adding gthread unconditionally is what
you want?

> diff --git a/WebKit/ChangeLog b/WebKit/ChangeLog
> index 38c36fc..daa9e1d 100644
> --- a/WebKit/ChangeLog
> +++ b/WebKit/ChangeLog

Please start a new ChangeLog in WebKit/efl the soonish.


> diff --git a/cmake/FindGthread.cmake b/cmake/FindGthread.cmake
> new file mode 100644
> index 0000000..7dabbee
> --- /dev/null
> +++ b/cmake/FindGthread.cmake
> @@ -0,0 +1,21 @@
> +# Find include and libraries for GTHREAD library
> +# GTHREAD_INCLUDE	 Directories to include to use GTHREAD
> +# GTHREAD_INCLUDE-I	 Directories to include to use GTHREAD (with -I)
> +# GTHREAD_LIBRARIES	 Libraries to link against to use GTHREAD
> +# GTHREAD_FOUND	 GTHREAD was found
> +
> +IF (UNIX)
> +    INCLUDE (UsePkgConfig)
> +    PKGCONFIG (gthread-2.0 GTHREAD_include_dir GTHREAD_link_dir
GTHREAD_libraries GTHREAD_include)
> +    IF (GTHREAD_include AND GTHREAD_libraries)
> +	   SET (GTHREAD_FOUND TRUE)
> +	   EXEC_PROGRAM ("echo"
> +	       ARGS "${GTHREAD_include} | sed 's/[[:blank:]]*-I/;/g'"
> +	       OUTPUT_VARIABLE GTHREAD_INCLUDE
> +	   )
> +	   SET (GTHREAD_INCLUDE-I ${GTHREAD_include})
> +	   SET (GTHREAD_LIBRARIES ${GTHREAD_libraries})
> +    ELSE (GTHREAD_include AND GTHREAD_libraries)
> +	   SET (GTHREAD_FOUND FALSE)
> +    ENDIF (GTHREAD_include AND GTHREAD_libraries)
> +ENDIF (UNIX)
> diff --git a/cmake/OptionsEfl.cmake b/cmake/OptionsEfl.cmake
> index 3408ea7..65bf4f7 100644
> --- a/cmake/OptionsEfl.cmake
> +++ b/cmake/OptionsEfl.cmake
> @@ -8,14 +8,18 @@ ADD_DEFINITIONS(-DDATA_DIR="${DATA_DIR}")
>  ADD_DEFINITIONS(-DWTF_PLATFORM_EFL=1)
>  SET(WTF_PLATFORM_EFL 1)
>  
> -SET(LIBSOUP24_MIN_VERSION 2.28.2)
> +#
-----------------------------------------------------------------------------
> +# Determine which network backend will be used
> +#
-----------------------------------------------------------------------------
> +SET(ALL_NETWORK_BACKENDS soup curl)
> +OPTION(NETWORK_BACKEND "choose which network backend to use (one of
${ALL_NETWORK_BACKENDS})" "soup")
>  
>  FIND_PACKAGE(Cairo 1.6 REQUIRED)
>  FIND_PACKAGE(EFL REQUIRED)
>  FIND_PACKAGE(Freetype 9.0 REQUIRED)
>  FIND_PACKAGE(GDK 2.10 REQUIRED)
>  FIND_PACKAGE(Glib REQUIRED)
> -FIND_PACKAGE(LibSoup2 2.28.2 REQUIRED)
> +FIND_PACKAGE(Gthread REQUIRED)
>  FIND_PACKAGE(Sqlite REQUIRED)
>  FIND_PACKAGE(LibXml2 2.6 REQUIRED)
>  FIND_PACKAGE(LibXslt 1.1.7 REQUIRED)
> @@ -32,14 +36,6 @@ LIST(APPEND WTF_INCLUDE_DIRECTORIES ${ICU_INCLUDE_DIRS})
>  SET(WTF_PLATFORM_CAIRO 1)
>  ADD_DEFINITIONS(-DWTF_PLATFORM_CAIRO=1)
  

Are these gthread changes related to this patch? should not it go in a
followup?