Bug 101621 – CookieJar uses Document class, which is a layering violation

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Alexey Proskuryakov]

Platform code should not use Document.

[Alexey Proskuryakov]

Created attachment 173063 [details]
first attempt

No ChangeLog yet, just running by EWS.

[Adam Barth]

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

[Alexey Proskuryakov]

Created attachment 173072 [details]
second attempt

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Created attachment 173078 [details]
third attempt

WIll it finally apply?

[Adam Barth]

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

[Alexey Proskuryakov]

Created attachment 173083 [details]
another attempt

Manually edited the patch to apply.

[Alexey Proskuryakov]

> 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.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 173083 [details]
another attempt

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

[Early Warning System Bot]

Comment on attachment 173083 [details]
another attempt

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

[EFL EWS Bot]

Comment on attachment 173083 [details]
another attempt

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

[Adam Barth]

> 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.

[kov's GTK+ EWS bot]

Comment on attachment 173083 [details]
another attempt

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

[Alexey Proskuryakov]

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.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 173113 [details]
patch for review

Attachment 173113 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14775230

[Early Warning System Bot]

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

[Brady Eidson]

Comment on attachment 173113 [details]
patch for review

This approach seems fine to me.  Gotta fix the last lingering errors.

[EFL EWS Bot]

Comment on attachment 173113 [details]
patch for review

Attachment 173113 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14777140

[Alexey Proskuryakov]

Created attachment 173117 [details]
more build fixing

[WebKit Review Bot]

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.

[Alexey Proskuryakov]

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.

[Brady Eidson]

Comment on attachment 173143 [details]
proposed patch

Looks good.  If Adam wants another chance to take a look, do that too.  :)

[Adam Barth]

> 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.

[Alexey Proskuryakov]

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

[WebKit Review Bot]

Comment on attachment 173143 [details]
proposed patch

Clearing flags on attachment: 173143

Committed r134082: <http://trac.webkit.org/changeset/134082>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> 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.

[Adam Barth]

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.

[Alexey Proskuryakov]

> 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.

[Mark Pilgrim (Google)]

Followup: bug 102456 in preparation for Chromium integration.