Summary: | [Gtk] WebKit GTK with libsoup won't recognize proxies | ||
---|---|---|---|
Product: | WebKit | Reporter: | Wang Lu <coolwanglu> |
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED WONTFIX | ||
Severity: | Normal | CC: | christian, gustavo, jmalonzo, xan.lopez |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | Linux | ||
Attachments: |
Description
Wang Lu
2009-04-17 02:07:48 PDT
Created attachment 29573 [details] patch for enable proxy in libsoup based on Webkit-r42569 (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, 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
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 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. 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 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 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 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! Created attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no
Comment on attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no
r=me
Landed in r44479. Thanks a lot! (In reply to comment #12) > Landed in r44479. > I think this patch should be reverted, please see bug #26242 for rationale. (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? (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. (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? (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 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 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?
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. 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. |