Bug 63674 – Get webkit to compile with USE(CFNETWORK) enabled on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Pratik Solanki]

Tracks the patches remaining to get WebKit to compile with USE(CFNETWORK) enabled

[Pratik Solanki]

Created attachment 99195 [details]
Patch 1 - Changes to ResourceHandle class

[Pratik Solanki]

Created attachment 99199 [details]
Patch 2 - the rest

[Andy Estes]

Comment on attachment 99195 [details]
Patch 1 - Changes to ResourceHandle class

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

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:220
> +#if PLATFORM(MAC)
> +    // Avoid MIME type sniffing if the response comes back as 304 Not Modified.
> +    CFHTTPMessageRef msg = CFURLResponseGetHTTPResponse(cfResponse);
> +    int statusCode =  msg ? CFHTTPMessageGetResponseStatusCode(msg) : 0;
> +
> +    if (statusCode != 304)
> +        adjustMIMETypeIfNecessary(cfResponse);
> +
> +    if (_CFURLRequestCopyProtocolPropertyForKey(handle->firstRequest().cfURLRequest(), CFSTR("ForceHTMLMIMEType")))
> +        CFURLResponseSetMIMEType(cfResponse, CFSTR("text/html"));

This seemed like a change in behavior to until I looked in ResourceHandleMac.mm and saw the NSURLResponse version of this code. A note in the ChangeLog about why this was added would have cleared this up.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:831
> -        request = 0;
> +        CFURLRequestRef nullRequest = 0;
> +        request = nullRequest;

Can this be done with a cast instead of creating a new local?

[Andy Estes]

Comment on attachment 99195 [details]
Patch 1 - Changes to ResourceHandle class

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

>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:220
>> +        CFURLResponseSetMIMEType(cfResponse, CFSTR("text/html"));
> 
> This seemed like a change in behavior to until I looked in ResourceHandleMac.mm and saw the NSURLResponse version of this code. A note in the ChangeLog about why this was added would have cleared this up.

It'd be even nicer to share this with the Mac rather than duplicating it. The Mac port could do this in terms of CF API and toll-free bridging should ensure that the NSURLResponse has the right changes.

[Pratik Solanki]

Comment on attachment 99195 [details]
Patch 1 - Changes to ResourceHandle class

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

Thanks for looking at this!

>>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:220
>>> +        CFURLResponseSetMIMEType(cfResponse, CFSTR("text/html"));
>> 
>> This seemed like a change in behavior to until I looked in ResourceHandleMac.mm and saw the NSURLResponse version of this code. A note in the ChangeLog about why this was added would have cleared this up.
> 
> It'd be even nicer to share this with the Mac rather than duplicating it. The Mac port could do this in terms of CF API and toll-free bridging should ensure that the NSURLResponse has the right changes.

Yes thats a good idea. I can refactor the code so its shared between the two implementations.

>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:831
>> +        request = nullRequest;
> 
> Can this be done with a cast instead of creating a new local?

I had it as a cast in an older patch for bug 51836 but Darin suggested I use a local variable.

[Jessie Berlin]

Comment on attachment 99199 [details]
Patch 2 - the rest

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

This patch seems to cover a couple different things. The CookieStorage, RunLoop, Downloads and Authentications changes all seem pretty separate. Since I am not familiar with Downloads / Auth, I feel a little bit uncomfortable unofficially reviewing that code.

> Source/WebCore/platform/mac/WebCoreSystemInterface.h:342
> +

No need for this extra line.

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:72
> +    return wkGetDefaultHTTPCookieStorage();

This will be wrong for Windows WK2.

On Windows WK2, it needs to return the cookie storage for the storage session that is shared between the UI and Web Processes.

Instead of returning wkGetDefaultHTTPCookieStorage(), you should return defaultCookieStorage() like the original version of the method does, and move defaultCookieStorage out of the Windows-only code section.

> 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?

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

Ditto.

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

Ditto.

[David Kilzer (:ddkilzer)]

Comment on attachment 99195 [details]
Patch 1 - Changes to ResourceHandle class

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

r=me

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:214
> +    int statusCode =  msg ? CFHTTPMessageGetResponseStatusCode(msg) : 0;

Nit: Extra space after "=".

