Tracks the patches remaining to get WebKit to compile with USE(CFNETWORK) enabled
Created attachment 99195 [details] Patch 1 - Changes to ResourceHandle class
Created attachment 99199 [details] Patch 2 - the rest
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 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 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 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 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 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.
> > 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 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!
Committed r90873: <http://trac.webkit.org/changeset/90873>
Created attachment 100704 [details] LoaderRunLoop changes
Created attachment 100897 [details] Authentication/Credential changes
Created attachment 100898 [details] Compile fixes
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 on attachment 100897 [details] Authentication/Credential changes r=me
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.
(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.
(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.
Committed r91081: <http://trac.webkit.org/changeset/91081>
Committed r91084: <http://trac.webkit.org/changeset/91084>
Committed r91102: <http://trac.webkit.org/changeset/91102>
Created attachment 101068 [details] WebKitSystemInterface changes
Created attachment 101072 [details] Downloads in webkit1
Created attachment 101079 [details] Cookie changes
Comment on attachment 101079 [details] Cookie changes Attachment 101079 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9097531
Comment on attachment 101068 [details] WebKitSystemInterface changes r=me
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 on attachment 101079 [details] Cookie changes r=me, but it might be good for jessieberlin to look at this as well.
Committed r91154: <http://trac.webkit.org/changeset/91154>
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 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 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.
Committed r91198: <http://trac.webkit.org/changeset/91198>
Committed r91200: <http://trac.webkit.org/changeset/91200>
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.