Bug 22624 - [SOUP][GTK] Need API to get SoupSession from WebKit.
Summary: [SOUP][GTK] Need API to get SoupSession from WebKit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Christian Dywan
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2008-12-03 05:26 PST by Xan Lopez
Modified: 2010-08-20 13:13 PDT (History)
11 users (show)

See Also:


Attachments
Trivial example of how to do this. (3.44 KB, patch)
2008-12-04 13:10 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
0001-Add-API-to-get-the-internal-SoupSession-used-by-web.patch (8.30 KB, patch)
2008-12-21 10:42 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
API to retrieve the soup session from the view (14.05 KB, patch)
2008-12-23 15:26 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
API to retrieve the soup session from the view #2 (13.62 KB, patch)
2008-12-24 11:55 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
API to retrieve the soup session from the view #3 (20.43 KB, patch)
2008-12-28 16:43 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
API to retrieve and set soup sessions #4 (22.89 KB, patch)
2009-01-14 16:36 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
API to retrieve and set soup sessions #5 (23.51 KB, patch)
2009-01-14 18:29 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
API to retrieve and set soup sessions #6 (23.65 KB, patch)
2009-01-18 14:43 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
API to retrieve and set soup sessions #7 (24.26 KB, patch)
2009-01-24 08:37 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
API to retrieve and set soup sessions #8 (24.50 KB, patch)
2009-01-24 16:29 PST, Christian Dywan
mrowe: review-
Details | Formatted Diff | Diff
removecurl.patch (8.98 KB, patch)
2009-02-19 02:52 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
removecurl.patch (9.02 KB, patch)
2009-02-19 03:38 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
getsession.patch (12.61 KB, patch)
2009-02-19 03:44 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
getsession.patch (11.15 KB, patch)
2009-02-19 06:38 PST, Xan Lopez
ap: review-
Details | Formatted Diff | Diff
removecurl.patch (9.01 KB, patch)
2009-02-23 01:48 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
defaultsession.patch (12.28 KB, patch)
2009-02-23 01:48 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
defaultsession2.patch (12.36 KB, patch)
2009-02-23 02:08 PST, Xan Lopez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2008-12-03 05:26:07 PST
Soup trunk (2.25.x) has support for persistent cookies now. We need to add some API/code in order to use that functionality in a WebKit browser:

- First of all, we need a way to get the SoupSession created internally in WebKit, as that it's the entry point for most actions in libsoup (ie, you need that to see all the cookie data, for example).
- We need a way to let the user select which backend will be used to persist the cookies, the existing options at the time of writing being txt and sqlite.
- We need a way to let the user select where will the cookies file be stored.

For the first problem the most straightforward solution is to define a soup-only getSession() method in ResourceHandle.h, and just use that to return the session in a public webkit function (webkit_get_soup_session()).

There's many ways to fix the configurability issues. For the backend selection we could add API to define the SoupSessionFeatures to use, which would also serve for future bugs/functionality (proxy, http cache, ...).

To set the directory where we want to store our data I guess a reasonable solution would be to use the XDG Base Directory as default optionally allowing to override it via an environment variable?

I have a patch that implements the get_session function hardcoding everything else that I used for testing, I can attach it here if anyone thinks it would be helpful.
Comment 1 Xan Lopez 2008-12-04 13:10:02 PST
Created attachment 25749 [details]
Trivial example of how to do this.

The mentioned hack I used for testing. Everything is hardcoded and there's actually a bug: we need to create the session if it does not exist in the get method.
Comment 2 Xan Lopez 2008-12-21 10:42:58 PST
Created attachment 26185 [details]
0001-Add-API-to-get-the-internal-SoupSession-used-by-web.patch
Comment 3 Christian Dywan 2008-12-23 15:26:05 PST
Created attachment 26230 [details]
API to retrieve the soup session from the view

Based on the previous patch: I made libsoup a requirement by removing the HTTP backend concept. I changed webkit_web_view_get_session so that it requires a valid view, with regard to language bindings we can't work with a NULL view. And I bumped the required libsoup version and used the new soup_session_get_feature found in yesterday's libsoup release.

I started a discussion on the WebKit mailing list with regard to always using libsoup in the Gtk port, and so far the response seems to be positive.
Comment 4 Holger Freyther 2008-12-24 07:38:00 PST
Comment on attachment 26230 [details]
API to retrieve the soup session from the view


