Bug 17837

Summary: Separate Windows Networking into CFNetwork and Curl Files
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: zucker
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 17730    
Attachments:
Description Flags
Split CFNetwork-specific code into separate files
none
Patch moving logic to WebCore
aroben: review-
Revision 2 based on aroben's review
aroben: review-
Revision 3 based on aroben's further comments. aroben: review+

Description Brent Fulgham 2008-03-13 16:09:10 PDT
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.
Comment 1 Brent Fulgham 2008-03-13 16:57:22 PDT
Indicate relationship with 17730.  This bug implements part of the changes intended by Bug 17837.
Comment 2 Brent Fulgham 2008-03-13 17:04:31 PDT
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.
Comment 3 Brent Fulgham 2008-03-16 21:03:40 PDT
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.
Comment 4 Adam Roben (:aroben) 2008-03-17 11:48:07 PDT
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.
Comment 5 Brent Fulgham 2008-03-17 12:19:57 PDT
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.
Comment 6 Adam Roben (:aroben) 2008-03-17 12:29:18 PDT
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.
Comment 7 Brent Fulgham 2008-03-17 12:52:25 PDT
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.
Comment 8 Adam Roben (:aroben) 2008-03-18 10:34:47 PDT
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
Comment 9 Brent Fulgham 2008-03-18 11:27:34 PDT
(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.
Comment 10 Matt Lilek 2008-03-18 16:23:33 PDT
Committed revision 31141.
=