Bug 77341

Created attachment 124558 [details] Patch

Attachment 124558 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:447: Extra space between SoupSession and *session [whitespace/declaration] [3] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

This is a WIP patch supposed to carry on the multi-SoupSession work started in bug 22624. I'd like some feedback on the WebCore and GTK+ changes: I tried to keep the existing behavior as much as possible, so that the changes are transparent to users. The WebKit2 part has not been touched, as there currently does not seem to be an easy way to pass SoupSessions around between processes (and the CookieJar calls called by WK2 assume there is only one cookie jar). I'm not sure if there are ownership and lifetime issues in the way I'm using the SoupSession or WebKitWeb{Frame,View} pointers, so help in this part is also appreciated.

May I ask what is this change required for?

Comment on attachment 124558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124558&action=review Interesting. I'm not sure we would like to use that API for WebKitGTK+ though, we may have to discuss it a bit more, I think it's probably material for wk2 in any case - where we have the WebContext object which we could use to split sessions instead of having the API user assign sessions to each webview. > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:82 > - SoupCookieJar* jar = defaultCookieJar(); > + const NetworkingContext* context = networkingContext(document); > + if (!context) > + return; > + Shouldn't you use the default cookie jar in case no network context exists? It's a bit weird that you have a networkingContext method that you only use to obtain the cookie jar, I'd recommend going straight to the jar. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:163 > + else { > + if (session == ResourceHandle::defaultSession()) Make this an else if and do away with the braces =)

