This patch places #if/def conditionals around CFNetwork-specific logic and provides initial stubs for the Windows cURL backend. Where possible, the conditional regions are on function boundaries. The plan is for a follow-up patch will move these conditional regions into separate files to properly separate these areas.
Created attachment 26399 [details] Patch to conditionalize CFNetwork/cURL logic
Comment on attachment 26399 [details] Patch to conditionalize CFNetwork/cURL logic I wonder why the Curl-Windows port wouldn't want to just fork these files.
(In reply to comment #2) > (From update of attachment 26399 [details] [review]) > I wonder why the Curl-Windows port wouldn't want to just fork these files. > Yes, that's phase II: "The plan is for a follow-up patch will move these conditional regions into separate files to properly separate these areas."
Created attachment 26511 [details] Move CFNetwork-specific logic to separate files. This patch moves the CFNetwork-specific logic into separate files, and provides initial cURL-based stub files to be filled in with logic in a subsequent update. * A handful of files were not split, as they only had one or two lines that had to be conditionally compiled. E.g., WebURLAuthenticationChallenge. An cURL-specific AuthenticationChallenge object should render this uniform across both implementations. * Project file has been updated so the CFNet files are excluded for cURL builds, and vice versa. The patch has been tested against my Cairo/cURL build and a stock Apple Safari-based build.
Comment on attachment 26511 [details] Move CFNetwork-specific logic to separate files. For simpler reviewing, the XXXCFNet.cpp files should start out with an "svn copy" of the original file. This allows us to see the changes in those files, too. You can do this after the fact by moving the file aside, doing an svn revert, then the svn copy, then move the file back into place. Would you be willing to do that?
Created attachment 26516 [details] Update using svn copy to create new CFNet versions This patch moves the CFNetwork-specific logic into separate files, and provides initial cURL-based stub files to be filled in with logic in a subsequent update. * A handful of files were not split, as they only had one or two lines that had to be conditionally compiled. E.g., WebURLAuthenticationChallenge. An cURL-specific AuthenticationChallenge object should render this uniform across both implementations. * Project file has been updated so the CFNet files are excluded for cURL builds, and vice versa. The patch has been tested against my Cairo/cURL build and a stock Apple Safari-based build.
Comment on attachment 26516 [details] Update using svn copy to create new CFNet versions > Index: WebKit/win/WebDownload.cpp > =================================================================== > --- WebKit/win/WebDownload.cpp (revision 39682) > +++ WebKit/win/WebDownload.cpp (working copy) > @@ -38,12 +38,16 @@ > #include "WebURLCredential.h" > #include "WebURLResponse.h" > > +#include <wtf/platform.h> I'm surprised you need to include this. I thought config.h pulled it in. In any case, "Platform" should be capitalized, and this header should be in ASCII order with the others (unless there's a reason why it can't be, in which case a comment would be good). > +#if USE(CFNETWORK) > #include <WebCore/AuthenticationCF.h> > +#endif I'm surprised this #include is still needed in WebDownload.cpp after these changes. > + static const WebCore::String BundleExtension; > + static const UInt32 BundleMagicNumber; I think a slightly better approach would be to make some static getter functions to return these values. The main advantage of this is that it won't require the BundleExtension String to be constructed when the DLL is first loaded (a "static initializer"). Whether or not you turn these into getter functions, they should begin with lowercase letters. r=me
Created attachment 26568 [details] Revised previous patch per Adam's comments. A few small revisions: 1. Remove unneeded "#include <wtf/platform.h>" and #"include <WebCore/AuthenticationCF.h>" from WebKit/win/WebDownload.cpp 2. Change BundleExtension to static method. 3. Change BundleMagicNumber to static method. 4. Use DEFINE_STATIC_LOCAL macro for bundleExtension definition.
Comment on attachment 26568 [details] Revised previous patch per Adam's comments. > -HRESULT STDMETHODCALLTYPE WebCookieManager::setCookieStorage( > - /* [in] */ CFHTTPCookieStorageRef storage) > -{ > - setCurrentCookieStorage(storage); > - return S_OK; > -} > +} > \ No newline at end of file You should add a newline before committing. > +UInt32 WebDownload::bundleMagicNumber() > +{ > + static UInt32 bundleMagicNumber = 0xDECAF4EA; > + return bundleMagicNumber; > +} We could probably just say "return 0xDECAF4EA", but it doesn't matter too much. It's probably possible to trim down the #includes in the new *Curl.cpp files. r=me I don't think any of the changes I suggested will require a re-review.
(In reply to comment #9) > > +UInt32 WebDownload::bundleMagicNumber() > > +{ > > + static UInt32 bundleMagicNumber = 0xDECAF4EA; > > + return bundleMagicNumber; > > +} > > We could probably just say "return 0xDECAF4EA", but it doesn't matter too much. If you said "static const UInt32" then in gcc it would create more efficient code. gcc fails to realize this number can't be modified and will actually generate a global variable without the const.
Created attachment 26573 [details] Updated per Adam's last round of suggestions. Re-marking as + per aroben.
Committed revision 39763.
*** Bug 18471 has been marked as a duplicate of this bug. ***