WebCore should use CFNetwork-based loader on Mac

• Component: Page Loading
• Status: RESOLVED WONTFIX

[Pratik Solanki]

On Mac, the WebCore code uses Foundation (NSURLRequest, NSURLConnection, etc.) for network. On Windows, it uses CFNetwork directly. It would be better for bug fixing and maintainability if we used CFNetwork on both platforms.

[Pratik Solanki]

rdar://problem/5575737

[Pratik Solanki]

Created attachment 83207 [details]
Patch 1 - Gets code compiling and running on Mac

[Darin Adler]

Pratik, I think we can do this when running WebKit2, but it would break API compatibility to do it when running WebKit1.

[Brady Eidson]

(In reply to comment #3)
> Pratik, I think we can do this when running WebKit2, but it would break API compatibility to do it when running WebKit1.

This is definitely true, but I don't know how big a task it would be to get WebCore building with both loaders, which is what we'd have to do to support that - since the same WebCore supports both WK1 and WK2.

[Pratik Solanki]

(In reply to comment #4)
> (In reply to comment #3)
> > Pratik, I think we can do this when running WebKit2, but it would break API compatibility to do it when running WebKit1.

Well, thats why I have wrapper objects exposed from WebCore to WebKit so that we don't break the API. I haven't tested client other than Safari but it should work.

> This is definitely true, but I don't know how big a task it would be to get WebCore building with both loaders, which is what we'd have to do to support that - since the same WebCore supports both WK1 and WK2.

Yeah, having a runtime switch to toggle between the two would be a huge task

[Build Bot]

Attachment 83207 [details] did not build on win:
Build output: http://queues.webkit.org/results/7939855

[Darin Adler]

Comment on attachment 83207 [details]
Patch 1 - Gets code compiling and running on Mac

View in context: https://bugs.webkit.org/attachment.cgi?id=83207&action=review

Looks generally good. Needs some work. In particular, has a few storage leaks.

> Source/WebCore/WebCore.exp.in:291
> +#if USE(CFNETWORK)
> +__ZN7WebCore12SchedulePairC1EP11__CFRunLoopPK10__CFString
> +#else
>  __ZN7WebCore12SchedulePairC1EP9NSRunLoopPK10__CFString
> +#endif

We put conditional includes in their own paragraphs rather than sorting them alphabetically in the unconditional includes. See the bottom of the file to see what I mean. These should be moved to the bottom of the file.

> Source/WebCore/loader/mac/ResourceLoaderMac.mm:37
> +@interface NSCachedURLResponse (FoundationSecretsWebCoreKnowsAbout)

The term “secrets” is not appropriate here. Please use Details or something along those lines, as in ResourceHandleMac.mm.

> Source/WebCore/loader/mac/ResourceLoaderMac.mm:60
> +    NSCachedURLResponse *nsCachedResponse = [[[NSCachedURLResponse alloc] _initWithCFCachedURLResponse:cachedResponse] autorelease];

It’s better to just release this after using it than to use autorelease. Or you could use RetainPtr.

> Source/WebCore/loader/mac/ResourceLoaderMac.mm:62
> +    NSCachedURLResponse *willCacheResponse = frameLoader()->client()->willCacheResponse(documentLoader(), identifier(), nsCachedResponse);
> +    return [willCacheResponse _CFCachedURLResponse];

I think we’d be better off without the local variable here, not sure.

> Source/WebCore/platform/cf/SchedulePair.h:50
> +#if PLATFORM(MAC) &&!USE(CFNETWORK)

Missing space here after "&&".

> Source/WebCore/platform/mac/SchedulePairMac.mm:35
> +#if !USE(CFNETWORK)

Not sure why you put the #if further inside here than CookieJar.mm.

> Source/WebCore/platform/mac/SoftLinking.h:48
> +#define SOFT_LINK_CORE_FRAMEWORK(framework) \

I don’t think the name here is great. I suggest a more specific name like: SOFT_LINK_FRAMEWORK_IN_CORESERVICES_UMBRELLA.

> Source/WebCore/platform/mac/SoftLinking.h:86
> +#define SOFT_LINK_OPTIONAL(framework, functionName, resultType, parameterDeclarations) \
> +    typedef resultType (*functionName##PtrType) parameterDeclarations; \
> +    \
> +    static functionName##PtrType functionName##Ptr() \
> +    { \
> +        static functionName##PtrType ptr; \
> +        static bool initialized; \
> +        \
> +        if (initialized) \
> +            return ptr; \
> +        initialized = true; \
> +        \
> +        ptr = reinterpret_cast<functionName##PtrType>(dlsym(framework##Library(), #functionName)); \
> +        return ptr; \
> +    }

Whoever wrote the Windows macro you based this on made it unnecessarily complicated. It can just be:

    static functionName##PtrType pointer = reinterpret_cast<functionName##PtrType>(dlsym(framework##Library(), #functionName));
    return pointer;

No need for the initialized boolean.

> Source/WebCore/platform/network/ResourceHandle.h:126
> +#if USE(CFNETWORK)
> +    CFURLConnectionRef connection() const;
> +    CFURLConnectionRef releaseConnectionForDownload();
> +    static void setHostAllowsAnyHTTPSCertificate(const String&);
> +    static void setClientCertificate(const String& host, CFDataRef);
> +#endif

I’m not sure you should have moved this when changing the #if. The diff would read better if you changed the #ifdefing but didn’t move things around.

> Source/WebCore/platform/network/ResourceHandleClient.h:35
> +#include <Availability.h>

Is this include really needed?

> Source/WebCore/platform/network/ResourceHandleInternal.h:129
>              , m_needsSiteSpecificQuirks(false)

Is this really NSURL-specific? I think you are removing a feature that is probably needed on Mac.

> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:43
>  #include <CoreFoundation/CoreFoundation.h>
> +#if PLATFORM(WIN)
>  #include <WebKitSystemInterface/WebKitSystemInterface.h>
>  #include <windows.h>
> +#endif

Conditional includes should go in a separate paragraph. Please add a blank line.

> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:72
> +#if PLATFORM(WIN)
>  SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieCopyDomain, CFStringRef, __cdecl, (CFHTTPCookieRef))
>  SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieGetExpirationTime, CFAbsoluteTime, __cdecl, (CFHTTPCookieRef))
>  SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieCopyName, CFStringRef, __cdecl, (CFHTTPCookieRef))
>  SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieCopyPath, CFStringRef, __cdecl, (CFHTTPCookieRef))
>  SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieCopyValue, CFStringRef, __cdecl, (CFHTTPCookieRef))
> +#else
> +SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieCopyDomain, CFStringRef, (CFHTTPCookieRef))
> +SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieGetExpirationTime, CFAbsoluteTime, (CFHTTPCookieRef))
> +SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieCopyName, CFStringRef, (CFHTTPCookieRef))
> +SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieCopyPath, CFStringRef, (CFHTTPCookieRef))
> +SOFT_LINK_OPTIONAL(CFNetwork, CFHTTPCookieCopyValue, CFStringRef, (CFHTTPCookieRef))
> +#endif

