Bug 23092 – Conditionalize CFNetwork-specific logic in WebKit.dll

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 26399 [details]
Patch to conditionalize CFNetwork/cURL logic

[Eric Seidel (no email)]

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.

[Brent Fulgham]

(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."

[Brent Fulgham]

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.

[Darin Adler]

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?

[Brent Fulgham]

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.

[Adam Roben (:aroben)]

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

[Brent Fulgham]

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.

[Adam Roben (:aroben)]

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.

[Darin Adler]

(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.

[Brent Fulgham]

Created attachment 26573 [details]
Updated per Adam's last round of suggestions.

Re-marking as + per aroben.

[Alexey Proskuryakov]

Committed revision 39763.

[Brent Fulgham]

*** Bug 18471 has been marked as a duplicate of this bug. ***