Bug 17730 – Allows Windows version to use Curl

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

[Daniel Zucker]

Support for Curl in Windows-Cairo build

[Daniel Zucker]

Created attachment 19615 [details]
patch to allow curl in windows-cario build

[Daniel Zucker]

Created attachment 19616 [details]
webcore.vcproj file

[Daniel Zucker]

Created attachment 19625 [details]
now, includes ChangeLog

[Adam Roben (:aroben)]

Comment on attachment 19625 [details]
now, includes ChangeLog

Please read over <http://webkit.org/coding/coding-style.html> if you haven't already. Many of the comments below are due to style problems.

[phone]  U-IBM-X30\Daniel Zucker  <set EMAIL_ADDRESS environment variable>

You should set the CHANGE_LOG_NAME and CHANGE_LOG_EMAIL_ADDRESS environment variables so that this first line will be generated correctly.

We typically put comments by each file/function we've modified to explain the changes. This is particularly important for changes to .vcproj/.sln files where it's very hard to read the diff.

-#define WTF_PLATFORM_CG 1
-#undef WTF_PLATFORM_CAIRO
-#define WTF_USE_CFNETWORK 1
+//DFZ
+//#define WTF_PLATFORM_CG 1
+//#undef WTF_PLATFORM_CAIRO
+#undef WTF_PLATFORM_CG
+#define WTF_PLATFORM_CAIRO 1
+//#define WTF_USE_CFNETWORK 1
+#undef WTF_USE_CFNETWORK
+#define WTF_USE_CURL 1

This change will break the CG-based builds. We need some way of having both configurations co-existing. We also don't land commented-out code.

+				<Filter
+					Name="curl"
+					>
+					<File
+						RelativePath="..\platform\network\curl\AuthenticationChallenge.h"
+						>
+					</File>
+					<File
+						RelativePath="..\platform\network\curl\ResourceError.h"
+						>
+					</File>
+					<File
+						RelativePath="..\platform\network\curl\ResourceHandleCurl.cpp"
+						>
+					</File>
+					<File
+						RelativePath="..\platform\network\curl\ResourceHandleManager.cpp"
+						>
+					</File>
+					<File
+						RelativePath="..\platform\network\curl\ResourceHandleManager.h"
+						>
+					</File>
+					<File
+						RelativePath="..\platform\network\curl\ResourceRequest.h"
+						>
+					</File>
+					<File
+						RelativePath="..\platform\network\curl\ResourceResponse.h"
+						>
+					</File>
+				</Filter>

Every file in this new filter needs to be excluded from the three CG-based configurations: Debug, Release, and Debug_Internal.