Sure would be nice if we could design this macro so we didn’t need this #if.

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

Again, conditional includes should go in their own paragraph.

> Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:46
>  #include <CoreFoundation/CFStreamAbstract.h>
> +#if PLATFORM(MAC)
> +#include "WebCoreSystemInterface.h"
> +#endif
> +#if PLATFORM(WIN)
>  #include <WebKitSystemInterface/WebKitSystemInterface.h>
> +#endif
>  #include <sys/types.h>

Again, conditional includes should go in their own paragraph.

> Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:68
>          while (!loaderRunLoopObject) {
> +#if PLATFORM(WIN)
>              // FIXME: Sleep 10? that can't be right...
>              Sleep(10);
> +#else
> +            sleep(1);
> +#endif
>          }

I think this needs a comment. Why is sleep(1) correct?

> Source/WebCore/platform/network/cf/ResourceError.h:68
> +    ResourceError(NSError* error)

This is not our conventional formatting for Objective-C class names.

I think the definition of this constructor should go at the end of the file, rather than right here in the class.

> Source/WebCore/platform/network/cf/ResourceError.h:70
> +        , m_platformError((CFErrorRef) error)

Strange to see a C-style cast here.

> Source/WebCore/platform/network/cf/ResourceError.h:72
> +        m_isNull = !error;

I suggest using initialization rather than assignment.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:65
> +#if PLATFORM(MAC)
> +#include <CFNetwork/CFURLConnectionPriv.h>
> +#endif
> +#if PLATFORM(WIN)
>  #include <WebKitSystemInterface/WebKitSystemInterface.h>
>  #include <process.h> // for _beginthread()
> +#endif
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  #include <wtf/HashMap.h>
> +#include <wtf/StdLibExtras.h>
>  #include <wtf/Threading.h>
>  #include <wtf/text/CString.h>
> +#if PLATFORM(MAC)
> +#include "WebCoreSystemInterface.h"
> +#endif

Again, conditional includes should get their own paragraph.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:128
> -    static HashMap<String, RetainPtr<CFDataRef> > certs;
> +    static HashMap<String, RetainPtr<CFDataRef> >* certsPtr = new HashMap<String, RetainPtr<CFDataRef> >();
> +    HashMap<String, RetainPtr<CFDataRef> >& certs = *certsPtr;

Why can’t you use DEFINE_STATIC_LOCAL for this?

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:146
> +    UNUSED_PARAM(conn);

In future versions of clang this won’t work, because we can’t just say UNUSED_PARAM and then later use the parameter. One way to avoid that would be:

    #if !LOG_ENABLED
    UNUSED_PARAM(conn);
    #endif

