Bug 25263 - [Gtk] WebKit GTK with libsoup won't recognize proxies
Summary: [Gtk] WebKit GTK with libsoup won't recognize proxies
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-17 02:07 PDT by Wang Lu
Modified: 2009-12-20 02:42 PST (History)
4 users (show)

See Also:


Attachments
patch for enable proxy in libsoup (671 bytes, patch)
2009-04-17 02:16 PDT, Wang Lu
no flags Details | Formatted Diff | Diff
to enable proxy-resolver feature in libsoup-gnome (3.48 KB, patch)
2009-04-20 00:26 PDT, Wang Lu
jmalonzo: review-
Details | Formatted Diff | Diff
enable libsoup-gnome features, default=no (4.32 KB, patch)
2009-05-21 01:40 PDT, Wang Lu
no flags Details | Formatted Diff | Diff
added an option to enable GNOME-specific features of libsoup, default=no (4.30 KB, patch)
2009-05-21 01:53 PDT, Wang Lu
no flags Details | Formatted Diff | Diff
added an option to enable GNOME-specific features of libsoup, default=no (4.31 KB, patch)
2009-05-21 02:28 PDT, Wang Lu
jmalonzo: review-
Details | Formatted Diff | Diff
added an option to enable GNOME-specific features of libsoup, default=no (4.76 KB, patch)
2009-05-30 23:23 PDT, Wang Lu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wang Lu 2009-04-17 02:07:48 PDT
Environment:
    Debian 2.6.18 amd64
    latest libsoup & libproxy till now
    WebKit 1.1.5 (should also affect latest nightly build)

My computer connect with Internet through a HTTP proxy, which is set in the 'http_proxy' environment variable. 

I've been working on WebKit for months, at first, while I was using 1.0.1, (with curl as its backend), everything's all right. But recently I updated it to 1.1.5, and found that GtkLauncher could not load http://www.google.com.

It took me some time to realize that the network backend has been changed to libsoup. And I did test my libsoup, which worked well.

It took me another some time to compare WebCore/platform/network/soup/ResourceHandlSoup.cpp and test.c in libsoup tests. And at last my conclusion is maybe the proxy-resolver feature is not enabled. So I made a dirty patch:

1.in configure, add dependency rule of libsoup-gnome-2.4 (I failed in editing configure.ac and re-automake, for some unknown reason)

2.in WebCore/platform/network/soup/ResourceHandleSoup.cpp
    1) at the beginning, add
#include <libsoup/soup-gnome.h>

    2) at arount line 444, replace the body of function 'static SoupSession* createSoupSession()' with the following code

    return soup_session_async_new_with_options(SOUP_SESSION_ADD_FEATURE_BY_TYPE, SOUP_TYPE_GNOME_FEATURES_2_26, NULL);

I don't know whether it'll affect other parts of webkitgtk, but at least it works for me

so please check this, thx
Comment 1 Wang Lu 2009-04-17 02:16:24 PDT
Created attachment 29573 [details]
patch for enable proxy in libsoup

based on Webkit-r42569
Comment 2 Jan Alonzo 2009-04-19 02:51:35 PDT
(In reply to comment #1)
> Created an attachment (id=29573) [review]
> patch for enable proxy in libsoup
> 
> based on Webkit-r42569
> 

This is good. For adding the configure option, you could just look at the LIBSOUP parts in configure.ac and add a LIBSOUP_GNOME and check for libsoup-gnome-2.4.

Please add a ChangeLog and set the review flag to "?" as well so reviewers can review your changes.

Thanks,
Comment 3 Wang Lu 2009-04-20 00:26:20 PDT
Created attachment 29611 [details]
to enable proxy-resolver feature in libsoup-gnome

Detect libsoup-gnome, and use it instead of libsoup, if exists.
If libsoup-gnome exists, enable SOUP_TYPE_GNOME_FEATURES_2_26 when creating a new soup session
Comment 4 Xan Lopez 2009-04-20 06:47:49 PDT
I'm really not sure that this should be done in webkit, or that it should autodetect the library and enable the proxy without explicit setup (we are not doing this for anything that it's not a hard dependency AFAIK).
Comment 5 Jan Alonzo 2009-05-15 17:15:51 PDT
Comment on attachment 29611 [details]
to enable proxy-resolver feature in libsoup-gnome

(In reply to comment #3)
> Created an attachment (id=29611) [review]
> to enable proxy-resolver feature in libsoup-gnome
> 
> Detect libsoup-gnome, and use it instead of libsoup, if exists.
> If libsoup-gnome exists, enable SOUP_TYPE_GNOME_FEATURES_2_26 when creating a
> new soup session

Since libsoup-gnome is not a hard-dependency, this feature should be configurable at configure time and should be disabled by default. I would suggest adding --with-gnome or --with-libsoup-gnome in configure.ac for this.

I'm going to say r- for now until have this feature configurable.
Comment 6 Wang Lu 2009-05-21 01:40:52 PDT
Created attachment 30529 [details]
enable libsoup-gnome features, default=no

this is based on r43933 because I can't build latest svn version, see https://bugs.webkit.org/show_bug.cgi?id=25910
Comment 7 Wang Lu 2009-05-21 01:53:44 PDT
Created attachment 30531 [details]
added an option to enable GNOME-specific features of libsoup, default=no

this is based on r43961, because latest svn version of webkitgtk  could not compile, see https://bugs.webkit.org/show_bug.cgi?id=25910
Comment 8 Wang Lu 2009-05-21 02:28:39 PDT
Created attachment 30535 [details]
added an option to enable GNOME-specific features of libsoup, default=no

this is based on r43961, since I couldn't compile the lastest svn version of webkitgtk, see https://bugs.webkit.org/show_bug.cgi?id=25910

besides, sorry for the last 2 failed patches, this one should work
Comment 9 Jan Alonzo 2009-05-22 20:40:23 PDT
Comment on attachment 30535 [details]
added an option to enable GNOME-specific features of libsoup, default=no

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 43961)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> +2009-05-21  WANG Lu <coolwanglu@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@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!
Comment 10 Wang Lu 2009-05-30 23:23:29 PDT
Created attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no
Comment 11 Jan Alonzo 2009-06-05 20:32:18 PDT
Comment on attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no