@@ -118,18 +118,22 @@ String cookies(const Document* /*documen
         return String();
 
     Vector<UChar> buffer(count);
-    InternetGetCookie(buffer.data(), 0, buffer, &count);
-    buffer.shrink(count - 1); // Ignore the null terminator.
+    InternetGetCookie(str.charactersWithNullTermination(), 0, buffer.data(), &count);
+	buffer.shrink(count - 1); // Ignore the null terminator.
     return String::adopt(buffer);
 #endif

Please put the justification for this change in your ChangeLog.

Please put the justification for the ForEachCoClass.h changes in you ChangeLog.

+++ WebKit/win/WebDataSource.cpp	(working copy)
@@ -27,6 +27,8 @@
 #include "WebKitDLL.h"
 #include "WebDataSource.h"
 
+#include <wtf/RetainPtr.h>
+
 #include "WebKit.h"
 #include "MemoryStream.h"
 #include "WebDocumentLoader.h"

This new #include is misplaced. It should be placed in ASCII order with the second paragraph of #includes. Ditto for WebError.cpp, WebMutableURLRequest.cpp, WebURLAuthenticationChallenge.cpp.

@@ -204,11 +205,13 @@ HRESULT STDMETHODCALLTYPE WebError::sslP
         return E_POINTER;
     *result = 0;
 
-    if (!m_cfErrorUserInfoDict) {
+#if USE(CFNETWORK)
+	if (!m_cfErrorUserInfoDict) {
         // copy userinfo from CFErrorRef
         CFErrorRef cfError = m_error;
         m_cfErrorUserInfoDict.adoptCF(CFErrorCopyUserInfo(cfError));
     }
+#endif

It looks like the indentation of the if line is wrong. We use 4-space indents, no tabs. This occurs elsewhere throughout your patch.

+#include "notImplemented.h"

The case of the filename is "NotImplemented.h". Please change your #includes to match.

+++ WebKit/win/WebKit.vcproj/WebKit.sln	(working copy)

It looks like this patch adds WinLauncher.vcproj to WebKit.sln. I believe we already have another bug for doing that, so I'd rather it not be included in this patch.

I'm not sure it's good to have so many configuration-specific #ifs in WebKit source files. I think we need to figure out some better way of handling this.

Thanks for the patch!. r- so that the style issues can be fixed and the ChangeLogs can be made more thorough, and so that we can sort out the issue of the configuration-specific WebKit code.

[Brent Fulgham]

Dan -- welcome to the world of peer-reviewed source patches!  Don't lose hope -- the result will be better than you expect, and well worth the revisions.

WebKit/win/WebError.cpp (WebError::sslPeerCertificate):
In addition to the region you alread excluded, I notice that a few lines further down we have a call to 'wkGetSSLPeerCertificateData'.  This is an internal platform API which we should also excluded.

I have discovered the same problem with some of my CoreGraphics changes, as there are a few font-handling routines in the 'wk' namespace I must get rid of.

We might want to break this into three files:  WebError (common stuff), WebErrorCF (CFNetwork stuff) and WebErrorCurl.

WebKit/win/WebFrame.cpp:
Again, we have a fairly small chunk of this file that is CFNetwork-specific.  Maybe we should move the download chunk into its own file (WebDownloadCF.cpp and WebDownloadCurl.cpp)?

It also looks like some of my temporary CoreGraphics diff is in this file, as the "spoolPages" has a #if(CG) around it when you use my manual patch; it looks like the tail end of that is in this revision.

WebKit/win/WebMutableURLRequest.cpp:
This whole patch might go away if we just stub out something in the Curl-based ResourceHandle to issue a "NotImplemented" when calling ResourceHandle::setHostAllowsAnyHTTPSCertificate.

Similarly, maybe we should move the certificate stuff into its own file so that we can just stub out the Curl things until we have valid implementations.

WebKit/win/WebURLAuthenticationChallenge.cpp:
WebKit/win/WebURLAuthenticationChallengeSender.cpp:
It seems like both of these changes could go away if we provided stub implementations in the WebURLAuthenticationChallenge stuff.

WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:
ditto above

[Brent Fulgham]

(In reply to comment #4)
> It looks like this patch adds WinLauncher.vcproj to WebKit.sln. I believe we
> already have another bug for doing that, so I'd rather it not be included in
> this patch.

See http://bugs.webkit.org/show_bug.cgi?id=17715.

[Daniel Zucker]

Created attachment 19640 [details]
updated Curl patch

Fixes Problems identified by Aroben:
(does not yet include response to bfulgham's comments)

You should set the CHANGE_LOG_NAME and CHANGE_LOG_EMAIL_ADDRESS environment
variables so that this first line will be generated correctly.

>>done

This change will break the CG-based builds. We need some way of having both
configurations co-existing. We also don't land commented-out code.

>>done.  config.h is reverted.

Every file in this new filter needs to be excluded from the three CG-based
configurations: Debug, Release, and Debug_Internal.

>>done

Please put the justification for this change in your ChangeLog.

>>done

Please put the justification for the ForEachCoClass.h changes in you ChangeLog.

>>done

This new #include is misplaced. It should be placed in ASCII order with the
second paragraph of #includes. Ditto for WebError.cpp,
WebMutableURLRequest.cpp, WebURLAuthenticationChallenge.cpp.

>>done


It looks like the indentation of the if line is wrong. We use 4-space indents,
no tabs. This occurs elsewhere throughout your patch.

>>done.  I fixed all the problem areas I could find.  Please let me know if I missed any.

The case of the filename is "NotImplemented.h". Please change your #includes to
match.

>>done

It looks like this patch adds WinLauncher.vcproj to WebKit.sln. I believe we
already have another bug for doing that, so I'd rather it not be included in
this patch.

>>done (removed WinLauncher.vcproj)

[Brent Fulgham]

Portion of this patch moved to 17783.

[Brent Fulgham]

Please note the changes in http://bugs.webkit.org/show_bug.cgi?id=17788, which split CookieJarWin.cpp.  The Cairo (Curl) build will need to compile CookieJarWin.cpp, rather than the new CookieJarCFNetWin.cpp which is now the default in the project file.

[Brent Fulgham]

Please see http://bugs.webkit.org/show_bug.cgi?id=17837 for another sub-patch of this work, revised to use multiple files (rather than #ifdef's).

[Brent Fulgham]

Please note that a final set of VCPROJ updates have been presented under http://bugs.webkit.org/show_bug.cgi?id=17985 for review.

[Daniel Zucker]

Due to the other subsequent patches submitted (See Bug 17730 depends on:..), the attached patch is obsolete.  However, all dependent bugs must be fixed to get the build to work with Curl.  As of the time of this comment, bug 18468 is still open.

[Brent Fulgham]

This bug is no longer relevant.  The cURL components are now fully building and running under the redistributable Windows port.