[webkit-reviews] review denied: [Bug 17730] Allows Windows version to use Curl : [Attachment 19625] now, includes ChangeLog

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Daniel Zucker
<zucker at wake3.com>'s request for review:
Bug 17730: Allows Windows version to use Curl
http://bugs.webkit.org/show_bug.cgi?id=17730

Attachment 19625: now, includes ChangeLog
http://bugs.webkit.org/attachment.cgi?id=19625&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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.

+2008-03-09  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.