Bug 101621 - CookieJar uses Document class, which is a layering violation
Summary: CookieJar uses Document class, which is a layering violation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-08 10:22 PST by Alexey Proskuryakov
Modified: 2012-11-15 19:07 PST (History)
16 users (show)

See Also:


Attachments
first attempt (108.33 KB, patch)
2012-11-08 10:24 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
second attempt (114.66 KB, patch)
2012-11-08 10:54 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
third attempt (114.71 KB, patch)
2012-11-08 11:16 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
another attempt (114.71 KB, patch)
2012-11-08 11:42 PST, Alexey Proskuryakov
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch for review (125.54 KB, patch)
2012-11-08 13:52 PST, Alexey Proskuryakov
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
more build fixing (126.07 KB, patch)
2012-11-08 14:29 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed patch (126.88 KB, patch)
2012-11-08 16:22 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-11-08 10:22:33 PST
Platform code should not use Document.
Comment 1 Alexey Proskuryakov 2012-11-08 10:24:02 PST
Created attachment 173063 [details]
first attempt

No ChangeLog yet, just running by EWS.
Comment 2 Adam Barth 2012-11-08 10:48:36 PST
Mark Pilgrim is also working on this issue in bug 99340, using a slightly different approach.
Comment 3 Alexey Proskuryakov 2012-11-08 10:54:31 PST
Created attachment 173072 [details]
second attempt
Comment 4 Alexey Proskuryakov 2012-11-08 10:58:41 PST
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.
Comment 5 Alexey Proskuryakov 2012-11-08 11:16:15 PST
Created attachment 173078 [details]
third attempt

WIll it finally apply?
Comment 6 Adam Barth 2012-11-08 11:27:02 PST
We want to delete PlatformSupport.  I'll need to look at your approach to see if it conflicts with that goal.
Comment 7 Alexey Proskuryakov 2012-11-08 11:42:35 PST
Created attachment 173083 [details]
another attempt

Manually edited the patch to apply.
Comment 8 Alexey Proskuryakov 2012-11-08 11:46:03 PST
> 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.
Comment 9 WebKit Review Bot 2012-11-08 11:47:44 PST
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 10 Early Warning System Bot 2012-11-08 11:48:31 PST
Comment on attachment 173083 [details]
another attempt

Attachment 173083 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14775195
Comment 11 Early Warning System Bot 2012-11-08 11:50:05 PST
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 12 EFL EWS Bot 2012-11-08 11:51:50 PST
Comment on attachment 173083 [details]
another attempt

Attachment 173083 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14779005
Comment 13 Adam Barth 2012-11-08 12:35:25 PST
> 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 14 kov's GTK+ EWS bot 2012-11-08 13:40:27 PST
Comment on attachment 173083 [details]
another attempt

Attachment 173083 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14770333
Comment 15 Alexey Proskuryakov 2012-11-08 13:52:50 PST
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.
Comment 16 WebKit Review Bot 2012-11-08 13:58:26 PST
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 17 Early Warning System Bot 2012-11-08 14:02:30 PST
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 18 Early Warning System Bot 2012-11-08 14:06:29 PST
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 19 Brady Eidson 2012-11-08 14:13:34 PST
Comment on attachment 173113 [details]
patch for review

This approach seems fine to me.  Gotta fix the last lingering errors.
Comment 20 EFL EWS Bot 2012-11-08 14:17:24 PST
Comment on attachment 173113 [details]
patch for review

Attachment 173113 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14777140
Comment 21 Alexey Proskuryakov 2012-11-08 14:29:21 PST
Created attachment 173117 [details]
more build fixing
Comment 22 WebKit Review Bot 2012-11-08 14:34:51 PST
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.
Comment 23 Alexey Proskuryakov 2012-11-08 16:22:37 PST
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 24 Brady Eidson 2012-11-08 16:44:56 PST
Comment on attachment 173143 [details]
proposed patch

Looks good.  If Adam wants another chance to take a look, do that too.  :)
Comment 25 Adam Barth 2012-11-08 19:12:31 PST
> 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.
Comment 26 Alexey Proskuryakov 2012-11-09 09:00:13 PST
Adam, did you have any comments? I'd like to land soon.
Comment 27 WebKit Review Bot 2012-11-09 10:08:35 PST
Comment on attachment 173143 [details]
proposed patch

Clearing flags on attachment: 173143

Committed r134082: <http://trac.webkit.org/changeset/134082>
Comment 28 WebKit Review Bot 2012-11-09 10:08:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Adam Barth 2012-11-09 11:24:04 PST
> 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 30 Adam Barth 2012-11-09 11:28:00 PST
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.
Comment 31 Alexey Proskuryakov 2012-11-09 11:39:11 PST
> 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.
Comment 32 Mark Pilgrim (Google) 2012-11-15 19:07:33 PST
Followup: bug 102456 in preparation for Chromium integration.