[webkit-reviews] review denied: [Bug 110147] [WinCairo] Support for cookies is incomplete : [Attachment 188930] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Peter Nelson
<peter at peterdn.com>'s request for review:
Bug 110147: [WinCairo] Support for cookies is incomplete
https://bugs.webkit.org/show_bug.cgi?id=110147

Attachment 188930: Patch
https://bugs.webkit.org/attachment.cgi?id=188930&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
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'