Bug 63674

Summary: Get webkit to compile with USE(CFNETWORK) enabled on Mac
Product: WebKit Reporter: Pratik Solanki <psolanki>
Component: PlatformAssignee: Pratik Solanki <psolanki>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ap, beidson, darin, ddkilzer, jberlin, koivisto, psolanki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 51836    
Attachments:
Description Flags
Patch 1 - Changes to ResourceHandle class
ddkilzer: review+
Patch 2 - the rest
ddkilzer: review-
LoaderRunLoop changes
ddkilzer: review+
Authentication/Credential changes
ddkilzer: review+
Compile fixes
ddkilzer: review+
WebKitSystemInterface changes
ddkilzer: review+
Downloads in webkit1
ddkilzer: review+
Cookie changes ddkilzer: review+, webkit.review.bot: commit-queue-

Description Pratik Solanki 2011-06-29 17:37:57 PDT
Tracks the patches remaining to get WebKit to compile with USE(CFNETWORK) enabled
Comment 1 Pratik Solanki 2011-06-29 17:43:40 PDT
Created attachment 99195 [details]
Patch 1 - Changes to ResourceHandle class
Comment 2 Pratik Solanki 2011-06-29 18:25:15 PDT
Created attachment 99199 [details]
Patch 2 - the rest
Comment 3 Andy Estes 2011-06-29 21:13:42 PDT
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?
Comment 4 Andy Estes 2011-06-29 22:07:02 PDT
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.
Comment 5 Pratik Solanki 2011-06-30 16:04:50 PDT
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.
Comment 6 Jessie Berlin 2011-07-11 11:39:49 PDT
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.
Comment 7 David Kilzer (:ddkilzer) 2011-07-11 11:51:20 PDT
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.
Comment 8 David Kilzer (:ddkilzer) 2011-07-11 12:49:44 PDT
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.
Comment 9 Jessie Berlin 2011-07-11 13:08:33 PDT
> > 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.
Comment 10 Pratik Solanki 2011-07-12 13:48:09 PDT
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!
Comment 11 Pratik Solanki 2011-07-12 17:24:15 PDT
Committed r90873: <http://trac.webkit.org/changeset/90873>
Comment 12 Pratik Solanki 2011-07-13 13:29:02 PDT
Created attachment 100704 [details]
LoaderRunLoop changes
Comment 13 Pratik Solanki 2011-07-14 17:02:37 PDT
Created attachment 100897 [details]
Authentication/Credential changes
Comment 14 Pratik Solanki 2011-07-14 17:03:34 PDT
Created attachment 100898 [details]
Compile fixes
Comment 15 David Kilzer (:ddkilzer) 2011-07-15 08:35:01 PDT
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.
Comment 16 David Kilzer (:ddkilzer) 2011-07-15 08:36:04 PDT
Comment on attachment 100897 [details]
Authentication/Credential changes

r=me
Comment 17 David Kilzer (:ddkilzer) 2011-07-15 08:42:40 PDT
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.
Comment 18 Darin Adler 2011-07-15 09:24:13 PDT
(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.
Comment 19 Pratik Solanki 2011-07-15 10:06:21 PDT
(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.
Comment 20 Pratik Solanki 2011-07-15 11:39:07 PDT
Committed r91081: <http://trac.webkit.org/changeset/91081>
Comment 21 Pratik Solanki 2011-07-15 11:54:11 PDT
Committed r91084: <http://trac.webkit.org/changeset/91084>
Comment 22 Pratik Solanki 2011-07-15 14:03:09 PDT
Committed r91102: <http://trac.webkit.org/changeset/91102>
Comment 23 Pratik Solanki 2011-07-15 16:33:31 PDT
Created attachment 101068 [details]
WebKitSystemInterface changes
Comment 24 Pratik Solanki 2011-07-15 17:03:37 PDT
Created attachment 101072 [details]
Downloads in webkit1
Comment 25 Pratik Solanki 2011-07-15 17:58:21 PDT
Created attachment 101079 [details]
Cookie changes
Comment 26 WebKit Review Bot 2011-07-15 18:16:04 PDT
Comment on attachment 101079 [details]
Cookie changes

Attachment 101079 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9097531
Comment 27 David Kilzer (:ddkilzer) 2011-07-15 20:51:37 PDT
Comment on attachment 101068 [details]
WebKitSystemInterface changes

r=me
Comment 28 David Kilzer (:ddkilzer) 2011-07-15 21:06:30 PDT
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;
Comment 29 David Kilzer (:ddkilzer) 2011-07-15 21:10:20 PDT
Comment on attachment 101079 [details]
Cookie changes

r=me, but it might be good for jessieberlin to look at this as well.
Comment 30 Pratik Solanki 2011-07-16 16:00:54 PDT
Committed r91154: <http://trac.webkit.org/changeset/91154>
Comment 31 Pratik Solanki 2011-07-16 16:51:16 PDT
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.
Comment 32 Jessie Berlin 2011-07-17 13:31:34 PDT
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.
Comment 33 Pratik Solanki 2011-07-18 11:12:28 PDT
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.
Comment 34 Pratik Solanki 2011-07-18 11:43:28 PDT
Committed r91198: <http://trac.webkit.org/changeset/91198>
Comment 35 Pratik Solanki 2011-07-18 12:07:06 PDT
Committed r91200: <http://trac.webkit.org/changeset/91200>
Comment 36 Pratik Solanki 2011-07-18 12:23:12 PDT
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.