Bug 101621

Created attachment 173063 [details] first attempt No ChangeLog yet, just running by EWS.

Mark Pilgrim is also working on this issue in bug 99340, using a slightly different approach.

Created attachment 173072 [details] second attempt

I haven't seen that bug! I prefer my approach though - further spreading dependency of Frame and FrameLoader client seems undesirable when we already have NetowrkingContext that many platforms use.

Created attachment 173078 [details] third attempt WIll it finally apply?

We want to delete PlatformSupport. I'll need to look at your approach to see if it conflicts with that goal.

Created attachment 173083 [details] another attempt Manually edited the patch to apply.

> We want to delete PlatformSupport. I'll need to look at your approach to see if it conflicts with that goal. Please do have a look! I do not think that there will be a conflict. With this patch, CookieJar is split into two versions - one that uses non-platform concepts, and another that is purely an interface to underlying platform. There should be enough flexibility for any port here.

Attachment 173083 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1 Source/WebCore/platform/network/qt/CookieJarQt.cpp:33: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/network/PlatformCookieJar.h:31: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/network/soup/CookieJarSoup.cpp:25: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/loader/CookieJar.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 4 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 173083 [details] another attempt Attachment 173083 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14775195

Comment on attachment 173083 [details] another attempt Attachment 173083 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14762778

Comment on attachment 173083 [details] another attempt Attachment 173083 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14779005

> Please do have a look! Thanks. Unfortunately, my afternoon is booked. I'll be able to take a look this evening. One of the design pressures in Mark's patch is that Chromium might want to use different cookie jars in different frames. That's the reason for the introduction of the CookieJar class rather than using free functions. The CookieJar class can then carry platform-specific state. For example, on Chromium, the CookieJar can carry a reference to which cookie jar this document should use. On other platforms, it might be appropriate for the CookieJar class to carry a NetworkingContext. I haven't looked at Mark's most recent patch, but he says that it removes the Document layering violation as well.

Comment on attachment 173083 [details] another attempt Attachment 173083 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14770333

Created attachment 173113 [details] patch for review Chromium bot is slow today. Fixed errors that EWS reported for other ports. There will probably be more errors, but this is ready for review. > One of the design pressures in Mark's patch is that Chromium might want to use different cookie jars in different frames. That's the reason for the introduction of the CookieJar class rather than using free functions. This design goal makes sense, and I think that my patch doesn't make it harder. The WebCore level interface to CookieJar is unchanged - I would certainly prefer if chromium turned to a platform level abstraction like NetworkingContext, but you can keep using a Frame if such a change is undesirable. Unfortunately, converting CookieJar into a class won't work for Mac (and Mark's patch leaves Mac in a confusing state). We don't have a separate independent cookie storage that could be encapsulated in a class - in most cases, cookie storage is part of a "storage session". This is why getting cookie storage from a networking context is the design I prefer. I'm extremely sad that we've got so much wasted effort on Mark's part.

Attachment 173113 [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/WebCore/loader/CookieJar.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 173113 [details] patch for review Attachment 173113 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14775230

Comment on attachment 173113 [details] patch for review Attachment 173113 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14758887

Comment on attachment 173113 [details] patch for review This approach seems fine to me. Gotta fix the last lingering errors.

Comment on attachment 173113 [details] patch for review Attachment 173113 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14777140

Created attachment 173117 [details] more build fixing

Attachment 173117 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/loader/CookieJar.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 173143 [details] proposed patch EWS is still way behind. Someone helped me check and fix Chromium build. Should be fully ready for review now.

Comment on attachment 173143 [details] proposed patch Looks good. If Adam wants another chance to take a look, do that too. :)

> If Adam wants another chance to take a look, do that too. :) I would like to take a look before you land this patch. It's on my list for tonight.

Adam, did you have any comments? I'd like to land soon.

Comment on attachment 173143 [details] proposed patch Clearing flags on attachment: 173143 Committed r134082: <http://trac.webkit.org/changeset/134082>

All reviewed patches have been landed. Closing bug.

> Adam, did you have any comments? I'd like to land soon. Sorry, I got distracted last night and fell asleep. I'll look now.

Comment on attachment 173143 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=173143&action=review > Source/WebCore/loader/CookieJar.cpp:48 > + if (!loader) > + return 0; This check isn't needed. It's not possible for frame->loader() to be 0. > Source/WebCore/loader/CookieJar.cpp:55 > +String cookies(const Document* document, const KURL& url) > +{ > + return cookiesForDOM(networkingContext(document), document->firstPartyForCookies(), url); > +} We should be able to inline these functions into their callers. I guess the problem is that Chromium and Blackberry don't use this file? We should be able to teach Chromium to use the NetworkingContext rather than the document, especially if we have a NetworkingContextClient that the code can use to ask which cookie jar is appropriate. > Source/WebCore/loader/chromium/CookieJarChromium.cpp:40 > +// FIXME: Unfork. This file is forked because all other platforms use NetworkingContext to access cookie jar, not Document or Frame. We should be able to unfork this file by adding a NetworkingContextClient.

> We should be able to teach Chromium to use the NetworkingContext rather than the document, especially if we have a NetworkingContextClient that the code can use to ask which cookie jar is appropriate. Sound right to me. Not sure if a client is needed, or a proper configured NetworkingContext can be just associated with the frame in advance. I fully agree that once ports are unforked, further cleanup can be done.

Followup: bug 102456 in preparation for Chromium integration.