>>>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:220
>>>> +        CFURLResponseSetMIMEType(cfResponse, CFSTR("text/html"));
>>> 
>>> This seemed like a change in behavior to until I looked in ResourceHandleMac.mm and saw the NSURLResponse version of this code. A note in the ChangeLog about why this was added would have cleared this up.
>> 
>> It'd be even nicer to share this with the Mac rather than duplicating it. The Mac port could do this in terms of CF API and toll-free bridging should ensure that the NSURLResponse has the right changes.
> 
> Yes thats a good idea. I can refactor the code so its shared between the two implementations.

Make sure NSURLResponse is toll-free-bridged to CFURLResponse.  Not all of the NSURL* types are toll-free-bridged.

[David Kilzer (:ddkilzer)]

Comment on attachment 99199 [details]
Patch 2 - the rest

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.nsURLAuthenticationChallenge()];
> 
> Ditto.

And here.

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

And here.

[Jessie Berlin]

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

Because Mac doesn't use a special "default" storage session like Windows does, it does not need to specify the default storage session to use when creating the private storage session.

So this code is fine.

[Pratik Solanki]

Comment on attachment 99195 [details]
Patch 1 - Changes to ResourceHandle class

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

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

>>>> 
>>>> This seemed like a change in behavior to until I looked in ResourceHandleMac.mm and saw the NSURLResponse version of this code. A note in the ChangeLog about why this was added would have cleared this up.
>>> 
>>> It'd be even nicer to share this with the Mac rather than duplicating it. The Mac port could do this in terms of CF API and toll-free bridging should ensure that the NSURLResponse has the right changes.
>> 
>> Yes thats a good idea. I can refactor the code so its shared between the two implementations.
> 
> Make sure NSURLResponse is toll-free-bridged to CFURLResponse.  Not all of the NSURL* types are toll-free-bridged.

NSURLResponse is not toll-free bridged to CFURLResponse. But you can extract a CFURLResponse from an NSURLResponse and use that. I don't think I'll be able to refactor this since the CF code uses SPI in private headers thats not exposed on Mac headers (e.g. _CFURLRequestCopyProtocolPropertyForKey). Those calls would need to go through WKSI. That work is tracked by bug 63569. I did realize that I can use wk equivalents for CFURLResponseGetHTTPResponse and CFURLResponseSetMIMEType, so I'll do that. Meanwhile, I'll put in a comment in the Changelog as Andy suggested. Thanks for the review!

[Pratik Solanki]

Committed r90873: <http://trac.webkit.org/changeset/90873>

[Pratik Solanki]

Created attachment 100704 [details]
LoaderRunLoop changes

[Pratik Solanki]

Created attachment 100897 [details]
Authentication/Credential changes

[Pratik Solanki]

Created attachment 100898 [details]
Compile fixes

[David Kilzer (:ddkilzer)]

Comment on attachment 100704 [details]
LoaderRunLoop changes

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

r=me

> Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:67
> +            // FIXME: http://webkit.org/b/55402 - loaderRunLoop() should use synchronization instead of while loop

Nit: I like "<>" around URLs, but it's not necessary.

[David Kilzer (:ddkilzer)]

Comment on attachment 100897 [details]
Authentication/Credential changes

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 100898 [details]
Compile fixes

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

r=me, but please consider the min<>/max<> changes.

> Source/WebCore/platform/network/cf/ResourceErrorCF.cpp:123
>  #if PLATFORM(WIN)
>      errorCopy.m_certificate = m_certificate;
> +#else
> +    UNUSED_PARAM(errorCopy);
>  #endif

Is this not used or available on Mac OS X?

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:75
> +    return static_cast<time_t>(min(max(minTimeAsDouble, time + kCFAbsoluteTimeIntervalSince1970), maxTimeAsDouble));

You should probably switch to min<time_t>() and max<time_t>() here instead of casting the final result.

[Darin Adler]

