[webkit-reviews] review denied: [Bug 22624] [SOUP][GTK] Need API to get SoupSession from WebKit. : [Attachment 27002] API to retrieve and set soup sessions #8

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Christian Dywan
<christian at imendio.com>'s request for review:
Bug 22624: [SOUP][GTK] Need API to get SoupSession from WebKit.
https://bugs.webkit.org/show_bug.cgi?id=22624

Attachment 27002: API to retrieve and set soup sessions #8
https://bugs.webkit.org/attachment.cgi?id=27002&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
This patch appears to do a bunch of unrelated things:  removing the option to
build with curl, reworking some plumbing in WebCore, adding API in WebKit, and
making an apparently unrelated change in GtkLauncher. Without ChangeLog entries
its also rather hard to follow what's going on.  Can you please split out the
unrelated bits and add ChangeLog entries so it's easier to follow?

I have a few minor comments.

> diff --git a/WebCore/loader/FrameLoaderClient.h
b/WebCore/loader/FrameLoaderClient.h
> index b90cecd..2f1c41a 100644
> --- a/WebCore/loader/FrameLoaderClient.h
> +++ b/WebCore/loader/FrameLoaderClient.h
> @@ -35,6 +35,10 @@
>  #include <wtf/Platform.h>
>  #include <wtf/Vector.h>
>  
> +#if PLATFORM(GTK)
> +#include <libsoup/soup.h>
> +#endif
> +

You should forward-declare SoupSession rather than adding an #include to the
header.

> diff --git a/WebCore/platform/network/ResourceHandle.h
b/WebCore/platform/network/ResourceHandle.h
> index c93af21..515e0ee 100644
> --- a/WebCore/platform/network/ResourceHandle.h
> +++ b/WebCore/platform/network/ResourceHandle.h
> @@ -30,6 +30,10 @@
>  #include "HTTPHeaderMap.h"
>  #include <wtf/OwnPtr.h>
>  
> +#if USE(SOUP)
> +#include <libsoup/soup.h>
> +#endif

Same here.

> diff --git a/WebCore/platform/network/soup/CookieJarSoup.cpp
b/WebCore/platform/network/soup/CookieJarSoup.cpp
> index 88109e8..e647e21 100644
> --- a/WebCore/platform/network/soup/CookieJarSoup.cpp
> +++ b/WebCore/platform/network/soup/CookieJarSoup.cpp
> @@ -22,18 +22,28 @@
>  
>  #include "CString.h"
>  #include "KURL.h"
> +#include "Frame.h"
> +#include "FrameLoader.h"
> +#include "FrameLoaderClient.h"

Alphabetical order.

>  
>  namespace WebCore {
>  
> -SoupCookieJar* getCookieJar()
> +SoupCookieJar* getCookieJar(const Document* document)
>  {
> -    static SoupCookieJar* jar = soup_cookie_jar_new();
> +    Frame *frame = document->frame();
> +    if (!frame)
> +	   return 0;
> +    FrameLoader *loader = frame->loader();
> +    if (!loader)
> +	   return 0;
> +    SoupSession* session =
static_cast<FrameLoaderClient*>(loader->client())->session();
> +    SoupCookieJar* jar = (SoupCookieJar*)soup_session_get_feature(session,
SOUP_TYPE_COOKIE_JAR);
>      return jar;
>  }

The local "jar" variable doesn't seem necessary now.  The C-style cast should
be upgraded to a C++-style cast.  Is the cast on the result of loader->client()
necessary?  It doesn't seem like it should be, and you've added it in a few
different places.

> @@ -173,6 +173,8 @@ static void finishedCallback(SoupSession *session,
SoupMessage* msg, gpointer da
>	   return;
>  
>      ResourceHandleInternal* d = handle->getInternal();
> +    g_object_unref(d->m_session);
> +    d->m_session = NULL;

0 rather than NULL.

> +    SoupCookieJar* jar;
> +    const char* webkit_debug = g_getenv("WEBKIT_DEBUG");

"jar" should be declared when it is first initialised.	webkit_debug doesn't
follow our naming conventions.

> -	   const char* soup_debug = g_getenv("WEBKIT_SOUP_LOGGING");
> -	   if (soup_debug) {
> -	       int soup_debug_level = atoi(soup_debug);
> +    jar = (SoupCookieJar*)soup_session_get_feature(session,
SOUP_TYPE_COOKIE_JAR);

C++-style cast.

> +bool ResourceHandle::startHttp(SoupSession* session, String urlString)
> +{
> +    if(!session)
> +	   return false;

Need a space after "if".

> diff --git a/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h
b/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h
> index 299d023..ea54bf7 100644
> --- a/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h
> +++ b/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h
> @@ -22,6 +22,7 @@
>  
>  #include "ChromeClient.h"
>  #include "KURL.h"
> +#include <libsoup/soup.h>

Can just forward-declare SoupSession instead.

> +SoupSession* webkit_web_view_get_session (WebKitWebView* webView)
> +{
> +    SoupSession* session;
> +
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);
> +
> +    session = ResourceHandle::getSession(core(webView->priv->mainFrame));
> +    if (!session) {
> +	   session = soup_session_async_new ();
> +	   ResourceHandle::setSession(core(webView->priv->mainFrame), session);

> +    }

Why does it make sense to create a new session and set it when retrieving a
session?  Is there a reason that this doesn't reuse the same session as
ResourceHandle::getDefaultSession?

> diff --git a/WebKit/gtk/webkit/webkitwebview.h
b/WebKit/gtk/webkit/webkitwebview.h
> index 2bb8c61..1a9bf58 100644
> --- a/WebKit/gtk/webkit/webkitwebview.h
> +++ b/WebKit/gtk/webkit/webkitwebview.h
> @@ -29,6 +29,7 @@
>  #include <webkit/webkitwebbackforwardlist.h>
>  #include <webkit/webkitwebhistoryitem.h>
>  #include <webkit/webkitwebsettings.h>
> +#include <libsoup/soup.h>

Forward-declare rather than include?