Another would be to remove the logging and leave the argument unnamed. Not sure it’s all that useful.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:286
> +#if PLATFORM(WIN)
>      if (handle->client() && !handle->client()->shouldCacheResponse(handle, cachedResponse))
>          return 0;
> +#else
> +    CFCachedURLResponseRef newResponse = handle->client()->willCacheResponse(handle, cachedResponse);
> +    if (newResponse != cachedResponse)
> +        return newResponse;
> +#endif

This is kind of a crazy platform difference. Can we unify the platforms?

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:718
>  }
> +void ResourceHandle::schedule(SchedulePair* pair)

Missing blank line here.

> Source/WebCore/platform/network/cf/ResourceRequest.h:74
> +        ResourceRequest(NSURLRequest* nsRequest);

No need for an argument name here.

> Source/WebCore/platform/network/cf/ResourceResponse.h:59
> +    ResourceResponse(NSURLResponse* nsResponse);

No need for an argument name here.

> Source/WebCore/platform/network/mac/AuthenticationMac.mm:41
> +@interface NSURLProtectionSpace (FoundationSecretsWebCoreKnowsAbout)

Should not use the word “secret” here. See suggestion above.

> Source/WebCore/platform/network/mac/AuthenticationMac.mm:46
> +@interface NSURLAuthenticationChallenge (FoundationSecretsWebCoreKnowsAbout)

Should not use the word “secret” here. See suggestion above.

> Source/WebCore/platform/network/mac/AuthenticationMac.mm:51
> +@interface NSURLCredential (FoundationSecretsWebCoreKnowsAbout)

Should not use the word “secret” here. See suggestion above.

> Source/WebCore/platform/network/mac/AuthenticationMac.mm:153
> +    WebCoreAuthenticationClientAsChallengeSender *challengeSender = [[WebCoreAuthenticationClientAsChallengeSender alloc] initWithAuthenticationClient:authClient];

This leaks, because it’s allocated but not released.

> Source/WebCore/platform/network/mac/AuthenticationMac.mm:155
> +    return [[NSURLAuthenticationChallenge _authenticationChallengeForCFAuthChallenge:createCF(coreChallenge)
> +                                                                              sender:challengeSender] autorelease];

We don’t line up multiple lines of Objective-C method calls this way. It’s fragile because if you rename anything it’s not lined up any more. We just put it on one line or indent four spaces.

Given the name createCF, either this code leaks (creating a challenge and not releasing it), or the createCF function is misnamed.

> Source/WebCore/platform/network/mac/AuthenticationMac.mm:160
> +    return [[[NSURLCredential alloc] _initWithCFURLCredential:createCF(coreCredential)] autorelease];

Given the name createCF, either this code leaks (creating a credential and not releasing it), or the createCF function is misnamed.

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:50
> +                           userInfo:(NSDictionary*)CFErrorCopyUserInfo(error)];

This leaks the user info dictionary, because it calls a Copy function but never CFRelease.

> Source/WebCore/platform/network/mac/ResourceRequestMac.mm:48
> +@interface NSURLRequest (FoundationSecretsWebCoreKnowsAbout)

Again, not the “secret” word.

> Source/WebCore/platform/network/mac/ResourceRequestMac.mm:64
> +    if (!m_cfRequest)
> +        cfURLRequest();
> +    if (!m_nsRequest)
> +        const_cast<ResourceRequest*>(this)->m_nsRequest.adoptNS([[NSURLRequest alloc] _initWithCFURLRequest:m_cfRequest.get()]);
> +    return m_nsRequest.get();

There is no need to make m_cfRequest non-zero if m_nsRequest is already non-zero. It’s also ugly to call cfURLRequest just for its side effect.

Instead of using const_cast, m_nsRequest should be marked mutable in the header.

I suggest writing it like this:

    if (!m_nsRequest)
        m_nsRequest.adoptNS([[NSURLRequest alloc] _initWithCFURLRequest:cfURLRequest()]);
    return m_nsRequest.get();

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:36
>  @interface NSURLResponse (FoundationSecretsWebCoreKnowsAbout)

