|
Description
Xan Lopez
2008-12-03 05:26:07 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.
Created attachment 26185 [details]
0001-Add-API-to-get-the-internal-SoupSession-used-by-web.patch
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 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? 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. (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. 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. 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). 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.
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.
(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). 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. (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... 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. 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.
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". 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 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? (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. > > 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.
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? (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. > > 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.
(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); (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. Created attachment 27783 [details]
removecurl.patch
Trying to do this piece by piece now. This patch removes the CURL backend support from WebKit/GTK+.
Created attachment 27787 [details]
removecurl.patch
Remove unneeded LIBS flag in WebKitTools.
Created attachment 27788 [details]
getsession.patch
Add API to get the default soup session.
(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 :) 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 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. Created attachment 27873 [details]
removecurl.patch
Created attachment 27874 [details]
defaultsession.patch
Address ap's review points.
Comment on attachment 27873 [details]
removecurl.patch
r=me
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 on attachment 27875 [details] defaultsession2.patch r=me > + static SoupSession* session = createSoupSession();; That's a bit too many semicolons! 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 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. This issue seems resolved. Feel free to re-open if not. |