> +static SoupCookieJar* cookie_jar = NULL;

No null, please use 0 which will be turned into a null pointer.


>  SoupCookieJar* getCookieJar()
>  {
> -    static SoupCookieJar* jar = soup_cookie_jar_new();
> -    return jar;
> +    if (cookie_jar == NULL) {
> +        cookie_jar = soup_cookie_jar_new();
> +    }


> +void setCookieJar(SoupCookieJar *jar)
> +{
> +    if (cookie_jar == NULL) {
> +        cookie_jar = jar;
> +    }
>  }

According to our coding style we do not use '{' in these two cases.



> +static void soupSessionInitalize()
> +{
> +    SoupSessionFeature *jar = 0;
> +    const char* soup_debug = g_getenv("WEBKIT_SOUP_LOGGING");

It would be cool if we could unite this with the LogNetworking WTFLogChannel (WebCore/platform/Logging.h)



> +
> +    jar = soup_session_get_feature(session, SOUP_TYPE_COOKIE_JAR);
> +    if (!jar) {
>          soup_session_add_feature(session, SOUP_SESSION_FEATURE(getCookieJar()));
> +    } else {
> +        setCookieJar(SOUP_COOKIE_JAR(g_object_ref(jar)));
> +    }

Coding Style :)


>      WebKitWebViewPrivate* priv = webView->priv;
>      if (priv->zoomFullContent == zoomFullContent)
> -      return;
> +        return;

right, not for this patch though.

>  
>      priv->zoomFullContent = zoomFullContent;
>      webkit_web_view_apply_zoom_level(webView, webkit_web_view_get_zoom_level(webView));
> @@ -2593,4 +2612,24 @@ void webkit_web_view_set_full_content_zoom(WebKitWebView* webView, gboolean zoom
>      g_object_notify(G_OBJECT(webView), "full-content-zoom");
>  }
>  
> +/**
> + * webkit_web_view_get_session

I think you need a colon here for gtk-doc










General comments:
  - Coding Style, sepcially brackets on the if/else :)
  - on QtWebKit we allow to have something like a SoupSession per WebView... would that make sense as well? E.g. sometimes you do not want two different WebView to share the same cookie pool? If we want that the WebView should hold a SoupSession and there should be one default session so by default every WebView is sharing this session?
Comment 5 Christian Dywan 2008-12-24 11:55:55 PST
Created attachment 26240 [details]
API to retrieve the soup session from the view #2

(In reply to comment #4)
> (From update of attachment 26230 [details] [review])
> General comments:
>   - Coding Style, sepcially brackets on the if/else :)
Updated to correct the coding style.

>   - on QtWebKit we allow to have something like a SoupSession per WebView...
> would that make sense as well? E.g. sometimes you do not want two different
> WebView to share the same cookie pool? If we want that the WebView should hold
> a SoupSession and there should be one default session so by default every
> WebView is sharing this session?
That is actually the plan, and we discussed this a bit already. We figured it would be good to allow for it in the API, by having a view specific function, and in a second step add the logic for individual sessions and another function.

> It would be cool if we could unite this with the LogNetworking WTFLogChannel
> (WebCore/platform/Logging.h)
I changed the logging part to look for WEBKIT_DEBUG with a value of 'network', similar to GTK_DEBUG or G_DEBUG. It seems like a good idea to integrate it with WebCore's logging, however probably not in this same patch.
Comment 6 Holger Freyther 2008-12-24 14:05:42 PST
(In reply to comment #5)
> Created an attachment (id=26240) [review]
> API to retrieve the soup session from the view #2
> 
> (In reply to comment #4)
> > (From update of attachment 26230 [details] [review] [review])
> > General comments:
> >   - Coding Style, sepcially brackets on the if/else :)
> Updated to correct the coding style.
> 
> >   - on QtWebKit we allow to have something like a SoupSession per WebView...
> > would that make sense as well? E.g. sometimes you do not want two different
> > WebView to share the same cookie pool? If we want that the WebView should hold
> > a SoupSession and there should be one default session so by default every
> > WebView is sharing this session?
> That is actually the plan, and we discussed this a bit already. We figured it
> would be good to allow for it in the API, by having a view specific function,
> and in a second step add the logic for individual sessions and another
> function.


For the QtWebKit port we have added the necessary Document* pointers to be able to get the QNetworkManager of the the QWebView. What I probably tried to say/encourage is that newly created code should try to get the SoupSession through the (Document->ChromeClient->WebKitWebPage). Whenever we decide to allow to set a per session SoupSession we only have to change WebKitWebPage and the rest will be fine. I think it should be little work to do that right from the start.
Comment 7 Christian Dywan 2008-12-28 16:43:55 PST
Created attachment 26286 [details]
API to retrieve the soup session from the view #3

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=26240) [review] [review]
> > API to retrieve the soup session from the view #2
> For the QtWebKit port we have added the necessary Document* pointers to be
> able to get the QNetworkManager of the the QWebView. What I probably tried to
> say/ encourage is that newly created code should try to get the SoupSession
> through the (Document->ChromeClient->WebKitWebPage). Whenever we decide to
> allow to set a per session SoupSession we only have to change WebKitWebPage
> and the rest will be fine. I think it should be little work to do that right > from the start.

I took an inspirative look at what Qt does and ended up adding a FrameLoaderClient::session() function, giving ResourceHandle::Session a Frame* argument and replacing global jar and session variables with Document* or Session* arguments. FrameLoaderClient seemed the right place for a network session.
I don't know if this is the best approach, particularly I'm not sure where the initial session should be set from and if it's good that WebKit now always creates a cookie jar if there is none.
Comment 8 Xan Lopez 2009-01-12 17:51:08 PST
Some points from a discussion on IRC between Christian, Gustavo and me:

It seems to us the following API should be enough to cover all use cases for global an per-view soup sessions.

- SoupSession* webkit_get_global_session()

Returns the global session used by all views by default.

- SoupSession* webkit_web_view_get_session (WebKitWebView *view)

Returns a private soup session for the view, created on demand the first time it's requested. The view will use that session from now on.

- void webkit_web_view_set_session (WebKitWebView *view, SoupSession *session)

Makes view use the given session from now on. Useful for: reset views to use the global session, create groups of "private" views all using the same session different from the global one.

The documentation should probably mention that creating sessions from scratch and setting them on views shouldn't be done unless you really know what you are doing. It should also probably say all WebKit-GTK soup sessions have to be async (and _set_session should check the type of the session given to make sure of this).
Comment 9 Christian Dywan 2009-01-14 16:36:52 PST
Created attachment 26736 [details]
API to retrieve and set soup sessions #4

Revised patch, including webkit_get_default_session and webkit_web_view_set_session now. I haven't tested it very much. There seem to be a race condition which leads to warnings with soup_session_get_feature.

Wrt webkit_get_global_session Vs. webkit_get_default_session, I think the _default_ naming is pretty common, if you think of GdkScreen, GtkSettings and such. Arguments in favour or against either are welcome.
Comment 10 Christian Dywan 2009-01-14 18:29:14 PST
Created attachment 26741 [details]
API to retrieve and set soup sessions #5

Small correction, "session" is writable and webkit_web_view_set_session is actually defined in the header.
Comment 11 Xan Lopez 2009-01-15 06:00:16 PST
(In reply to comment #10)
> Created an attachment (id=26741) [review]
> API to retrieve and set soup sessions #5
> 
> Small correction, "session" is writable and webkit_web_view_set_session is
> actually defined in the header.
> 

Looks pretty good to me.

+#if PLATFORM(GTK)
+        virtual SoupSession* session() = 0;
+	virtual void setSession(SoupSession*) = 0;
+#endif

Indentation.

+    if (!jar) {
+        jar = soup_cookie_jar_new();
+        soup_session_add_feature(session, SOUP_SESSION_FEATURE(jar));
+        g_object_unref(G_OBJECT(jar));
+    }

g_object_unref takes a gpointer.

+SoupSession* ResourceHandle::getSession(const Frame *frame)
+{
+    static SoupSession* default_session = soup_session_async_new();
+
+    if (!frame)
+        return default_session;
+    FrameLoader *loader = frame->loader();
+    if (!loader)
+        return 0;

Why not return the default session here? If this is some kind of catastrophic error situation maybe we should assert or something...

+    SoupSession* session = static_cast<FrameLoaderClient*>(loader->client())->session();
+    return session ? session : default_session;
+}

Also, just wondering in general, if the sessions are async maybe some checks wold be in order when swapping them (making sure they have no pending work?). Maybe that's related to your error...

And as a soptimization, I think a flag saying the session is initialized instead of checking for the features every time we go through startHttp would be good (I think I had this in previous patches).
Comment 12 Christian Dywan 2009-01-18 14:43:23 PST
Created attachment 26836 [details]
API to retrieve and set soup sessions #6

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=26741) [review] [review]
> > API to retrieve and set soup sessions #5
> +SoupSession* ResourceHandle::getSession(const Frame *frame)
> +{
> +    static SoupSession* default_session = soup_session_async_new();
> +
> +    if (!frame)
> +        return default_session;
> +    FrameLoader *loader = frame->loader();
> +    if (!loader)
> +        return 0;
> 
> Why not return the default session here? If this is some kind of catastrophic
> error situation maybe we should assert or something...

There is no catstrohpe, but I acknowledge you might think so if you don't see it in the full context. In fact I defined getSession(0) to return default session. Your question lead me to separate this into a function getDefaultSession, to avoid confusion.

I think we must not assert actually, in case the view is being used while the object is finalized. Nothing harmful should happen in any case.

> +    SoupSession* session =
> static_cast<FrameLoaderClient*>(loader->client())->session();
> +    return session ? session : default_session;
> +}
> 
> Also, just wondering in general, if the sessions are async maybe some checks
> wold be in order when swapping them (making sure they have no pending work?).
> Maybe that's related to your error...

My error came during browsing while keeping the default session, and while using a non-default session, so it doesn't look like swapping is the problem. I added an if(!session) to ensureSession now, and it looks like it prevents that wanring. I'm still unsure however under what condition startHttp would have an empty session...

> And as an optimization, I think a flag saying the session is initialized
> instead of checking for the features every time we go through startHttp would
> be good (I think I had this in previous patches).

I wonder about that, I was thinking you intended to make it dynamic later on. Do you think the feature test is that slow? If we agree on which we prefer and how to document that API wise, I can change it accordingly.
Comment 13 Xan Lopez 2009-01-19 17:49:11 PST
(In reply to comment #12)
> > +    SoupSession* session =
> > static_cast<FrameLoaderClient*>(loader->client())->session();
> > +    return session ? session : default_session;
> > +}
> > 
> > Also, just wondering in general, if the sessions are async maybe some checks
> > wold be in order when swapping them (making sure they have no pending work?).
> > Maybe that's related to your error...
> 
> My error came during browsing while keeping the default session, and while
> using a non-default session, so it doesn't look like swapping is the problem. I
> added an if(!session) to ensureSession now, and it looks like it prevents that
> wanring. I'm still unsure however under what condition startHttp would have an
> empty session...

That still does not cover changing the session for a view while the view is using it. It seems to me that the way it's done in the patch can't be too safe. At the very least we should check the queued messages and cancel them...?

> 
> > And as an optimization, I think a flag saying the session is initialized
> > instead of checking for the features every time we go through startHttp would
> > be good (I think I had this in previous patches).
> 
> I wonder about that, I was thinking you intended to make it dynamic later on.
> Do you think the feature test is that slow? If we agree on which we prefer and
> how to document that API wise, I can change it accordingly.
> 

It might be a soptimization, but I think it's pretty sane to just set a flag after you are done (and reset it when we swap sessions) so you just don't check the features over and over again...
Comment 14 Xan Lopez 2009-01-20 14:59:46 PST
A couple more things before I forget:

- I think the FrameLoaderClient should ref its session.
- On the topic of "safe swapping", it seems that at least we should cancel all queued messages in the session before replacing it.
Comment 15 Christian Dywan 2009-01-24 08:37:11 PST
Created attachment 26996 [details]
API to retrieve and set soup sessions #7

Changed to address the last comments.

The frame loader does keep a reference to the session now, I completely forgot that.

The session has a flag indicating whether it was initialized, similar to before, but the flag is on the actual message since sessions can be shuffled around and we only want to fill it once.

Setting the session *stops* the loader, I hope it's the right way of cancelling pending loads when exchanging the session. I did not cancel all messages on the session because the session might be used for other frames independently.
Comment 16 Xan Lopez 2009-01-24 15:58:09 PST
Looks pretty good to me with these changes. Maybe one nitpick would be to print a human readable message in webkit_web_view_set_session when the session is not async, something along the lines: "You are trying to set a sync session for the view to use, but webkit needs async sessions in order to work properly".
Comment 17 Christian Dywan 2009-01-24 16:29:00 PST
Created attachment 27002 [details]
API to retrieve and set soup sessions #8

Updated patch, including a verbose message when trying to set a non-async session as Xan suggested. I also added a comment to the function documentation that WebKit only supports SoupSessionAsync.
Comment 18 Mark Rowe (bdash) 2009-01-30 05:49:40 PST
Comment on attachment 27002 [details]
API to retrieve and set soup sessions #8

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?
Comment 19 Xan Lopez 2009-01-30 05:56:56 PST
(In reply to comment #18)
> > +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?
> 

The idea is to allow individual views to use their own private sessions instead of the default/global one (think of privacy mode or similar). We decided to do this by creating them on demand when the user asks for the session of an individual view.
Comment 20 Mark Rowe (bdash) 2009-01-30 06:07:14 PST
> > 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?
> > 
> 
> The idea is to allow individual views to use their own private sessions instead
> of the default/global one (think of privacy mode or similar). We decided to do
> this by creating them on demand when the user asks for the session of an
> individual view.

That seems like it could be confusing.  Calling setSession will stop a load that is in progress, which means that calling this "get" API at the wrong time could result in some rather odd behaviour.
Comment 21 Mark Rowe (bdash) 2009-01-30 06:09:41 PST
I just noticed that the API is added to the WebView, but the sessions appears to only ever be set on the main frame's loader.  Is that really the behaviour that you're after?
Comment 22 Xan Lopez 2009-01-30 06:13:53 PST
(In reply to comment #20)
> > The idea is to allow individual views to use their own private sessions instead
> > of the default/global one (think of privacy mode or similar). We decided to do
> > this by creating them on demand when the user asks for the session of an
> > individual view.
> 
> That seems like it could be confusing.  Calling setSession will stop a load
> that is in progress, which means that calling this "get" API at the wrong time
> could result in some rather odd behaviour.
> 

Well, I don't see a way around this. If you provide this functionality the programmer will always be able to replace the session while it's being used. But he should know what he's doing.
Comment 23 Mark Rowe (bdash) 2009-01-30 06:16:55 PST
> > That seems like it could be confusing.  Calling setSession will stop a load
> > that is in progress, which means that calling this "get" API at the wrong time
> > could result in some rather odd behaviour.
> > 
> 
> Well, I don't see a way around this. If you provide this functionality the
> programmer will always be able to replace the session while it's being used.
> But he should know what he's doing.

It makes sense for webkit_web_view_set_session to potentially have this sort of effect, but it doesn't make sense to me that a getter would be a potentially destructive operation.
Comment 24 Xan Lopez 2009-01-30 06:21:52 PST
(In reply to comment #23)
> > Well, I don't see a way around this. If you provide this functionality the
> > programmer will always be able to replace the session while it's being used.
> > But he should know what he's doing.
> 
> It makes sense for webkit_web_view_set_session to potentially have this sort of
> effect, but it doesn't make sense to me that a getter would be a potentially
> destructive operation.
> 

Right. The idea here is that you shouldn't need to create sessions outside of webkit. An slightly less magical alternative would be...

session = webkit_create_soup_session ();
webkit_web_view_set_session (view, session);

Comment 25 Christian Dywan 2009-02-12 09:44:27 PST
(In reply to comment #24)
> (In reply to comment #23)
> > > Well, I don't see a way around this. If you provide this functionality the
> > > programmer will always be able to replace the session while it's being used.
> > > But he should know what he's doing.
> > 
> > It makes sense for webkit_web_view_set_session to potentially have this sort of
> > effect, but it doesn't make sense to me that a getter would be a potentially
> > destructive operation.
> > 
> 
> Right. The idea here is that you shouldn't need to create sessions outside of
> webkit. An slightly less magical alternative would be...
> 
> session = webkit_create_soup_session ();
> webkit_web_view_set_session (view, session);

I understand the concerns. And I'm fine if we agree to make it explicit, without any 'magic'. At this point I wonder if we should just recommend using any async session and leave out the webkit_create_soup_session function. At least currently it wouldn't do more than a non-persistent cookie jar. That *might* be different if we add more features, not sure.
Comment 26 Xan Lopez 2009-02-19 02:52:29 PST
Created attachment 27783 [details]
removecurl.patch

Trying to do this piece by piece now. This patch removes the CURL backend support from WebKit/GTK+.
Comment 27 Xan Lopez 2009-02-19 03:38:31 PST
Created attachment 27787 [details]
removecurl.patch

Remove unneeded LIBS flag in WebKitTools.
Comment 28 Xan Lopez 2009-02-19 03:44:54 PST
Created attachment 27788 [details]
getsession.patch

Add API to get the default soup session.
Comment 29 Christian Dywan 2009-02-19 03:52:53 PST
(In reply to comment #28)
> Created an attachment (id=27788) [review]
> getsession.patch
> 
> Add API to get the default soup session.

"so if you insert for your own #SoupCookieJar before any network"

should be

"so if you insert your own #SoupCookieJar before any network"

And you seem to have forgotten the webkit_web_view_get_session() function

Otherwise nice, let's get this cool stuff in :)
Comment 30 Xan Lopez 2009-02-19 06:38:38 PST
Created attachment 27790 [details]
getsession.patch

Remove the session property and the view-specific API after some discussion on IRC. They'll come in the next patches, where they'll actually do something relevant.
Comment 31 Alexey Proskuryakov 2009-02-23 00:33:22 PST
Comment on attachment 27790 [details]
getsession.patch

>         WARNING: NO TEST CASES ADDED OR CHANGED

This line is just a reminder to the author of the patch, please remove it (perhaps explaining why test cases aren't needed or possible).

> +#if USE(SOUP)
> +#include <libsoup/soup.h>
> +#endif

Could you just forward declare SoupSession?

> + *  Copyright (C) 2009 Igalia S.L., Author: Xan Lopez <xlopez@igalia.com>

This isn't a usual form of copyright line. I don't know if it's a real problem, but it certainly makes me nervous.

> +static void ensureSession(SoupSession* session)

This name doesn't seem descriptive enough to me. What does this function do with the session?

> +    if (soup_session_get_feature(session, SOUP_TYPE_LOGGER) == NULL

Per coding style, this should be "if (!soup_session_get_feature(session, SOUP_TYPE_LOGGER)"

> +bool ResourceHandle::startHttp(String urlString)
> +{
> +    if (!session)
> +        createSoupSession();

This is what getSession() does - could you just call it? In this case, the static variable could just move to getSession().

> +SoupSession* ResourceHandle::getSession()

The names "getSession" and "getCookieJar" are misleading, and don't follow out usual style. First, a getXXX function is one that returns its result in an argument, a getter would be just "cookieJar", for example. More importantly, these functions aren't getters, as they can create objects. I think that the usual naming would be something like "sharedSession", "commonSession", "defaultSession" etc.

> - *  Copyright (C) 2007, 2008 Christian Dywan <christian@imendio.com>
> + *  Copyright (C) 2007-2009 Christian Dywan <christian@imendio.com>

Again, I don't know why, but we list all years comma-separated in copyright lines.

Looks like this needs another round of review.
Comment 32 Xan Lopez 2009-02-23 01:48:04 PST
Created attachment 27873 [details]
removecurl.patch
Comment 33 Xan Lopez 2009-02-23 01:48:31 PST
Created attachment 27874 [details]
defaultsession.patch

Address ap's review points.
Comment 34 Alexey Proskuryakov 2009-02-23 02:07:23 PST
Comment on attachment 27873 [details]
removecurl.patch

r=me
Comment 35 Xan Lopez 2009-02-23 02:08:11 PST
Created attachment 27875 [details]
defaultsession2.patch

Do not recreate the cookie jar all the time in defaultCookieJar, as it can be set to NULL by the user.
Comment 36 Alexey Proskuryakov 2009-02-23 02:12:12 PST
Comment on attachment 27875 [details]
defaultsession2.patch

r=me 

> +    static SoupSession* session = createSoupSession();;

That's a bit too many semicolons!
Comment 37 Christian Dywan 2009-02-23 03:28:02 PST
Comment on attachment 27873 [details]
removecurl.patch

Remove CURL support in the Gtk port, the HTTP backend is SOUP now.
    
    https://bugs.webkit.org/show_bug.cgi?id=22624
    [SOUP][GTK] Need API to get SoupSession from WebKit.
Comment 38 Christian Dywan 2009-02-23 03:28:51 PST
Comment on attachment 27875 [details]
defaultsession2.patch

Allow retrieving the Soup session and modify the code to take
    into account users changing features on it.
    
    https://bugs.webkit.org/show_bug.cgi?id=22624
    [SOUP][GTK] Need API to get SoupSession from WebKit.
Comment 39 Martin Robinson 2010-08-20 13:13:51 PDT
This issue seems resolved. Feel free to re-open if not.