Bug 136467

Summary: [iOS] Support using Foundation networking code
Product: WebKit Reporter: Daniel Bates <dbates>
Component: PlatformAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, benjamin, cmarcelo, commit-queue, ddkilzer, japhet, mjs, psolanki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: iPhone / iPad   
OS: Unspecified   
Attachments:
Description Flags
Patch psolanki: review+

Description Daniel Bates 2014-09-02 15:27:05 PDT
We should support building iOS WebKit with the Foundation networking code in addition to the current CFNetwork code. For now, we should also disable USE(CFNETWORK) when building iOS WebKit with the public iOS SDK so as to expedite its bring up.
Comment 1 Daniel Bates 2014-09-02 15:34:52 PDT
Created attachment 237522 [details]
Patch
Comment 2 WebKit Commit Bot 2014-09-02 15:37:49 PDT
Attachment 237522 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/cf/CFNetworkConnectionCacheSPI.h:33:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Pratik Solanki 2014-09-02 15:48:11 PDT
Comment on attachment 237522 [details]
Patch

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

I think I would prefer the platform name come first e.g.

    #if PLATFORM(IOS) && !USE(CFNETWORK) 

instead of

    #if !USE(CFNETWORK) && PLATFORM(IOS)

Seems easier to read for me. But what you have is fine as well if that is what we use in other code.

> Source/WebCore/loader/ResourceLoader.cpp:583
> +#if USE(CF_NETWORK) && PLATFORM(IOS)

This should be USE(CFNETWORK)

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:38
>  #import <Foundation/NSURLError.h>

Not part of your patch, but I don't think we need this include anymore. Foundation.h seems to include NSURLError.h.
Comment 4 Daniel Bates 2014-09-02 16:57:51 PDT
(In reply to comment #3)
> (From update of attachment 237522 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237522&action=review
> 
> I think I would prefer the platform name come first e.g.
> 
>     #if PLATFORM(IOS) && !USE(CFNETWORK) 
> 
> instead of
> 
>     #if !USE(CFNETWORK) && PLATFORM(IOS)
> 
> Seems easier to read for me. But what you have is fine as well if that is what we use in other code.
> 

Will update the patch such that we query PLATFORM(IOS) before USE(CFNETWORK).

> > Source/WebCore/loader/ResourceLoader.cpp:583
> > +#if USE(CF_NETWORK) && PLATFORM(IOS)
> 
> This should be USE(CFNETWORK)
> 

Notice that ResourceLoader.cpp is compiled in a Windows build by <http://trac.webkit.org/export/173191/trunk/Source/WebCore/WebCore.vcxproj/WebCore.vcxproj>. Also ResourceHandleMac.mm and ResourceHandleCFNet.cpp both call ResourceHandleClient::connectionProperties() and ResourceLoader::connectionProperties()  overrides ResourceHandleClient::connectionProperties(). So, this macro guard should read:

#if PLATFORM(IOS)

> > Source/WebCore/platform/network/mac/ResourceErrorMac.mm:38
> >  #import <Foundation/NSURLError.h>
> 
> Not part of your patch, but I don't think we need this include anymore. Foundation.h seems to include NSURLError.h.

Will remove header <Foundation/NSURLError.h> before landing.
Comment 5 Daniel Bates 2014-09-02 17:14:24 PDT
Committed r173192: <http://trac.webkit.org/changeset/173192>
Comment 6 Daniel Bates 2014-09-02 17:59:18 PDT
Committed Windows build fix in <http://trac.webkit.org/changeset/173196>.