[webkit-reviews] review denied: [Bug 63674] Get webkit to compile with USE(CFNETWORK) enabled on Mac : [Attachment 99199] Patch 2 - the rest

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Pratik Solanki
<psolanki at apple.com>'s request for review:
Bug 63674: Get webkit to compile with USE(CFNETWORK) enabled on Mac
https://bugs.webkit.org/show_bug.cgi?id=63674

Attachment 99199: Patch 2 - the rest
https://bugs.webkit.org/attachment.cgi?id=99199&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99199&action=review

There are enough questions here that I think this needs another patch (and
could be broken up into smaller pieces if desired).

> Source/WebCore/platform/network/cf/CredentialStorageCFNet.cpp:42
> +#if PLATFORM(MAC)
> +#include "WebCoreSystemInterface.h"
> +#endif
> +
> +#if PLATFORM(WIN)
> +#include <WebKitSystemInterface/WebKitSystemInterface.h>
> +#endif

Use #elif PLATFORM(WIN) here to match similar code in other files.

> Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:71
> +	       // FIXME: Like Windows, we sleep for 10ms. Ideally, we should
have the loader thread

Need a radar or bugs.webkit.org bug covering this FIXME and referenced in the
comment.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:760
>      return RetainPtr<CFURLStorageSessionRef>(AdoptCF,
wkCreatePrivateStorageSession(identifier, defaultStorageSession()));
> +#else
> +    return RetainPtr<CFURLStorageSessionRef>(AdoptCF,
wkCreatePrivateStorageSession(identifier));

This may suffer from the same issue jessieberlin pointed out above.  You may
want to check with her about this as I'm not as familiar with the private
browsing code.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:292
> +#if USE(CFNETWORK)
> +    // FIXME: Need to implement this.
> +#else

Need a bugs.webkit.org bug reference here.

> Source/WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:138
> +#if USE(CFNETWORK)
> +    // FIXME: Needs to be implemented.
> +#else

Need a bugs.webkit.org bug reference here.

>> Source/WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:188
>>	[authenticationChallenge.sender() useCredential:mac(credential)
forAuthenticationChallenge:authenticationChallenge.nsURLAuthenticationChallenge
()];
> 
> Is this going to be handled somewhere else for the CFNetwork on Mac version?

Need a bugs.webkit.org bug reference here.

>> Source/WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:195
>>	[authenticationChallenge.sender()
continueWithoutCredentialForAuthenticationChallenge:authenticationChallenge.nsU
RLAuthenticationChallenge()];
> 
> Ditto.

And here.

>> Source/WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:202
>>	[authenticationChallenge.sender()
cancelAuthenticationChallenge:authenticationChallenge.nsURLAuthenticationChalle
nge()];
> 
> Ditto.

And here.