Bug 42286 – [EFL] Add support for using libcurl network backend.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Rafael Antognolli]

[EFL] Add support for using libcurl network backend.

[Rafael Antognolli]

Created attachment 61561 [details]
Patch

[Rafael Antognolli]

Comment on attachment 61561 [details]
Patch

Sending an updated patch soon.

[Rafael Antognolli]

Created attachment 61722 [details]
New patch that behaves correctly with cookie functions.

[Antonio Gomes]

Comment on attachment 61722 [details]
New patch that behaves correctly with cookie functions.

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?

[Rafael Antognolli]

(In reply to comment #4)
> (From update of attachment 61722 [details])
> 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)

Ok, fixing that.

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

Yes, I don't know why but the FindXml defines DIR instead of DIRS (which is the default). This was wrong but somehow worked before trying the curl backend.

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

Same here, I think libsoup was adding the -lgthread flag automatically and that's how it was working before. But now in order to keep it compiling, it's necessary to add this gthread flag, since we still didn't remove glib completely. This is done in the next patch (I've just sent it).

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

Sure :-) 

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

As I explained above, this was necessary because libsoup was getting this dependency automatically, but now that I removed it our call for g_thread_init in our ewk_main.cpp was unresolved.

Should I send the changes from bug 42480 in the same patch, keep them splitted, or split even more? (I'm just trying make sure that both changes keep it compiling fine).

[Antonio Gomes]

> Yes, I don't know why but the FindXml defines DIR instead of DIRS (which is the default). This was wrong but somehow worked before trying the curl backend.

Ok. Keep it

> > > @@ -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?
> 
> Same here, I think libsoup was adding the -lgthread flag automatically and that's how it was working before. But now in order to keep it compiling, it's necessary to add this gthread flag, since we still didn't remove glib completely. This is done in the next patch (I've just sent it).

if we commit this patch without the gthread part, does your build get broken? for both curl or soup?

[Rafael Antognolli]

Created attachment 61850 [details]
Patch

Is it correct the way I added the WebKit/efl/ChangeLog? Should I add an entry to ./ChangeLog too?

[Rafael Antognolli]

Created attachment 61855 [details]
Patch

Ok, I think now it's right. It has correct entries for each ChangeLog.

[Antonio Gomes]

Comment on attachment 61855 [details]
Patch

thank you spinning off the gthread part.

r=me

[WebKit Commit Bot]

Comment on attachment 61855 [details]
Patch

Rejecting patch 61855 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 61855, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 66, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 48, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

[Rafael Antognolli]

Created attachment 62431 [details]
Patch

[Antonio Gomes]

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

It is just a rebase against ToT? what changed?

[Rafael Antognolli]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=62431) [details] [details]
> > Patch
> 
> It is just a rebase against ToT? what changed?

The following line:

+SET(NETWORK_BACKEND "soup" CACHE STRING "choose which network backend to use (one of ${ALL_NETWORK_BACKENDS})")

was changed from OPTION(NETWORK_BACKEND ...) because OPTION is used just for boolean values. I don't know why I never got a warning for this before, but now it was just not working.

This was also fixed now:
+    SET(ENABLE_GLIB_SUPPORT 1)

Previous patch was setting it to "ON", which doesn't work as expected.

[WebKit Commit Bot]

Comment on attachment 62431 [details]
Patch

Clearing flags on attachment: 62431

Committed r63988: <http://trac.webkit.org/changeset/63988>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.