Bug 17837 – Separate Windows Networking into CFNetwork and Curl Files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

This patch splits some WebCore and WebKit files into generic, CFNetwork-specific, and Curl-Specific implementations of some network interface implementations.

It also extends the Curl authentication handle with some non-functional, but signature-compatible implementations that reduce the amount of conditional logic needed in WebKit/win.

[Brent Fulgham]

Indicate relationship with 17730.  This bug implements part of the changes intended by Bug 17837.

[Brent Fulgham]

Created attachment 19750 [details]
Split CFNetwork-specific code into separate files

This patch takes some CFNetwork-specific logic and moves the relevant code to new files.  Stubs for Curl versions of these routines are provided.

* Please note the use of 'svn cp' to create the CFNet files, as this produces diff's that appear to involve large changes, when it is simply a copy/delete.
* Note that the Cairo targets are not yet updated to use these new Curl files.  These changes will be committed as part of Bug 17730 once these changes are in place.

[Brent Fulgham]

Created attachment 19823 [details]
Patch moving logic to WebCore

This revision places some implementation in WebCore/platform/network/curl, which reduces the amount of #ifdef's needed in WebKit.  Since the patch no longer adds any files, the project files are removed from this patch resulting in a smaller set of changes.

[Adam Roben (:aroben)]

Comment on attachment 19823 [details]
Patch moving logic to WebCore

+#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().cfURLAuthChallengeRef(), 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.

[Brent Fulgham]

Created attachment 19843 [details]
Revision 2 based on aroben's review

* No longer construct dummy authentication-challenge object.  Default constructed 'blank' object will function properly.
* Return E_FAIL for the case of attempt to create authentication/challenge pair in non-CFNetwork mode.

[Adam Roben (:aroben)]

Comment on attachment 19843 [details]
Revision 2 based on aroben's review

I have the same comments as before about the WebCore changes. Plus you don't need to change AuthenticationChallenge.h at all anymore.

[Brent Fulgham]

Created attachment 19845 [details]
Revision 3 based on aroben's further comments.

* Removed unused constructor from curl/AuthenticationChallenge.h
* However:  Retained implementation of get and member variable so that normal use of AuthenticationChallenge object does not have to be commented out for Curl case.  This will eventually be used for 'real stuff'.
* Cleaned up formatting.
* Removed some unneeded "wtf/RetainPtr" includes.

[Adam Roben (:aroben)]

Comment on attachment 19845 [details]
Revision 3 based on aroben's further comments.

I think this looks OK. I still haven't decided what I think about having these #ifdefs in WebKit. Maybe we can find some way of pushing the required functionality down into WebCore to avoid the #ifdef in WebKit.

r=me

[Brent Fulgham]

(In reply to comment #8)
> I think this looks OK. I still haven't decided what I think about having these
> #ifdefs in WebKit. Maybe we can find some way of pushing the required
> functionality down into WebCore to avoid the #ifdef in WebKit.
> 

I think that once the Curl implementation of some of these functions is up and running we can better determine how to move forward.  Right now the main obstacle to moving them to WebCore is that there are some member variables in the WebKit classes.  E.g., WebURLResponse holds onto an SSL certificate (dictionary ref) to ensure that this stays valid for the lifetime of the response.

[Matt Lilek]

Committed revision 31141.
=