(In reply to comment #4) > May I ask what is this change required for? I'm presently doing some downstream work using webkit-efl in which I need different parts using the port to use different SoupSessions, so that they do not share cookie jars, custom soup request handlers, callbacks to SoupSession signals etc.

(In reply to comment #5) > (From update of attachment 124558 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124558&action=review > > Interesting. I'm not sure we would like to use that API for WebKitGTK+ though, we may have to discuss it a bit more, I think it's probably material for wk2 in any case - where we have the WebContext object which we could use to split sessions instead of having the API user assign sessions to each webview. Would you like me to just make NetworkingContextGtk::soupSession() return ResourceHandle::defaultSession() for now while we put some thought on the GTK+ part? > > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:82 > > - SoupCookieJar* jar = defaultCookieJar(); > > + const NetworkingContext* context = networkingContext(document); > > + if (!context) > > + return; > > + > > Shouldn't you use the default cookie jar in case no network context exists? AFAICS, the only way for no NetworkContext to exist is by passing 0 to ResourceHandle::create(), which is only done by webkit_download_start. If there is a Document, there should be a FrameNetworkingContext (I guess). > It's a bit weird that you have a networkingContext method that you only use to obtain the cookie jar, I'd recommend going straight to the jar. Can you elaborate? What do you mean by "straight to the jar" in this context? > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:163 > > + else { > > + if (session == ResourceHandle::defaultSession()) > > Make this an else if and do away with the braces =) Will do.

Comment on attachment 124558 [details] Patch Attachment 124558 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11367618

Comment on attachment 124558 [details] Patch Attachment 124558 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11365660

(In reply to comment #7) > Would you like me to just make NetworkingContextGtk::soupSession() return ResourceHandle::defaultSession() for now while we put some thought on the GTK+ part? I think so, yeah, we'll have to think about this feature a bit more. > > Shouldn't you use the default cookie jar in case no network context exists? > > AFAICS, the only way for no NetworkContext to exist is by passing 0 to ResourceHandle::create(), which is only done by webkit_download_start. If there is a Document, there should be a FrameNetworkingContext (I guess). Right, but downloads started with webkit_download_start() used to have access to the default cookie jar, so they should keep having, right? =) > > It's a bit weird that you have a networkingContext method that you only use to obtain the cookie jar, I'd recommend going straight to the jar. > > Can you elaborate? What do you mean by "straight to the jar" in this context? I mean instead of having a method that is given the document and obtains the context and then from that you get the jar you can have a method which is given a document and returns the jar (returning the default jar if there's no context).

Created attachment 124753 [details] Appease the bots and do not add new API to the GTK+ port

(In reply to comment #10) > (In reply to comment #7) > > > Shouldn't you use the default cookie jar in case no network context exists? > > > > AFAICS, the only way for no NetworkContext to exist is by passing 0 to ResourceHandle::create(), which is only done by webkit_download_start. If there is a Document, there should be a FrameNetworkingContext (I guess). > > Right, but downloads started with webkit_download_start() used to have access to the default cookie jar, so they should keep having, right? =) As discussed on IRC, this part of the CookieJar API is only used via Document, which doesn't exist if ResourceHandle::create(0, ...) is used, as no NetworkingContext (and no Frame) is used.

Attachment 124753 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit ab43d00..676824a master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 106362 = ab43d00719700a5da4bc2b60d447b2153c0dbe70 r106363 = 676824ad52096f0bc058e283b7f9071a227cd7a6 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124753 [details] Appease the bots and do not add new API to the GTK+ port Attachment 124753 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11386040

Created attachment 125648 [details] Drop some USE(SOUP) checks now that it depends on bug 77874

Comment on attachment 125648 [details] Drop some USE(SOUP) checks now that it depends on bug 77874 Attachment 125648 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11434376

Created attachment 126802 [details] Same as 125648; resubmitted to workaround flaky EWS Resubmitting patch to workaround flaky EWS.

Comment on attachment 126802 [details] Same as 125648; resubmitted to workaround flaky EWS View in context: https://bugs.webkit.org/attachment.cgi?id=126802&action=review It looks sane to me and shouldn't change anything for GTK+ as long as we discussed, I'm still not sure about the static networkingContext() static function, see bellow: > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:50 > +static NetworkingContext* networkingContext(const Document* document) > +{ > + if (!document) > + return 0; > + const Frame* frame = document->frame(); > + if (!frame) > + return 0; > + const FrameLoader* loader = frame->loader(); > + if (!loader) > + return 0; > + return loader->networkingContext(); > +} I still find it very odd that you have this static function to return the networkingContext and in every call you make you just check for null and gets the soup session. Why don't you rename this cookieJarForDocument(const Document* document), null check and get the soup session, and return the cookie jar from it?

(In reply to comment #18) > I still find it very odd that you have this static function to return the networkingContext and in every call you make you just check for null and gets the soup session. Why don't you rename this cookieJarForDocument(const Document* document), null check and get the soup session, and return the cookie jar from it? You're right, that makes total sense -- I probably just looked at how the NetworkingContext was obtained in platform/qt/CookieJarQt.cpp and did the same without giving it too much thought.

Created attachment 127192 [details] Obtain the cookie jar in a saner way in CookieJarSoup.cpp

Attachment 127192 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource git.webkit.org[0: 17.254.20.231]: errno=Connection refused fatal: unable to connect a socket (Connection refused) Died at Tools/Scripts/update-webkit line 162. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 127192 [details] Obtain the cookie jar in a saner way in CookieJarSoup.cpp View in context: https://bugs.webkit.org/attachment.cgi?id=127192&action=review > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:52 > + return SOUP_COOKIE_JAR(soup_session_get_feature(context->soupSession(), SOUP_TYPE_COOKIE_JAR)); Hmm, this could return NULL, so better null check the return of soup_session_get_feature, and only do cast/return after that; looks fine otherwise. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:160 > + SoupCookieJar* jar = SOUP_COOKIE_JAR(soup_session_get_feature(session, SOUP_TYPE_COOKIE_JAR)); Same here. This code was probably emitting warnings we just haven't seen them in any of our usual suspects.

Created attachment 127230 [details] Patch for landing

Created attachment 127232 [details] Patch for landing with reviewer set

Created attachment 127269 [details] Patch for landing with reviewer, third time is a charm

(In reply to comment #22) > (From update of attachment 127192 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127192&action=review > > > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:52 > > + return SOUP_COOKIE_JAR(soup_session_get_feature(context->soupSession(), SOUP_TYPE_COOKIE_JAR)); > > Hmm, this could return NULL, so better null check the return of soup_session_get_feature, and only do cast/return after that; looks fine otherwise. > > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:160 > > + SoupCookieJar* jar = SOUP_COOKIE_JAR(soup_session_get_feature(session, SOUP_TYPE_COOKIE_JAR)); > > Same here. This code was probably emitting warnings we just haven't seen them in any of our usual suspects. For posterity: after talking to kov and Bejanmin Otte on #webkitgtk+, we've decided to ignore these, as SOUP_COOKIE_JAR(0) just returns 0 and doesn't emit any warning. I've submitted a patch without the suggested changes.

Committed r107854: <http://trac.webkit.org/changeset/107854>

Attachment 127232 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList Using index info to reconstruct a base tree... <stdin>:84: trailing whitespace. <stdin>:333: trailing whitespace. <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" <stdin>:334: trailing whitespace. width="300" height="300" onload="runRepaintTest()"> <stdin>:335: trailing whitespace. <script xlink:href="../../fast/repaint/resources/repaint.js" /> <stdin>:336: trailing whitespace. <script><![CDATA[ warning: squelched 16 whitespace errors warning: 21 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.