[webkit-reviews] review denied: [Bug 17837] Separate Windows Networking into CFNetwork and Curl Files : [Attachment 19823] Patch moving logic to WebCore

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Brent Fulgham
<bfulgham at gmail.com>'s request for review:
Bug 17837: Separate Windows Networking into CFNetwork and Curl Files
http://bugs.webkit.org/show_bug.cgi?id=17837

Attachment 19823: Patch moving logic to WebCore
http://bugs.webkit.org/attachment.cgi?id=19823&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
+#if PLATFORM(WIN) && USE(CURL)
     static void setHostAllowsAnyHTTPSCertificate(const String&);
     static void setClientCertificate(const String& host, CFDataRef);
 #endif

You should probably include #if PLATFORM(CF) here as well, since you're using
CFDataRef.

+#if PLATFORM(WIN)
+// FIXME:  The CFDataRef will need to be something else when
+// building without CoreFoundation
+static HashMap<String, RetainPtr<CFDataRef> >& clientCerts()
+{
+    static HashMap<String, RetainPtr<CFDataRef> > certs;
+    return certs;
+}
+
+void ResourceHandle::setClientCertificate(const String& host, CFDataRef cert)
+{
+    clientCerts().set(host.lower(), cert);
+}
+#endif

Ditto here.

+++ WebKit/win/WebDataSource.cpp	(working copy)
@@ -46,6 +46,7 @@
 #include <WebCore/FrameLoader.h>
 #include <WebCore/KURL.h>
 #pragma warning(pop)
+#include <wtf/RetainPtr.h>

Why is this needed?

+++ WebKit/win/WebError.cpp	(working copy)
@@ -28,7 +28,9 @@
 #include "WebError.h"
 #include "WebKit.h"
 
+#if USE(CFNETWORK)
 #include <WebKitSystemInterface/WebKitSystemInterface.h>
+#endif
 #pragma warning(push, 0)
 #include <WebCore/BString.h>
 #pragma warning(pop)

We normally put headers that are conditionally included in their own paragraph
after all the unconditional headers.

+++ WebKit/win/WebURLAuthenticationChallenge.cpp	(working copy)
@@ -40,6 +40,7 @@
 #include <WebCore/BString.h>
 #include <WebCore/ResourceHandle.h>
 #pragma warning(pop)
+#include <wtf/RetainPtr.h>

Why is this needed?

@@ -168,7 +169,11 @@ HRESULT STDMETHODCALLTYPE WebURLAuthenti
     if (!webSender)
	 return E_NOINTERFACE;
 
+#if USE(CFNETWORK)
     m_authenticationChallenge =
AuthenticationChallenge(webChallenge->authenticationChallenge().cfURLAuthChalle
ngeRef(), webSender->resourceHandle());
+#else
+    m_authenticationChallenge =
AuthenticationChallenge(webSender->resourceHandle());
+#endif

I think it would be better to return E_FAIL here and not modify
m_authenticationChallenge. Then you can remove the changes to
AuthenticationChallenge

r- for now.