Bug 42721 – [EFL] Set proxy address for Soup

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

If PC network needs to use proxy address, EWebLauncher cannot use network without proxy address setting.

[Gyuyoung Kim]

Created attachment 62145 [details]
Patch

Patch

[Lucas De Marchi]

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

[Lucas De Marchi]

More or less what QT guys did: bug 27495

[Gyuyoung Kim]

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 ?

[Lucas De Marchi]

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

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 62268 [details]
Patch

Oops, sorry. Other patch was uploaded. This is new patch.

[Lucas De Marchi]

Comment on attachment 62268 [details]
Patch

> Index: WebKit/efl/ChangeLog
> ===================================================================
> --- WebKit/efl/ChangeLog	(revision 63877)
> +++ WebKit/efl/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Gyuyoung Kim  <[e-mail]>
> +
> +        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);

[Lucas De Marchi]

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)

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Hello

[Kenneth Rohde Christiansen]

Comment on attachment 62290 [details]
Patch

Looks fine to me. If leandro is ok with this, please cq+

[Leandro Pereira]

Comment on attachment 62290 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 62290 [details]
Patch

Clearing flags on attachment: 62290

Committed r63972: <http://trac.webkit.org/changeset/63972>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.