Bug 99340

Summary:

[Chromium] Remove cookie-related functions from PlatformSupport

Product:

WebKit

Reporter:

Mark Pilgrim (Google) <pilgrim>

Component:

New Bugs

Assignee:

Mark Pilgrim (Google) <pilgrim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, danw, dglazkov, eric.carlson, feature-media-reviews, fishd, gtk-ews, gustavo, gyuyoung.kim, japhet, mrobinson, rakuco, webkit.review.bot, xan.lopez, yutak

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

102456

Bug Blocks:

82948

Attachments:

Description	Flags
WIP Patch	none
WIP Patch	none
Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
WIP Patch	none
Patch	none
Patch	none
Patch	none

Mark Pilgrim (Google)

Reported 2012-10-15 10:57:53 PDT

[Chromium] Remove cookie-related functions from PlatformSupport

Attachments
WIP Patch (11.79 KB, patch) 2012-10-15 11:03 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (21.04 KB, patch) 2012-10-19 12:35 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (32.55 KB, patch) 2012-10-19 13:12 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (38.73 KB, patch) 2012-10-24 12:44 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (38.74 KB, patch) 2012-10-24 13:07 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (38.74 KB, patch) 2012-10-24 13:28 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (38.76 KB, patch) 2012-10-29 10:08 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (38.85 KB, patch) 2012-10-29 10:42 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (47.06 KB, patch) 2012-10-29 11:21 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (49.13 KB, patch) 2012-10-29 11:30 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (49.13 KB, patch) 2012-10-29 12:03 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (50.72 KB, patch) 2012-10-29 12:19 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (50.47 KB, patch) 2012-10-29 12:32 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (50.47 KB, patch) 2012-10-29 13:00 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (52.89 KB, patch) 2012-10-31 10:54 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (55.00 KB, patch) 2012-11-01 07:32 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (56.61 KB, patch) 2012-11-01 09:43 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (56.61 KB, patch) 2012-11-01 12:32 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (57.21 KB, patch) 2012-11-05 11:51 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (57.43 KB, patch) 2012-11-05 13:06 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (64.72 KB, patch) 2012-11-07 12:56 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (67.72 KB, patch) 2012-11-07 15:15 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (67.91 KB, patch) 2012-11-08 06:18 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
WIP Patch (15.22 KB, patch) 2012-11-14 10:13 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (27.40 KB, patch) 2012-11-14 11:11 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (24.96 KB, patch) 2012-11-16 06:57 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (25.02 KB, patch) 2012-11-16 13:51 PST, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Show Obsolete (26) View All Add attachment proposed patch, testcase, etc.

Mark Pilgrim (Google)

Comment 1 2012-10-15 11:03:44 PDT

Created attachment 168741 [details] WIP Patch

Adam Barth

Comment 2 2012-10-15 19:35:45 PDT

Comment on attachment 168741 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168741&action=review Presumably you'll need to make similar changes to CookieJarFoo.cpp > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:436 > + Frame* frame = m_document->frame(); > + if (frame) { > + CookieJar cookieJar = frame->loader()->client()->cookieJar(); > + if (cookieJar.cookiesEnabled(m_document)) { This all seems like work that the Document should do. Can we ask the document if cookies are enabled? > Source/WebCore/dom/Document.cpp:3820 > + String cookies; > + if (Frame* frame = this->frame()) { > + CookieJar cookieJar = frame->loader()->client()->cookieJar(); > + cookies = cookieJar.cookies(this, cookieURL); > + } > + return cookies; This can all be written much more simply as: if (!frame()) return String(); return frame()->loader()->client()->cookieJar().cookies(this, cookieURL); > Source/WebCore/dom/Document.cpp:3844 > - setCookies(this, cookieURL, value); > + if (Frame* frame = this->frame()) { > + CookieJar cookieJar = frame->loader()->client()->cookieJar(); > + cookieJar.setCookies(this, cookieURL, value); > + } Same here: if (!frame()) return; frame()->loader()->client()->cookieJar().setCookies(this, cookieURL, value); > Source/WebCore/inspector/InspectorPageAgent.cpp:555 > + WebCore::CookieJar cookieJar = frame->loader()->client()->cookieJar(); I doubt the "WebCore::" is needed here. > Source/WebCore/page/Navigator.cpp:122 > + CookieJar cookieJar = m_frame->loader()->client()->cookieJar(); > + return cookieJar.cookiesEnabled(m_frame->document()); These lines can be combined, or we can just ask the document since other code seems to want to ask the document whether cookies are enabled.