r=me
Comment 12 Jan Alonzo 2009-06-05 22:20:59 PDT
Landed in r44479. 
Comment 13 Wang Lu 2009-06-06 00:44:32 PDT
Thanks a lot!

(In reply to comment #12)
> Landed in r44479. 
> 

Comment 14 Xan Lopez 2009-06-07 10:54:02 PDT
I think this patch should be reverted, please see bug #26242 for rationale.
Comment 15 Jan Alonzo 2009-06-07 15:55:37 PDT
(In reply to comment #14)
> I think this patch should be reverted, please see bug #26242 for rationale.

This currently defaults to 'no' so only people who want it can have it. Let's discuss better ways to handle optional features in the future in bug #26242, and fix whatever is needed to be fixed in that bug. I rather have people behind a proxy can still browse the web using webkitgtk today instead of waiting for a concensus on #26242 (we may hit a road block and decide to recommit this patch again, who knows). 

Can I close this bug again and track whatever needs to be fixed in bug #26242?
Comment 16 Xan Lopez 2009-06-07 22:57:47 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > I think this patch should be reverted, please see bug #26242 for rationale.
> 
> This currently defaults to 'no' so only people who want it can have it. Let's
> discuss better ways to handle optional features in the future in bug #26242,
> and fix whatever is needed to be fixed in that bug. I rather have people behind
> a proxy can still browse the web using webkitgtk today instead of waiting for a
> concensus on #26242 (we may hit a road block and decide to recommit this patch
> again, who knows).

I think this is ignoring the main point of the issue: the 'people' we care about here are distros, and they will have to make a choice if we make a release with this option. That will only add a new variable to the equation and make things worse, when it seems that we already have a consensus that what we are doing is wrong.

Besides, I believe this feature request in particular is slighltly artificial. I don't think GtkLauncher is a good use case for almost anything, and the WebKit apps that provide the hability to navigate the Web can trivially use the GNOME proxy settings if they want to (Epiphany is, for example). So it's not like reverting this patch will prevent people from using proxies, it will only put us back where we were before, when they have to manually request it in their applications.


Comment 17 Jan Alonzo 2009-06-07 23:30:53 PDT
(In reply to comment #16)
> 
> I think this is ignoring the main point of the issue: the 'people' we care
> about here are distros, and they will have to make a choice if we make a
> release with this option. That will only add a new variable to the equation and
> make things worse, when it seems that we already have a consensus that what we
> are doing is wrong.

Honestly, I wasn't aware of any concensus. I was assuming you're all OK with this since there were no comments since the patched was marked as r?
> 
> Besides, I believe this feature request in particular is slighltly artificial.
> I don't think GtkLauncher is a good use case for almost anything, and the
> WebKit apps that provide the hability to navigate the Web can trivially use the
> GNOME proxy settings if they want to (Epiphany is, for example). So it's not

How about mobile platforms that want to use WebKitGtk? Or users that don't want GNOME but want to use WebKitGtk? 
Comment 18 Xan Lopez 2009-06-07 23:35:20 PDT
(In reply to comment #17)
> > Besides, I believe this feature request in particular is slighltly artificial.
> > I don't think GtkLauncher is a good use case for almost anything, and the
> > WebKit apps that provide the hability to navigate the Web can trivially use the
> > GNOME proxy settings if they want to (Epiphany is, for example). So it's not
> 
> How about mobile platforms that want to use WebKitGtk? Or users that don't want
> GNOME but want to use WebKitGtk? 
> 

I'm not sure I get what you mean. If they don't want to use GNOME I guess they won't have much use for a --with-libsoup-gnome option, no matter if it's in WebKit or elsewhere. The only thing I'm saying is that whoever was interested in this configure option in WebKit can trivially replicate the functionality in his own application, like Epiphany is doing. If you are not interested you don't have to do anything.

Comment 19 Jan Alonzo 2009-06-07 23:40:23 PDT
Comment on attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no

Clearing r+, setting it back to r?. Patch reverted in r44494 as suggested in this bug.
Comment 20 Xan Lopez 2009-06-10 12:09:13 PDT
Comment on attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no

I think we can clear the review flag here for now?
Comment 21 Christian Dywan 2009-08-13 16:03:47 PDT
I think this is overkill. Enabling proxy server support or all GNOME features is a one-liner per GNOME application. And in my opinion, libsoup should have a SOUP_TYPE_PROXY that uses whatever is available without requiring everything at build time. Or something that takes a string argument.
Comment 22 Christian Dywan 2009-12-20 02:42:42 PST
Concluding it is a one-liner to add the GNOME proxy resolver to GNOME applications, we shouldn't do that in WebKitGTK+. And http_proxy can be handled in libsoup, I filed https://bugzilla.gnome.org/show_bug.cgi?id=605048 for that. So this is resolved as far as WebKitGTK+ is concerned.