Bug 110147 - [WinCairo] Support for cookies is incomplete
Summary: [WinCairo] Support for cookies is incomplete
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2013-02-18 12:17 PST by Peter Nelson
Modified: 2013-03-05 17:14 PST (History)
3 users (show)

See Also:

Patch (13.05 KB, patch)
2013-02-18 12:37 PST, Peter Nelson
no flags Details | Formatted Diff | Diff
Patch (11.42 KB, patch)
2013-03-05 15:46 PST, Peter Nelson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Nelson 2013-02-18 12:17:20 PST
Support for cookies in WinCairo is incomplete. Problems include:

* Cookies set in HTTP response are not accessible from JavaScript
* Cookies set in JavaScript have an "unknown" domain
* Cookies set in JavaScript do not expire
Comment 1 Peter Nelson 2013-02-18 12:37:39 PST
Created attachment 188930 [details]
Comment 2 Brent Fulgham 2013-02-28 23:48:31 PST
Comment on attachment 188930 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=188930&action=review

This looks great! I just have a few suggestions to make it a little easier to read.

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:45
> +    // 7. value

How about making this an enum, so we can do something like 'attribute[kDomain]' or 'attribute[kExpirationTime]'?

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:59
> +    if (attributes.size())

Do you mean "!attributes.size()" here? It looks like you bail out any time there are attributes to process.

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:64
> +    Vector<String>::iterator attribute = attributes.begin();

Won't this always be empty?

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:108
> +    cookieStr.reserveCapacity(domain.length() + path.length() + cookieName.length() + cookieValue.length() + 26);

What is the magic number for? Can we give it some sort of descriptive label?

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:174
> +        if (attributes.size() != 7)

Let's make this a label, rather than a magic number. 'kNetscapeCookieAttributeCount' or some such.

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:190
> +        bool allowedPath = url.path().startsWith(attributes[2]);

Could all these attribute offsets be part of an enum? That would make this easier to follow.

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:191
> +        bool secure = attributes[3] == "FALSE" || url.protocolIs("https");

attributes[kSecureConnection] == 'FALSE'
Comment 3 Peter Nelson 2013-03-05 15:14:21 PST
Some of these issues seem to have been partially resolved in Changeset 143272. I will upload another patch to deal with the rest shortly.
Comment 4 Peter Nelson 2013-03-05 15:46:07 PST
Created attachment 191588 [details]
Comment 5 Brent Fulgham 2013-03-05 17:04:00 PST
Comment on attachment 191588 [details]

Looks good.  Thank you for revising the earlier patch.
Comment 6 WebKit Review Bot 2013-03-05 17:14:26 PST
Comment on attachment 191588 [details]

Clearing flags on attachment: 191588

Committed r144853: <http://trac.webkit.org/changeset/144853>
Comment 7 WebKit Review Bot 2013-03-05 17:14:29 PST
All reviewed patches have been landed.  Closing bug.