Adam Barth

Comment 3 2012-10-15 19:35:58 PDT

Looks like you're on the right track.

Mark Pilgrim (Google)

Comment 4 2012-10-19 12:35:37 PDT

Created attachment 169674 [details] WIP Patch

Mark Pilgrim (Google)

Comment 5 2012-10-19 12:39:03 PDT

(In reply to comment #4) > Created an attachment (id=169674) [details] > WIP Patch Implements all of Adam's suggestions to date, finishes Chromium implementation (m_webCookieJar in CookieJar.h, FrameLoaderClientImpl::cookieJar() implementation). Actually works on Chromium Linux. Other platforms TBD.

Mark Pilgrim (Google)

Comment 6 2012-10-19 13:12:20 PDT

Created attachment 169683 [details] Patch

Mark Pilgrim (Google)

Comment 7 2012-10-19 13:19:17 PDT

(In reply to comment #6) > Created an attachment (id=169683) [details] > WIP Patch Implements new CookieJar interface in CookieJarWin.cpp, CookieJarCurl.cpp, and CookieJarCFNet.cpp. It looks like CookieJarQt might need some attention; it actually uses the passed-in document to get some other reference.

Build Bot

Comment 8 2012-10-19 13:40:30 PDT

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

Build Bot

Comment 9 2012-10-19 13:51:15 PDT

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

Adam Barth

Comment 10 2012-10-19 14:02:36 PDT

Comment on attachment 169683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169683&action=review This looks great. There are some compile errors to work through, however. > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:265 > - String cookie = cookieRequestHeaderFieldValue(document, url); > + String cookie; > + Frame* frame = document->frame(); > + if (frame) { > + CookieJar cookieJar = frame->loader()->client()->cookieJar(); > + cookie = cookieJar.cookieRequestHeaderFieldValue(url, document->firstPartyForCookies()); > + } This code looks copy/pasted. Should we factor this out into a helper function in this code? > Source/WebCore/platform/CookieJar.h:46 > + CookieJar() { m_webCookieJar = 0; }; Usually we put this constructor outside the ifdef and just have the m_webCookieJar assignment inside the ifdef. > Source/WebCore/platform/CookieJar.h:47 > + CookieJar(WebKit::WebCookieJar* webCookieJar) { m_webCookieJar = webCookieJar; }; Please mark one-argument constructors "explicit" > Source/WebCore/platform/network/chromium/CookieJarChromium.cpp:35 > + No need for this blank line. > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1653 > + WebFrameImpl* frameImpl = WebFrameImpl::fromFrame(m_webFrame->frame()->document()->frame()); Why go though all this nuttiness? Isn't m_webFrame just a WebFrameImpl?

Mark Pilgrim (Google)

Comment 11 2012-10-24 12:44:24 PDT

Created attachment 170450 [details] WIP Patch

Mark Pilgrim (Google)

Comment 12 2012-10-24 12:45:03 PDT

Comment on attachment 170450 [details] WIP Patch Fixes for Mac and Windows builds. No need to review until EWS bots come back green.

Mark Pilgrim (Google)

Comment 13 2012-10-24 12:45:43 PDT

Comment on attachment 170450 [details] WIP Patch Also addressed Adam's latest feedback.

Early Warning System Bot

Comment 14 2012-10-24 12:51:20 PDT

Comment on attachment 170450 [details] WIP Patch Attachment 170450 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14553308

Early Warning System Bot

Comment 15 2012-10-24 12:52:43 PDT

Comment on attachment 170450 [details] WIP Patch Attachment 170450 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14545358

Mark Pilgrim (Google)

Comment 16 2012-10-24 13:07:51 PDT

Created attachment 170456 [details] WIP Patch

Mark Pilgrim (Google)

Comment 17 2012-10-24 13:08:19 PDT

Comment on attachment 170456 [details] WIP Patch Fixes for qt builds.

Early Warning System Bot

Comment 18 2012-10-24 13:14:31 PDT

Comment on attachment 170456 [details] WIP Patch Attachment 170456 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14566324

Early Warning System Bot

Comment 19 2012-10-24 13:16:25 PDT

Comment on attachment 170456 [details] WIP Patch Attachment 170456 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14548340

Mark Pilgrim (Google)

Comment 20 2012-10-24 13:28:11 PDT

Created attachment 170459 [details] WIP Patch

Early Warning System Bot

Comment 21 2012-10-24 13:39:49 PDT

Comment on attachment 170459 [details] WIP Patch Attachment 170459 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14548348

Early Warning System Bot

Comment 22 2012-10-24 13:41:45 PDT

Comment on attachment 170459 [details] WIP Patch Attachment 170459 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14546338

Build Bot

Comment 23 2012-10-24 14:00:46 PDT

Comment on attachment 170459 [details] WIP Patch Attachment 170459 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14559338

kov's GTK+ EWS bot

Comment 24 2012-10-24 14:16:33 PDT

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

EFL EWS Bot

Comment 25 2012-10-24 14:52:31 PDT

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

Build Bot

Comment 26 2012-10-24 15:14:57 PDT

Comment on attachment 170459 [details] WIP Patch Attachment 170459 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14543406

Mark Pilgrim (Google)

Comment 27 2012-10-29 10:08:59 PDT

Created attachment 171274 [details] WIP Patch

Mark Pilgrim (Google)

Comment 28 2012-10-29 10:09:17 PDT

Comment on attachment 171274 [details] WIP Patch Should fix Windows build.

Early Warning System Bot

Comment 29 2012-10-29 10:20:38 PDT

Comment on attachment 171274 [details] WIP Patch Attachment 171274 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14641145

Early Warning System Bot

Comment 30 2012-10-29 10:22:16 PDT

Comment on attachment 171274 [details] WIP Patch Attachment 171274 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14627206

Mark Pilgrim (Google)

Comment 31 2012-10-29 10:42:13 PDT

Created attachment 171280 [details] WIP Patch

Mark Pilgrim (Google)

Comment 32 2012-10-29 10:42:34 PDT

Comment on attachment 171280 [details] WIP Patch Should fix Mac build.

Early Warning System Bot

Comment 33 2012-10-29 10:53:42 PDT

Comment on attachment 171280 [details] WIP Patch Attachment 171280 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14623402

Early Warning System Bot

Comment 34 2012-10-29 10:55:59 PDT

Comment on attachment 171280 [details] WIP Patch Attachment 171280 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14631196

Build Bot

Comment 35 2012-10-29 11:18:21 PDT

Comment on attachment 171280 [details] WIP Patch Attachment 171280 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14620493

Mark Pilgrim (Google)

Comment 36 2012-10-29 11:21:00 PDT

Created attachment 171282 [details] WIP Patch

Mark Pilgrim (Google)

Comment 37 2012-10-29 11:21:24 PDT

Comment on attachment 171282 [details] WIP Patch Attempted Qt fix.

Mark Pilgrim (Google)

Comment 38 2012-10-29 11:30:10 PDT

Created attachment 171284 [details] WIP Patch

Mark Pilgrim (Google)

Comment 39 2012-10-29 11:30:37 PDT

Comment on attachment 171284 [details] WIP Patch Fix for Win build.

Early Warning System Bot

Comment 40 2012-10-29 11:40:23 PDT

Comment on attachment 171284 [details] WIP Patch Attachment 171284 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14618830

Early Warning System Bot

Comment 41 2012-10-29 11:42:09 PDT

Comment on attachment 171284 [details] WIP Patch Attachment 171284 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14618831

Mark Pilgrim (Google)

Comment 42 2012-10-29 12:03:26 PDT

Created attachment 171287 [details] WIP Patch

Mark Pilgrim (Google)

Comment 43 2012-10-29 12:04:04 PDT

Comment on attachment 171287 [details] WIP Patch Qt fix.

Early Warning System Bot

Comment 44 2012-10-29 12:14:47 PDT

Comment on attachment 171287 [details] WIP Patch Attachment 171287 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14631213

Mark Pilgrim (Google)

Comment 45 2012-10-29 12:19:13 PDT

Created attachment 171290 [details] WIP Patch

Mark Pilgrim (Google)

Comment 46 2012-10-29 12:19:45 PDT

Comment on attachment 171290 [details] WIP Patch Fix for WK2 builds.

Early Warning System Bot

Comment 47 2012-10-29 12:28:58 PDT

Comment on attachment 171290 [details] WIP Patch Attachment 171290 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14558057

Mark Pilgrim (Google)

Comment 48 2012-10-29 12:32:16 PDT

Created attachment 171293 [details] WIP Patch

Early Warning System Bot

Comment 49 2012-10-29 12:57:20 PDT

Comment on attachment 171293 [details] WIP Patch Attachment 171293 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14630215

Mark Pilgrim (Google)

Comment 50 2012-10-29 13:00:31 PDT

Created attachment 171296 [details] WIP Patch

Mark Pilgrim (Google)

Comment 51 2012-10-29 13:02:23 PDT

Comment on attachment 171296 [details] WIP Patch WK2 fix.

Build Bot

Comment 52 2012-10-29 13:47:54 PDT

Comment on attachment 171296 [details] WIP Patch Attachment 171296 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14626284

EFL EWS Bot

Comment 53 2012-10-29 14:20:45 PDT

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

Mark Pilgrim (Google)

Comment 54 2012-10-31 10:54:13 PDT

Created attachment 171684 [details] Patch

Build Bot

Comment 55 2012-10-31 11:56:08 PDT

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

EFL EWS Bot

Comment 56 2012-10-31 11:57:32 PDT

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

Mark Pilgrim (Google)

Comment 57 2012-11-01 07:32:27 PDT

Created attachment 171848 [details] Patch

EFL EWS Bot

Comment 58 2012-11-01 08:28:18 PDT

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

Build Bot

Comment 59 2012-11-01 08:53:08 PDT

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

Mark Pilgrim (Google)

Comment 60 2012-11-01 09:43:52 PDT

Created attachment 171875 [details] Patch

EFL EWS Bot

Comment 61 2012-11-01 10:30:19 PDT

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

Build Bot

Comment 62 2012-11-01 10:33:27 PDT

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

Mark Pilgrim (Google)

Comment 63 2012-11-01 12:32:00 PDT

Created attachment 171908 [details] Patch

EFL EWS Bot

Comment 64 2012-11-01 12:58:47 PDT

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

Build Bot

Comment 65 2012-11-01 13:36:06 PDT

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

Mark Pilgrim (Google)

Comment 66 2012-11-05 11:42:28 PST

I can not reproduce this Mac compile failure locally.

Mark Pilgrim (Google)

Comment 67 2012-11-05 11:51:24 PST

Created attachment 172373 [details] Patch

Mark Pilgrim (Google)

Comment 68 2012-11-05 11:52:18 PST

This latest patch may -- or may not -- solve the Mac compilation problems that I can't reproduce.

Build Bot

Comment 69 2012-11-05 12:48:22 PST

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

Mark Pilgrim (Google)

Comment 70 2012-11-05 13:06:44 PST

Created attachment 172384 [details] Patch

EFL EWS Bot

Comment 71 2012-11-05 18:21:02 PST

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

Mark Pilgrim (Google)

Comment 72 2012-11-07 12:56:42 PST

Created attachment 172862 [details] Patch

Mark Pilgrim (Google)

Comment 73 2012-11-07 12:57:40 PST

(In reply to comment #72) > Created an attachment (id=172862) [details] > Patch Merge in changes to CookieJar.mm, plus EFL fixes.

kov's GTK+ EWS bot

Comment 74 2012-11-07 14:40:01 PST

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

EFL EWS Bot

Comment 75 2012-11-07 14:54:42 PST

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

EFL EWS Bot

Comment 76 2012-11-07 14:55:32 PST

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

Build Bot

Comment 77 2012-11-07 14:56:40 PST

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

kov's GTK+ EWS bot

Comment 78 2012-11-07 15:06:10 PST

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

Mark Pilgrim (Google)

Comment 79 2012-11-07 15:15:36 PST

Created attachment 172878 [details] Patch

kov's GTK+ EWS bot

Comment 80 2012-11-07 16:43:44 PST

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

kov's GTK+ EWS bot

Comment 81 2012-11-07 16:44:07 PST

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

EFL EWS Bot

Comment 82 2012-11-07 17:24:20 PST

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

Mark Pilgrim (Google)

Comment 83 2012-11-08 06:18:27 PST

Created attachment 173031 [details] Patch

Adam Barth

Comment 84 2012-11-08 10:49:03 PST

Alexey is also working on this code in bug 101621 using a slightly different approach.

Alexey Proskuryakov

Comment 85 2012-11-08 11:00:42 PST

I haven't seen this bug, because its title incorrectly started with [chromium]. Bracketed titles are used to signal that people working on other ports need not care about this work. Usually it means that no cross-platform files are touched at all.

Alexey Proskuryakov

Comment 86 2012-11-08 11:08:42 PST

Comment on attachment 173031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173031&action=review I think that going with NetworkingContext is the right thing to do here - ports already use it for cookies, and it feels quite nice. > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:44 > +#include "Frame.h" > +#include "FrameLoader.h" > +#include "FrameLoaderClient.h" It's not great that this approach requires adding more Frame dependencies into loading code like this. Even though WebSocketHandshake.cpp is not in platform directory (and already has document dependencies), it's logically part of non-Web platform functionality. > Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:218 > +void CookieJar::getHostnamesWithCookies(HashSet<String>& hostnames) > { > RetainPtr<CFHTTPCookieStorageRef> cookieStorage = defaultCFHTTPCookieStorage(); This makes Mac and CFNet ports quite a bit more confusing. If we are going to have multiple "CookieJar" instances, then they should incorporate storage - otherwise, it's not really a class. > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h:57 > +class CookieJar; You have also added '#include "CookieJar.h"' above.

Alexey Proskuryakov

Comment 87 2012-11-08 11:20:39 PST

Note that my patch does not attempt to remove cookie-related functions from Chromium's PlatformSupport. It does a conflicting refactoring and fixes a layering violation. Chromium and Blackberry end up with forked code, because I cannot practically address the issue for these ports.

Mark Pilgrim (Google)

Comment 88 2012-11-14 10:13:17 PST

Created attachment 174188 [details] WIP Patch

Mark Pilgrim (Google)

Comment 89 2012-11-14 11:11:44 PST

Created attachment 174206 [details] Patch

Adam Barth

Comment 90 2012-11-14 11:33:53 PST

Comment on attachment 174206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174206&action=review This looks great. One nit and one question. > Source/WebCore/loader/CookieJar.cpp:68 > +#if PLATFORM(CHROMIUM) > + return cookiesEnabled(networkingContext(document), document->cookieURL(), document->firstPartyForCookies()); > +#else > return cookiesEnabled(networkingContext(document)); > +#endif You should change PlatformCookieJar.h to take these extra arguments and make all the other implementations ignore them. > Source/WebCore/loader/CookieJar.cpp:73 > +#if PLATFORM(CHROMIUM) ditto > Source/WebCore/loader/CookieJar.cpp:82 > +#if PLATFORM(CHROMIUM) ditto > Source/WebCore/platform/network/chromium/CookieJarChromium.cpp:44 > +void setCookiesFromDOM(NetworkingContext* context, const KURL& firstPartyForCookies, const KURL& url, const String& cookieStr) cookieStr -> cookieString > Source/WebCore/platform/network/chromium/CookieJarChromium.cpp:47 > + if (!context) > + return; Is this right, or should we use the default cookie jar from WebKit::Platform?

Alexey Proskuryakov

Comment 91 2012-11-14 13:39:15 PST

The approach looks good to me, too. > > + return cookiesEnabled(networkingContext(document), document->cookieURL(), document->firstPartyForCookies()); > You should change PlatformCookieJar.h to take these extra arguments and make all the other implementations ignore them. What does it mean for cookies to be enabled for a specific URL and first party only? This function is a bit of a mystery as is to me, but with the extra arguments, I'm not sure how to decipher it at all.

Adam Barth

Comment 92 2012-11-14 13:44:39 PST

> What does it mean for cookies to be enabled for a specific URL and first party only? This function is a bit of a mystery as is to me, but with the extra arguments, I'm not sure how to decipher it at all. Chrome has an option to enable and disable cookies on a fine-grained basis, including depending on the host names of the first and third parties.

Alexey Proskuryakov

Comment 93 2012-11-14 15:33:42 PST

Perhaps cookiesEnabled() needs a better name. I don't know what it means for cookies to be "enabled" - there is no such thing in UI, and technically, there are a lot of separate aspects. For example, Mac port says that cookies are enabled if any cookies can ever be accepted (i.e. policy is not "never accept", which is not at all clear from the function name.

Adam Barth

Comment 94 2012-11-14 15:40:31 PST

> Perhaps cookiesEnabled() needs a better name. It looks like it maps to navigator.cookieEnabled, which means we should probably keep the name for consistency with the DOM. > I don't know what it means for cookies to be "enabled" - there is no such thing in UI, and technically, there are a lot of separate aspects. Apparently there is such a thing in the DOM and we need to be able to provide a true/false value for each document. > For example, Mac port says that cookies are enabled if any cookies can ever be accepted (i.e. policy is not "never accept", which is not at all clear from the function name. That doesn't seem like the most helpful DOM API. I would expect that web sites reading navigator.cookieEnabled would be most interested in whether cookies are accepted in the specific context in question rather than whether there is some other context in a galaxy far, far away where cookies are accepted.

Alexey Proskuryakov

Comment 95 2012-11-14 16:29:33 PST

(In reply to comment #94) > > Perhaps cookiesEnabled() needs a better name. > > It looks like it maps to navigator.cookieEnabled, which means we should probably keep the name for consistency with the DOM. Makes much better sense to me now! But is there actually an inconsistency between "cookie_s_Enabled" and "cookieEnabled" here? Perhaps we could add "DOM" to function name, just like I did with two other functions a few days ago.

Mark Pilgrim (Google)

Comment 96 2012-11-15 19:10:17 PST

Split part of this post-PlatformCookieJar patch into bug 102456. Once that lands, this bug will deal with the remaining Chromium-specific changes. Restoring original bug name to reflect this.

Mark Pilgrim (Google)

Comment 97 2012-11-16 06:57:15 PST

Created attachment 174675 [details] Patch

Adam Barth

Comment 98 2012-11-16 09:37:25 PST

Comment on attachment 174675 [details] Patch Yay!

WebKit Review Bot

Comment 99 2012-11-16 11:02:41 PST

Comment on attachment 174675 [details] Patch Clearing flags on attachment: 174675 Committed r134973: <http://trac.webkit.org/changeset/134973>

WebKit Review Bot

Comment 100 2012-11-16 11:02:54 PST

All reviewed patches have been landed. Closing bug.

Dimitri Glazkov (Google)

Comment 101 2012-11-16 11:28:22 PST

Reverted r134973 for reason: Broke compile on at least Mac and Linux. Committed r134975: <http://trac.webkit.org/changeset/134975>

Adam Barth

Comment 102 2012-11-16 13:38:05 PST

Comment on attachment 174675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174675&action=review > Source/WebKit/chromium/src/FrameNetworkingContextImpl.cpp:35 > +#include "public/WebFrameClient.h" I don't think this is the right way to include this file. I think you should just include it as follows: #include "WebFrameClient.h"

Mark Pilgrim (Google)

Comment 103 2012-11-16 13:51:27 PST

Created attachment 174753 [details] Patch

WebKit Review Bot

Comment 104 2012-11-16 15:53:13 PST

Comment on attachment 174753 [details] Patch Clearing flags on attachment: 174753 Committed r135013: <http://trac.webkit.org/changeset/135013>

WebKit Review Bot

Comment 105 2012-11-16 15:53:25 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.