[webkit-reviews] review denied: [Bug 25263] [Gtk] WebKit GTK with libsoup won't recognize proxies : [Attachment 30535] added an option to enable GNOME-specific features of libsoup, default=no

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has denied Wang Lu <coolwanglu at gmail.com>'s
request for review:
Bug 25263: [Gtk] WebKit GTK with libsoup won't recognize proxies
https://bugs.webkit.org/show_bug.cgi?id=25263

Attachment 30535: added an option to enable GNOME-specific features of libsoup,
default=no
https://bugs.webkit.org/attachment.cgi?id=30535&action=review

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>
> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 43961)
> +++ ChangeLog (working copy)
> @@ -1,3 +1,12 @@
> +2009-05-21  WANG Lu <coolwanglu at gmail.com> 
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +		Added --with-libsoup-gnome (default=no) to enable
GNOME-specific
> +		features of libsoup, an important one is the proxy-resovler
feature

Please fix the spaces here.

> Index: configure.ac
> ===================================================================
> --- configure.ac	(revision 43933)
> +++ configure.ac	(working copy)
> @@ -497,7 +497,6 @@ AC_ARG_ENABLE([jit],
>  if test "$enable_jit" = "yes"; then
>      case "$host_cpu" in
>	   i*86|x86_64)
> -	       AC_DEFINE([ENABLE_JIT], [1], [Define to enable JIT])

Is this part of the patch? 

>	       AC_DEFINE([ENABLE_YARR], [1], [Define to enable YARR])
>	       AC_DEFINE([ENABLE_YARR_JIT], [1], [Define to enable YARR JIT])
>	       AC_DEFINE([ENABLE_JIT_OPTIMIZE_CALL], [1], [Define to enable
optimizing calls])
> @@ -550,10 +549,28 @@ else
>     CFLAGS="$CFLAGS -O0"
>  fi
>  
> -PKG_CHECK_MODULES([LIBSOUP],
> -		     [libsoup-2.4 >= $LIBSOUP_REQUIRED_VERSION])
> -AC_SUBST([LIBSOUP_CFLAGS])
> -AC_SUBST([LIBSOUP_LIBS])
> +# check whether to enable libsoup-gnome
> +AC_MSG_CHECKING([whether to use GNOME-specific features of libsoup])
> +AC_ARG_WITH(libsoup_gnome,
> +		 AC_HELP_STRING([--with-libsoup-gnome],
> +				[using GNOME-specific features of libsoup
[default=no]]),
> +		 [],[with_libsoup_gnome="no"])
> +AC_MSG_RESULT([$with_libsoup_gnome])
> +
> +if test "$with_libsoup_gnome" = "yes"; then
> +    PKG_CHECK_MODULES([LIBSOUP],
> +			 [libsoup-gnome-2.4 >= $LIBSOUP_REQUIRED_VERSION]) #
FIXME: does libsoup-gnome need a separated REQUIRED_VERSION ?

FIXME is not required. They should be the same.

> +    AC_DEFINE([ENABLE_LIBSOUP_GNOME],[1],[Define to enable libsoup-gnome])

Please use USE_SOUP_GNOME and this should be AM_CONDITIONAL. And move this in
the end of this file.

> +    AC_SUBST(LIBSOUPE_CFLAGS)
> +    AC_SUBST(LIBSOUPE_LIBS)

LIBSOUPE?

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 43961)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,13 @@
> +2009-05-21  WANG Lu	<coolwanglu at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +		If libsoup-gnome is enabled, add proper parameters to enable
> +		GNOME-specific features while creating a new soup session.

Fix the indentation please.

> Index: WebCore/platform/network/soup/ResourceHandleSoup.cpp
> ===================================================================
> --- WebCore/platform/network/soup/ResourceHandleSoup.cpp	(revision
43933)
> +++ WebCore/platform/network/soup/ResourceHandleSoup.cpp	(working copy)
> @@ -48,7 +48,12 @@
>  #include <fcntl.h>
>  #include <gio/gio.h>
>  #include <gtk/gtk.h>
> +
> +#ifdef ENABLE_LIBSOUP_GNOME 

Should be USE_SOUP_GNOME per comment above. Then USE(SOUP_GNOME) here.

>  #include <libsoup/soup.h>
> +

Not required.

>  static SoupSession* createSoupSession()
>  {
> +#ifdef ENABLE_LIBSOUP_GNOME

USE(SOUP_GNOME) here.

> +    return
soup_session_async_new_with_options(SOUP_SESSION_ADD_FEATURE_BY_TYPE,
SOUP_TYPE_GNOME_FEATURES_2_26,
> +						  NULL);

And put this all in one line please.

r- for now until the comments above have been addressed. 

Thanks!