[Webkit-unassigned] [Bug 110147] [WinCairo] Support for cookies is incomplete

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=110147


Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #188930|review?                     |review-
               Flag|                            |




--- Comment #2 from Brent Fulgham <bfulgham at webkit.org>  2013-02-28 23:48:31 PST ---
(From update of attachment 188930)
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'

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.