This category should be renamed for the same reasons I gave above for new code.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:54
> +    if (!m_cfResponse && !m_isNull) {

This function does a lot of work when m_nsResponse is already non-zero that should be unnecessary. It should be restructured to only do the m_cfResponse work if m_nsResponse is zero.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:59
> +        if (m_expectedContentLength < 0 || m_expectedContentLength > std::numeric_limits<NSInteger>::max())

We normally would put using namespace std in the header rather than using std:: here.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:64
> +        const_cast<ResourceResponse*>(this)->m_nsResponse.adoptNS([[NSURLResponse alloc] initWithURL:m_url MIMEType:m_mimeType expectedContentLength:expectedContentLength textEncodingName:m_textEncodingName]);
> +        const_cast<ResourceResponse*>(this)->m_cfResponse = [m_nsResponse.get() _CFURLResponse];

m_nsResponse and m_cfResponse should be marked mutable in the header instead of using const_cast here.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:71
> +        const_cast<ResourceResponse*>(this)->m_nsResponse.adoptNS([[NSURLResponse alloc] _initWithCFURLResponse:m_cfResponse.get()]);

m_nsResponse should be marked mutable in the header instead of using const_cast here.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:81
> +    m_isNull = !nsResponse;

Why not use initialization instead of assignment?

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:289
>  void WebFrameLoaderClient::download(ResourceHandle* handle, const ResourceRequest& request, const ResourceRequest& initialRequest, const ResourceResponse& response)
>  {
> +#if !USE(CFNETWORK)

Seems we’ll get unused argument warnings here.

> Source/WebKit/mac/WebView/WebView.mm:4364
> +#if USE(CFNETWORK)
> +    if (runLoop && mode)
> +        core(self)->addSchedulePair(SchedulePair::create([runLoop getCFRunLoop], (CFStringRef)mode));
> +#else
>      if (runLoop && mode)
>          core(self)->addSchedulePair(SchedulePair::create(runLoop, (CFStringRef)mode));
> +#endif

Would be nicer to cut down the ifdefs here with a local variable for the run loop argument.

> Source/WebKit/mac/WebView/WebView.mm:4375
> +#if USE(CFNETWORK)
> +    if (runLoop && mode)
> +        core(self)->removeSchedulePair(SchedulePair::create([runLoop getCFRunLoop], (CFStringRef)mode));
> +#else
>      if (runLoop && mode)
>          core(self)->removeSchedulePair(SchedulePair::create(runLoop, (CFStringRef)mode));
> +#endif

Would be nicer to cut down the ifdefs here with a local variable for the run loop argument.

> Source/WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:135
>  void Download::startWithHandle(WebPage* initiatingPage, ResourceHandle* handle, const ResourceRequest& initialRequest, const ResourceResponse& response)
>  {
> +#if !USE(CFNETWORK)

Seems we’ll get unused argument warnings here.

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Pratik, I think we can do this when running WebKit2, but it would break API compatibility to do it when running WebKit1.
> 
> Well, thats why I have wrapper objects exposed from WebCore to WebKit so that we don't break the API. I haven't tested client other than Safari but it should work.

That won’t work with applications that implement custom NSURLProtocol classes, for example. I don’t think this can be done without breaking API compatibility. Please talk to Maciej or Alexey about that.

[Alexey Proskuryakov]

What's the benefit of soft-linking CFNetwork?

Yes, it does look like it would break custom protocols.

[Pratik Solanki]

Darin, thanks for the detailed review. I'll update my patch.

(In reply to comment #8)
> That won’t work with applications that implement custom NSURLProtocol classes, for example. I don’t think this can be done without breaking API compatibility. Please talk to Maciej or Alexey about that.

Ok. I'll discuss this some more with Alexey.

(In reply to comment #9)
> What's the benefit of soft-linking CFNetwork?

Not sure. This was Windows code that I ported to Mac so as to have have fewer differences. I think CFNetwork would always be loaded on the Mac, right?

[Darin Adler]

(In reply to comment #10)
> (In reply to comment #9)
> > What's the benefit of soft-linking CFNetwork?
> 
> Not sure. This was Windows code that I ported to Mac so as to have have fewer differences. I think CFNetwork would always be loaded on the Mac, right?

The soft linking is not about CFNetwork itself, but about certain CFNetwork entry points. We have to use soft linking if the same binary needs to work on multiple versions of CFNetwork.

I don’t think there is any need for that on Mac, but we’d have to look at the specific entry points. It might be best to ask Jessie about this.

[Jessie Berlin]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > What's the benefit of soft-linking CFNetwork?
> > 
> > Not sure. This was Windows code that I ported to Mac so as to have have fewer differences. I think CFNetwork would always be loaded on the Mac, right?
> 
> The soft linking is not about CFNetwork itself, but about certain CFNetwork entry points. We have to use soft linking if the same binary needs to work on multiple versions of CFNetwork.
> 
> I don’t think there is any need for that on Mac, but we’d have to look at the specific entry points. It might be best to ask Jessie about this.

The CFHTTPCookieCopy___ functions may not be available on older versions of the system. Some older versions of the system might use CFHTTPCookieGet___ instead, or not have either available.

Therefore, soft linking is necessary to use these CFHTTPCookie___ functions on Mac. I suggest trying first to find the copy version of the function, fall back to the get version of the function, and bail if neither are found (though is bailing really an option?).

[Pratik Solanki]

Comment on attachment 83207 [details]
Patch 1 - Gets code compiling and running on Mac

View in context: https://bugs.webkit.org/attachment.cgi?id=83207&action=review

Thanks for the review. I'll have a new patch up with the comments addressed. Some responses below.

>> Source/WebCore/platform/network/ResourceHandleClient.h:35
>> +#include <Availability.h>
> 
> Is this include really needed?

No it doesn't look like its needed. I think the corresponding ConditionalMacros include on the Windows side is likely not needed as well.

>> Source/WebCore/platform/network/ResourceHandleInternal.h:129
>>              , m_needsSiteSpecificQuirks(false)
> 
> Is this really NSURL-specific? I think you are removing a feature that is probably needed on Mac.

Right. I would need to add supporting code in the CFNetwork side that honors that flag though. I'll do that in a later patch. But its okay to have the variable present when using CFNetwork.

>> Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:68
>>          }
> 
> I think this needs a comment. Why is sleep(1) correct?

I changed that to be a sleep for 10ms (like Windows) and added a comment. Ideally, we want the loader thread to signal to us that it is done setting the loaderRunLoopObject global. The sleep is a hack for this synchronization. Is there any other code in WebKit that does this kind of child thread synchronization and uses a better approach? I could fix this with pthread_cond_wait()/pthread_cond_signal() but I'm not sure if that's the recommended practice (or if it would work on Windows).

>> Source/WebCore/platform/network/cf/ResourceError.h:68
>> +    ResourceError(NSError* error)
> 
> This is not our conventional formatting for Objective-C class names.
> 
> I think the definition of this constructor should go at the end of the file, rather than right here in the class.

All the constructor definitions are in the class. Would you want me to move all of them to the end of the file?

>> Source/WebCore/platform/network/cf/ResourceError.h:70
>> +        , m_platformError((CFErrorRef) error)
> 
> Strange to see a C-style cast here.

I ended up using a dynamic_cast here instead. Using static_cast gave me errors.

>> Source/WebCore/platform/network/cf/ResourceError.h:72
>> +        m_isNull = !error;
> 
> I suggest using initialization rather than assignment.

m_isNull belongs to the super class ResourceErrorBase, so I couldn't use initialization.

>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:128
>> +    HashMap<String, RetainPtr<CFDataRef> >& certs = *certsPtr;
> 
> Why can’t you use DEFINE_STATIC_LOCAL for this?

Because of the comma in the type. The preprocessor thought I was passing 4 args to DEFINE_STATIC_LOCAL with arg 1 being HashMap<String and arg2 being RetainPtr<CFDataRef> >. But I realize that I can avoid this preprocessor error by using a typedef. Fixed patch.

>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:286
>> +#endif
> 
> This is kind of a crazy platform difference. Can we unify the platforms?

Yes it is an odd difference. I asked mitz about this since he wrote the original code for windows. He seemed to think he made a different method since all he wanted was a boolean indicating whether the response should be cached. He also said it may have been due to COM limitations. It would be good to unify them if possible. Perhaps worthy of a follow on bug?

>> Source/WebCore/platform/network/mac/ResourceRequestMac.mm:64
>> +    return m_nsRequest.get();
> 
> There is no need to make m_cfRequest non-zero if m_nsRequest is already non-zero. It’s also ugly to call cfURLRequest just for its side effect.
> 
> Instead of using const_cast, m_nsRequest should be marked mutable in the header.
> 
> I suggest writing it like this:
> 
>     if (!m_nsRequest)
>         m_nsRequest.adoptNS([[NSURLRequest alloc] _initWithCFURLRequest:cfURLRequest()]);
>     return m_nsRequest.get();

I restructured the code so that NSURLResponse is created and set when a CFURLResponseRef is created. That way, we can just use the Mac version of ResourceRequest::nsURLRequest().

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:36
>>  @interface NSURLResponse (FoundationSecretsWebCoreKnowsAbout)
> 
> This category should be renamed for the same reasons I gave above for new code.

This was the pre-existing category name which I "copied" around in my patch. I've changed all the category names to Details. Except for this one since we already have NSURLResponse (Details) in WebCoreURLResponse.h. So I named this one MoreDetails.

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:54
>> +    if (!m_cfResponse && !m_isNull) {
> 
> This function does a lot of work when m_nsResponse is already non-zero that should be unnecessary. It should be restructured to only do the m_cfResponse work if m_nsResponse is zero.

I added a check for m_nsResponse in this condition.

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:59
>> +        if (m_expectedContentLength < 0 || m_expectedContentLength > std::numeric_limits<NSInteger>::max())
> 
> We normally would put using namespace std in the header rather than using std:: here.

Ok. This code was copied over from the Mac version of nsURLResponse(). I ended up refactoring this into a separate private function so it could be used by both. And added "using namespace std" at the top.

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:81
>> +    m_isNull = !nsResponse;
> 
> Why not use initialization instead of assignment?

Because m_isNull is a member of super class ResourceResponseBase

>> Source/WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:135
>> +#if !USE(CFNETWORK)
> 
> Seems we’ll get unused argument warnings here.

Odd that I didn't see any warnings. Anyway, I updated this code so we won't get warnings.

[Alexey Proskuryakov]

> I could fix this with pthread_cond_wait()/pthread_cond_signal() but I'm not sure if that's the recommended practice (or if it would work on Windows).

You could use pthreads or ThreadCondition, but generally we just try to avoid having objects shared between threads.

[Darin Adler]

(In reply to comment #13)
> >> Source/WebCore/platform/network/cf/ResourceError.h:68
> >> +    ResourceError(NSError* error)
> > 
> > This is not our conventional formatting for Objective-C class names.
> > 
> > I think the definition of this constructor should go at the end of the file, rather than right here in the class.
> 
> All the constructor definitions are in the class. Would you want me to move all of them to the end of the file?

I don’t think you should move existing definitions as part of this same patch, but yes, I think end of the file is better even for the existing definitions. The only exception might be one that’s nearly empty and fits on one line, but even in that case, yes.

> >> Source/WebCore/platform/network/cf/ResourceError.h:70
> >> +        , m_platformError((CFErrorRef) error)
> > 
> > Strange to see a C-style cast here.
> 
> I ended up using a dynamic_cast here instead. Using static_cast gave me errors.

dynamic_cast is definitely wrong. Maybe we can look at this together.

> I restructured the code so that NSURLResponse is created and set when a CFURLResponseRef is created. That way, we can just use the Mac version of ResourceRequest::nsURLRequest().

Is that OK for performance?

[Pratik Solanki]

Created attachment 84105 [details]
Patch 1.1 - fixes for review comments

[WebKit Review Bot]

Attachment 84105 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:69:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pratik Solanki]

Comment on attachment 84105 [details]
Patch 1.1 - fixes for review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=84105&action=review

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:762
> +        request = (CFURLRequestRef) 0;

I needed this cast because this code actually constructs a ResourceRequest object and the compiler didn't know which of the two constructors to use - NSURLRequest* or CFURLRequestRef. Would a reinterpret_cast be better here? Or should it be something else.

[Darin Adler]

(In reply to comment #18)
> (From update of attachment 84105 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84105&action=review
> 
> > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:762
> > +        request = (CFURLRequestRef) 0;
> 
> I needed this cast because this code actually constructs a ResourceRequest object and the compiler didn't know which of the two constructors to use - NSURLRequest* or CFURLRequestRef. Would a reinterpret_cast be better here? Or should it be something else.

One way to do it is with a local variable:

    CFURLRequestRef nullRequest = 0;
    request = nullRequest;

I like that.

[Pratik Solanki]

(In reply to comment #14)
> > I could fix this with pthread_cond_wait()/pthread_cond_signal() but I'm not sure if that's the recommended practice (or if it would work on Windows).
> 
> You could use pthreads or ThreadCondition, but generally we just try to avoid having objects shared between threads.

https://bugs.webkit.org/show_bug.cgi?id=55402

(In reply to comment #15)
> (In reply to comment #13)

> > All the constructor definitions are in the class. Would you want me to move all of them to the end of the file?
> 
> I don’t think you should move existing definitions as part of this same patch, but yes, I think end of the file is better even for the existing definitions. The only exception might be one that’s nearly empty and fits on one line, but even in that case, yes.

https://bugs.webkit.org/show_bug.cgi?id=55403

> > I restructured the code so that NSURLResponse is created and set when a CFURLResponseRef is created. That way, we can just use the Mac version of ResourceRequest::nsURLRequest().
> 
> Is that OK for performance?

I think so. We're just front-loading the cost. WebKit needs the NSURLRequest anyway so creating it a bit early should be fine.

Also Jessie points out that I can't use the CFHTTPCookie* functions in CookieJarCFNet.cpp directly on the Mac since CFHTTPCookiePriv.h is not available for user installs. I would need to create WKSI functions for them.

I also tested custom NSURLProtocol usage and it doesn't look like my patch breaks anything in Mail. Custom NSURLProtocol methods continue to be invoked and run with my locally built WebKit.

[Build Bot]

Attachment 84105 [details] did not build on win:
Build output: http://queues.webkit.org/results/8072408

[WebKit Review Bot]

Attachment 84105 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8070572

[Pratik Solanki]

Created attachment 84238 [details]
Patch 1.2 - fixes for review comments and build fixes

[Pratik Solanki]

Created attachment 85776 [details]
Patch 1.3 - Update patch to ToT

[Jessie Berlin]

Comment on attachment 85776 [details]
Patch 1.3 - Update patch to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=85776&action=review

I am excited that you are making these changes, but this patch is too big for me to make sure all the changes work together  in my head. It would be better if you could split it up into smaller patches.
(I am also not officially a reviewer, so I cannot r+ or r- this patch).

> Source/WebCore/loader/ResourceLoader.h:122
>          virtual bool shouldCacheResponse(ResourceHandle*, CFCachedURLResponseRef);

We should have a bug about unifying these calls (the Windows and Mac CFNetwork versions).

> Source/WebCore/loader/mac/ResourceLoaderMac.mm:63
> +#endif

This would be clearer as a #if USE(CFNETWORK) ... #else ... #endif.

> Source/WebCore/platform/mac/SoftLinking.h:72
> +/* callingConvention is unused on Mac but is here to keep the macro prototype the same between Mac and Windows. */

I am not sure why having the macro prototype be the same between Mac and Windows is useful. We can't actually share the code, and the calls to SOFT_LINK_OPTIONAL will have to be in separate #ifdefs anyways.

> Source/WebCore/platform/network/ResourceHandleInternal.h:131
> +#endif

When two #endifs are right next to each other, I find it easier to read if each #endif has a comment as to which #if it belongs to.

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:41
> +#endif

This would be clearer as #if PLATFORM(MAC) ... #else ... #endif

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:52
> +RetainPtr<CFHTTPCookieStorageRef>& privateBrowsingCookieStorage()

Why did you remove the static modifier here?

> Source/WebCore/platform/network/cf/CookieStorageCFNet.h:-39
> -

Again, this seems unrelated to your other changes, makes your patch unnecessarily larger and distracts from the real changes you are making.

> Source/WebCore/platform/network/cf/CredentialStorageCFNet.cpp:42
> +#endif

This would be clearer as #if PLATFORM(MAC) ... #else ... #endif.

> Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:52
> +#endif

Ditto.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:64
> +#include <process.h> // for _beginthread()

We don't usually put comments to the side of the line of code. They should be put above. I am not sure that this comment is necessary at all (I know it was there before, but can probably remove it).

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:66
> +

I think this would be clearer as #if PLATFORM(MAC) ... #else ... #endif

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:121
>  

Might as well get rid of this extra line while you are making changes here.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:235
> +static void didSendBodyData(CFURLConnectionRef, CFIndex /* bytesWritten */, CFIndex totalBytesWritten, CFIndex totalBytesExpectedToWrite, const void *clientInfo)

You should just omit the bytesWritten parameter.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:723
> +    UNUSED_PARAM(frame);

Why is this UNUSED_PARAM unguarded by #if LOG_DISABLED when the earlier ones were guarded?

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:33
> +#include <CFNetwork/CFURLRequestPriv.h>

This #include will probably fail on non-internal machines (that header is not public).

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:43
>  #endif

This would be clearer as #if PLATFORM(MAC) ... #else ... #endif.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:54
> +    // Work around a mistake in the NSURLResponse class.

Is there a bug / radar for this mistake and something tracking changing our implementation once it is fixed?

[Darin Adler]

Alexey, Brady, are one of you going to be able to review this?

[Alexey Proskuryakov]

I keep meaning to, but it's difficult to review a patch of this size. I'm also not quite sure about plans and reasons for the change - which I should ask about elsewhere.

[Pratik Solanki]

I talked with Alexey and Jessie about the patch. I am going to try to split it up into smaller parts and measure its perf impact.

[Pratik Solanki]

Comment on attachment 85776 [details]
Patch 1.3 - Update patch to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=85776&action=review

>> Source/WebCore/loader/ResourceLoader.h:122
>>          virtual bool shouldCacheResponse(ResourceHandle*, CFCachedURLResponseRef);
> 
> We should have a bug about unifying these calls (the Windows and Mac CFNetwork versions).

Filed https://bugs.webkit.org/show_bug.cgi?id=57257

>> Source/WebCore/platform/mac/SoftLinking.h:72
>> +/* callingConvention is unused on Mac but is here to keep the macro prototype the same between Mac and Windows. */
> 
> I am not sure why having the macro prototype be the same between Mac and Windows is useful. We can't actually share the code, and the calls to SOFT_LINK_OPTIONAL will have to be in separate #ifdefs anyways.

This helps keep the SOFT_LINK_OPTIONAL from being in separate ifdefs. This was based on feedback from Darin on previous patch.

>> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:52
>> +RetainPtr<CFHTTPCookieStorageRef>& privateBrowsingCookieStorage()
> 
> Why did you remove the static modifier here?

Because (I think) it was being called from elsewhere in the code. This part of the patch has changed.

>> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:33
>> +#include <CFNetwork/CFURLRequestPriv.h>
> 
> This #include will probably fail on non-internal machines (that header is not public).

Right. I'll have to fix all uses of the headers. Will do that in a separate patch later.

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:54
>> +    // Work around a mistake in the NSURLResponse class.
> 
> Is there a bug / radar for this mistake and something tracking changing our implementation once it is fixed?

Not that I know of. This was code from ResourceResponse::nsURLResponse that I refactored.

[Pratik Solanki]

Created attachment 87196 [details]
SmallPatch 1 - Add CFNetwork framework location and soft link macros

[Pratik Solanki]

Created attachment 87198 [details]
SmallPatch 2 - Add willCacheResponse for CFNetwork, cleanup ifdefs

[Pratik Solanki]

Created attachment 87201 [details]
SmallPatch 3 - Merge privateBrowsingCookieStorage implementations

[Pratik Solanki]

Created attachment 87203 [details]
SmallPatch 4 - Add CFNetwork based authentication functions for mac

[Pratik Solanki]

Created attachment 87211 [details]
SmallPatch 5 - Implement CF based schedule/unschedule for ResourceHandle

[Jessie Berlin]

Comment on attachment 87196 [details]
SmallPatch 1 - Add CFNetwork framework location and soft link macros

View in context: https://bugs.webkit.org/attachment.cgi?id=87196&action=review

Unofficially (since I am not a reviewer), r=me

> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:38
>  #include <CFNetwork/CFHTTPCookiesPriv.h>

I assume you are going to deal with this being included (since it is an internal header on Mac) in another patch.

[Pratik Solanki]

(In reply to comment #35)
> (From update of attachment 87196 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87196&action=review
> 
> Unofficially (since I am not a reviewer), r=me
Thanks.

> > Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:38
> >  #include <CFNetwork/CFHTTPCookiesPriv.h>
> 
> I assume you are going to deal with this being included (since it is an internal header on Mac) in another patch.
Yes. This will need to be cleaned up before we can enable the define on Mac.

[Jessie Berlin]

Comment on attachment 87198 [details]
SmallPatch 2 - Add willCacheResponse for CFNetwork, cleanup ifdefs

View in context: https://bugs.webkit.org/attachment.cgi?id=87198&action=review

Unofficially (since I am not a reviewer), r=me.

> Source/WebCore/loader/EmptyClients.h:395
>      virtual bool shouldCacheResponse(DocumentLoader*, unsigned long, const ResourceResponse&, const unsigned char*, unsigned long long) { return true; }

You should a FIXME here with a reference to https://bugs.webkit.org/show_bug.cgi?id=57257

> Source/WebCore/loader/FrameLoaderClient.h:287
>          virtual bool shouldCacheResponse(DocumentLoader*, unsigned long identifier, const ResourceResponse&, const unsigned char* data, unsigned long long length) = 0;

Ditto.

> Source/WebCore/loader/ResourceLoader.h:127
> +#if PLATFORM(WIN) && USE(CFNETWORK)

This series of #if ... #endif #if ... #endif #if ... endif  would be clearer as a #if ... #elif ... #elif ... #endif.

> Source/WebCore/platform/network/ResourceHandleClient.h:35
>  #include <CFNetwork/CFURLResponsePriv.h>

I assume you will deal with these includes (since these headers are internal on Mac) in a separate patch.

> Source/WebCore/platform/network/ResourceHandleClient.h:109
>  #endif

I am not sure if we have a style about nesting vs. separate # ifs with more conditions, but you should be consistent with the style you use in ResourceLoader.h

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h:213
>      virtual bool shouldCacheResponse(WebCore::DocumentLoader*, unsigned long identifier, const WebCore::ResourceResponse&, const unsigned char* data, unsigned long long length);

You should a FIXME here with a reference to https://bugs.webkit.org/show_bug.cgi?id=57257

[Pratik Solanki]

Created attachment 87227 [details]
SmallPatch 6 - Fixes for warnings and other minor cleanup

[Build Bot]

Attachment 87201 [details] did not build on win:
Build output: http://queues.webkit.org/results/8277290

[Jessie Berlin]

Comment on attachment 87201 [details]
SmallPatch 3 - Merge privateBrowsingCookieStorage implementations

I suspect the reason this is failing to build on Windows is that you did not take into account the recent WebCookeManager (WebKit2) Windows changes which uses the Private Browsing Storage Session. Otherwise, unofficially as usual, this looks good to me.

[WebKit Review Bot]

Attachment 87201 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8276450

[WebKit Review Bot]

http://trac.webkit.org/changeset/85109 might have broken WinCE Release (Build)

[Eric Seidel (no email)]

So does this mean we'll be getting rid of the Mac loader and movign to CFNetwork only?  That would be nice, as it removes one more variable in the search for our random HTTP test flakiness (like bug 51613).

[Pratik Solanki]

(In reply to comment #43)
> So does this mean we'll be getting rid of the Mac loader and movign to CFNetwork only?  That would be nice, as it removes one more variable in the search for our random HTTP test flakiness (like bug 51613).

Eventually. If we can use CFNetwork on all supported Mac OS versions. I don't know if this flag can be turned on on Leopard for example. I've been testing with SnowLeopard only. So it may be a while before we can remove the Foundation based code.

[Eric Seidel (no email)]

This is still exciting.  Thanks! :)

[Pratik Solanki]

Patch 1 - Committed r85070: <http://trac.webkit.org/changeset/85070>
Patch 2 - Committed r85109: <http://trac.webkit.org/changeset/85109>
Followup to Patch 2 - r85113: <http://trac.webkit.org/changeset/85113>
Patch 3 - Committed r85721: <http://trac.webkit.org/changeset/85721>
Patch 4 - Committed r86414: <http://trac.webkit.org/changeset/86414>
Patch 5 - Committed r86415: <http://trac.webkit.org/changeset/86415>
Patch 6 - Committed r86416: <http://trac.webkit.org/changeset/86416>

Since this bug report has become quite huge, I'll use separate bugs to check in the remaining work.

[David Kilzer (:ddkilzer)]

I believe we're going to look at moving iOS and OS X platforms to NSURLSession instead.