[webkit-reviews] review granted: [Bug 10957] HttpOnly Cookie Option : [Attachment 25222] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 10957: HttpOnly Cookie Option
https://bugs.webkit.org/show_bug.cgi?id=10957

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> -    // <rdar://problem/5632883> On 10.5, NSHTTPCookieStorage would happily
store an empty cookie, which would be sent as "Cookie: =".
> +    // <rdar://problem/5632883> On 10.5, NSHTTPCookieStorage would happily
store an empty cookie,

You removed "happily" elsewhere, but not here :-)

> Index: WebCore/platform/network/soup/CookieJarSoup.h
> +#include <wtf/Platform.h>

Why is this include needed? I think that we rely on cpp files to include
config.h, which includes Platform.h.

> +    return
reinterpret_cast<IsHTTPOnlyFunction>(GetProcAddress(GetModuleHandleA("CFNetwork
"), "CFHTTPCookieIsHTTPOnly"));

Just to confirm: weak linking doesn't work with MSVC and/or CFNetwork.dll,
correct?

> +    frame->domWindow()->console()->addMessage(JSMessageSource,
ErrorMessageLevel, message, 1, String());

I really wish we could get rid of these source-less error messages everywhere,
but this patch is obviously not when it should be done.

> +    if (isSetCookieHeader(name) &&
!document()->securityOrigin()->canLoadLocalResources()) {
> +	   reportUnsafeUsage(document(), "Refused to get unsafe header \"" +
name + "\"");

I think this could have a comment explaining why we are doing this (which is
that we need to filter out HTTPOnly cookies, but that's hard, and Firefox trunk
also doesn't try to). Similarly, it may be better to explicitly test for
HTTPOnly cookies being hidden from XHR, or at least to explain that some
failures of the test are not catastrophic.

r=me, assuming this builds and works on Windows.