Bug 42721 - [EFL] Set proxy address for Soup
Summary: [EFL] Set proxy address for Soup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-20 19:38 PDT by Gyuyoung Kim
Modified: 2010-07-23 07:34 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.31 KB, patch)
2010-07-20 22:44 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (1.31 KB, patch)
2010-07-22 00:40 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2010-07-22 00:42 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.18 KB, patch)
2010-07-22 06:09 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2010-07-20 19:38:40 PDT
If PC network needs to use proxy address, EWebLauncher cannot use network without proxy address setting.
Comment 1 Gyuyoung Kim 2010-07-20 22:44:02 PDT
Created attachment 62145 [details]
Patch

Patch
Comment 2 Lucas De Marchi 2010-07-21 05:09:12 PDT
(In reply to comment #0)
> If PC network needs to use proxy address, EWebLauncher cannot use network without proxy address setting.

Nice, but this breaks when libsoub is not enabled (bug 42286, which did not get in yet because of a problem with changelogs).

I'd rather let the part of getting the environment variable to browser and add a method to set proxy. This way browser might decide for whatever reason to ask for a proxy to be used (be the environment var or because user explicitly configured browser for this). Then we would have 2 implementations surrounded by ifdefs, 1 for libsoup and another for libcurl (which can be let blank for now).
Comment 3 Lucas De Marchi 2010-07-21 05:14:39 PDT
More or less what QT guys did: bug 27495
Comment 4 Gyuyoung Kim 2010-07-21 05:27:15 PDT
Thank you lucas,

I understand your point as below, 

void ewk_XXX_proxy_uri_set(...)
{
#if USE(SOUP)
....
#elif USE(CURL)
....
#endif
}

And, EWebLauncher invokes the API.

In my opinion, this API is located in ewk_settings.cpp. How do you think about it ?
Comment 5 Lucas De Marchi 2010-07-21 06:28:41 PDT
(In reply to comment #4)
> Thank you lucas,
> 
> I understand your point as below, 
> 
> void ewk_XXX_proxy_uri_set(...)
> {
> #if USE(SOUP)
> ....
> #elif USE(CURL)
> ....
> #endif
> }
> 
> And, EWebLauncher invokes the API.
> 
> In my opinion, this API is located in ewk_settings.cpp. How do you think about it ?

I think ewk_settings.cpp is indeed the best place.
Comment 6 Gyuyoung Kim 2010-07-22 00:40:51 PDT
Created attachment 62267 [details]
Patch

I modify this patch according to Lucas's opinion. However, I don't support libcurl yet.
WebKit's Libcurl port already has proxy method. In order to use the function, proxy address needs to be modified. I will support it later.
Comment 7 Gyuyoung Kim 2010-07-22 00:42:26 PDT
Created attachment 62268 [details]
Patch

Oops, sorry. Other patch was uploaded. This is new patch.
Comment 8 Lucas De Marchi 2010-07-22 05:37:20 PDT
Comment on attachment 62268 [details]
Patch

> Index: WebKit/efl/ChangeLog
> ===================================================================
> --- WebKit/efl/ChangeLog	(revision 63877)
> +++ WebKit/efl/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> +2010-07-22  Gyuyoung Kim  <gyuyoung.kim@samsung.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [EFL] Set proxy address for Soup
> +        https://bugs.webkit.org/show_bug.cgi?id=42721
> +
> +        Set proxy address by http_proxy enviroment variable.

Now the description doesn't make much sense.

> Index: WebKit/efl/ewk/ewk_settings.cpp
> ===================================================================
> --- WebKit/efl/ewk/ewk_settings.cpp	(revision 63876)
> +++ WebKit/efl/ewk/ewk_settings.cpp	(working copy)
> @@ -29,6 +29,7 @@
>  #include "Image.h"
>  #include "IntSize.h"
>  #include "KURL.h"
> +#include "NotImplemented.h"

There's no other place inside WebKit/efl/ewk/ where we use notImplemented(). I'd rather let this out and use a warning in eina_log or an EINA_SAFETY_ON_TRUE_RETURN(1).



> +/**
> + * Sets proxy address.
> + *
> + * @param proxy address. If proxy address is null, proxy address isn't set.
> + */
> +void ewk_settings_proxy_uri_set(const char* proxy)
> +{
> +#ifdef WTF_USE_SOUP
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> +    g_object_set(session, SOUP_SESSION_PROXY_URI, soup_uri_new(proxy), NULL);

You are leaking the SoupURI struct here, since g_object_set always copies. Change to something like:

SoupURI* uri = soup_uri_new(proxy);
EINA_SAFETY_ON_NULL_RETURN(uri);

g_object_set(session, SOUP_SESSION_PROXY_URI, uri, NULL);
soup_uri_free(soupUri);
Comment 9 Lucas De Marchi 2010-07-22 05:43:13 PDT
Comment on attachment 62268 [details]
Patch

Not so important, but:

> +/**
> + * Sets proxy address.
> + *
> + * @param proxy address. If proxy address is null, proxy address isn't set.
> + */
> +void ewk_settings_proxy_uri_set(const char* proxy)
> +{
> +#ifdef WTF_USE_SOUP
#if USE(SOUP)

> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> +    g_object_set(session, SOUP_SESSION_PROXY_URI, soup_uri_new(proxy), NULL);
> +#elif defined WTF_USE_CURL
#elif USE(CURL)
Comment 10 Gyuyoung Kim 2010-07-22 06:09:27 PDT
Created attachment 62290 [details]
Patch

Hello lucas,

Thank you for your advice. 
I modify this patch. If there are still nits, please let me know.
Comment 11 Gyuyoung Kim 2010-07-22 19:58:24 PDT
Hello
Comment 12 Kenneth Rohde Christiansen 2010-07-23 05:05:16 PDT
Comment on attachment 62290 [details]
Patch

Looks fine to me. If leandro is ok with this, please cq+
Comment 13 Leandro Pereira 2010-07-23 06:43:29 PDT
Comment on attachment 62290 [details]
Patch

LGTM
Comment 14 WebKit Commit Bot 2010-07-23 07:34:41 PDT
Comment on attachment 62290 [details]
Patch

Clearing flags on attachment: 62290

Committed r63972: <http://trac.webkit.org/changeset/63972>
Comment 15 WebKit Commit Bot 2010-07-23 07:34:53 PDT
All reviewed patches have been landed.  Closing bug.