(In reply to comment #17)
> > Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:75
> > +    return static_cast<time_t>(min(max(minTimeAsDouble, time + kCFAbsoluteTimeIntervalSince1970), maxTimeAsDouble));
> 
> You should probably switch to min<time_t>() and max<time_t>() here instead of casting the final result.

I agree that’s nicer style, but since it also will do something different I am not sure we should change it. It will convert the three values to time_t before performing the clamping operation. I think it’s important to clamp before converting, since values outside the range might not convert correctly.

[Pratik Solanki]

(In reply to comment #15)
> (From update of attachment 100704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100704&action=review
> 
> r=me

Thanks!
 
> > Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:67
> > +            // FIXME: http://webkit.org/b/55402 - loaderRunLoop() should use synchronization instead of while loop
> 
> Nit: I like "<>" around URLs, but it's not necessary.

I'll add that. Also. I noticed that the patch failed on Windows because it couldn't find numeric_limits. I'll add an #include <limits> which should fix that. I guess that file was included implicitly on Mac somehow, but not on Windows.

[Pratik Solanki]

Committed r91081: <http://trac.webkit.org/changeset/91081>

[Pratik Solanki]

Committed r91084: <http://trac.webkit.org/changeset/91084>

[Pratik Solanki]

Committed r91102: <http://trac.webkit.org/changeset/91102>

[Pratik Solanki]

Created attachment 101068 [details]
WebKitSystemInterface changes

[Pratik Solanki]

Created attachment 101072 [details]
Downloads in webkit1

[Pratik Solanki]

Created attachment 101079 [details]
Cookie changes

[WebKit Review Bot]

Comment on attachment 101079 [details]
Cookie changes

Attachment 101079 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9097531

[David Kilzer (:ddkilzer)]

Comment on attachment 101068 [details]
WebKitSystemInterface changes

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 101072 [details]
Downloads in webkit1

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

r=me

> Source/WebKit/mac/Misc/WebDownload.mm:261
> +    [self _setRealDelegate:delegate];
> +    return [super _initWithLoadingCFURLConnection:connection request:request response:response delegate:_webInternal proxy:proxy];

Shouldn't you set self first before calling -_setRealDelegate: here?

    self = [super _initWithLoadingCFURLConnection:connection request:request response:response delegate:_webInternal proxy:proxy];
    if (!self)
        return nil;
    [self _setRealDelegate:delegate];
    return self;

[David Kilzer (:ddkilzer)]

Comment on attachment 101079 [details]
Cookie changes

r=me, but it might be good for jessieberlin to look at this as well.

[Pratik Solanki]

Committed r91154: <http://trac.webkit.org/changeset/91154>

[Pratik Solanki]

Comment on attachment 101072 [details]
Downloads in webkit1

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

>> Source/WebKit/mac/Misc/WebDownload.mm:261
>> +    return [super _initWithLoadingCFURLConnection:connection request:request response:response delegate:_webInternal proxy:proxy];
> 
> Shouldn't you set self first before calling -_setRealDelegate: here?
> 
>     self = [super _initWithLoadingCFURLConnection:connection request:request response:response delegate:_webInternal proxy:proxy];
>     if (!self)
>         return nil;
>     [self _setRealDelegate:delegate];
>     return self;

Good point. I had just copied code from the other init method. I'll change it to what you suggest.

[Jessie Berlin]

Comment on attachment 101079 [details]
Cookie changes

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

Unofficial r=me! (provided that the issues brought up by the comitt queue are addressed)

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:91
> +#if USE(CFURLSTORAGESESSIONS) && PLATFORM(WIN)

Considering that this is already within "#if USE(CFNETWORK) && PLATFORM(WIN)", it doesn't seem necessary to have the "&& PLATFORM(WIN)" part here.

[Pratik Solanki]

Comment on attachment 101072 [details]
Downloads in webkit1

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

>>> Source/WebKit/mac/Misc/WebDownload.mm:261
>>> +    return [super _initWithLoadingCFURLConnection:connection request:request response:response delegate:_webInternal proxy:proxy];
>> 
>> Shouldn't you set self first before calling -_setRealDelegate: here?
>> 
>>     self = [super _initWithLoadingCFURLConnection:connection request:request response:response delegate:_webInternal proxy:proxy];
>>     if (!self)
>>         return nil;
>>     [self _setRealDelegate:delegate];
>>     return self;
> 
> Good point. I had just copied code from the other init method. I'll change it to what you suggest.

I couldn't do that. The _setRealDelegate creates the _webInternal object that we then pass on to _initWithLoadingCFURLConnection. This seems to be the pattern used in other methods in this class as well. I'll check in my current version.

[Pratik Solanki]

Committed r91198: <http://trac.webkit.org/changeset/91198>

[Pratik Solanki]

Committed r91200: <http://trac.webkit.org/changeset/91200>

[Pratik Solanki]

Oops. Looks like I caused Windows build to break with my last checkin. Attempted fix is <http://trac.webkit.org/changeset/91201>. I should have looked more closely at the Windows EWS failure.