Bug 136863 - [iOS] Make WebKit build with public iOS SDK
Summary: [iOS] Make WebKit build with public iOS SDK
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on: 136487
Blocks:
  Show dependency treegraph
 
Reported: 2014-09-16 13:11 PDT by Daniel Bates
Modified: 2014-11-19 14:31 PST (History)
5 users (show)

See Also:


Attachments
Patch (38.05 KB, patch)
2014-09-16 13:46 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (37.82 KB, patch)
2014-09-16 16:18 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (38.06 KB, patch)
2014-09-17 12:05 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (38.06 KB, patch)
2014-09-17 13:16 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (44.18 KB, patch)
2014-11-18 18:57 PST, Daniel Bates
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-09-16 13:11:10 PDT
We should make WebKit build with the public iOS SDK.
Comment 1 Daniel Bates 2014-09-16 13:46:10 PDT
Created attachment 238206 [details]
Patch
Comment 2 Daniel Bates 2014-09-16 16:18:25 PDT
Created attachment 238218 [details]
Patch

Rebased patch
Comment 3 Daniel Bates 2014-09-17 12:05:13 PDT
Created attachment 238261 [details]
Patch

Rebased patch
Comment 4 WebKit Commit Bot 2014-09-17 12:07:32 PDT
Attachment 238261 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameNetworkingContext.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Daniel Bates 2014-09-17 13:16:01 PDT
Created attachment 238262 [details]
Patch

Update NSCalenderDateSPI.h to conditionally include <Foundation/NSCalendarDate.h> when building for Mac as it is a public API header.
Comment 6 WebKit Commit Bot 2014-09-17 13:17:18 PDT
Attachment 238262 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameNetworkingContext.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Daniel Bates 2014-11-18 18:57:29 PST
Created attachment 241844 [details]
Patch
Comment 8 WebKit Commit Bot 2014-11-18 18:59:34 PST
Attachment 241844 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameNetworkingContext.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 David Kilzer (:ddkilzer) 2014-11-19 11:17:27 PST
Comment on attachment 241844 [details]
Patch

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

r=me

> Source/WebCore/platform/spi/cf/CFNetworkSPI.h:50
> +#ifdef __OBJC__
> +// FIXME: As a workaround for <rdar://problem/18337182>, we conditionally enclose the header
> +// in an extern "C" linkage block to make it suitable for C++ use.
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#import <CFNetwork/CFNSURLConnection.h>
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +#endif // __OBJC__

Is the #ifdef __OBJC__ check really needed here?  Aren't all the CF* APIs work in plain C?

Or does this header include Objective-C headers without their own guards?

> Source/WebKit/mac/Misc/WebDownload.h:34
> +#if __has_include(<Foundation/NSURLDownload.h>)
>  #import <Foundation/NSURLDownload.h>
> +#else
> +@interface NSURLDownload : NSObject
> +@end
> +#endif

At minimum, it seems like we should add a comment here about why this is defined inline instead of an *SPI.h header so someone doesn't try to "clean it up" later.

Is it worth filing a radar either (1) to get internal clients off this WebKit SPI header (#irony), or (2) to make this API for all clients?
Comment 10 Daniel Bates 2014-11-19 13:03:36 PST
(In reply to comment #9)
> [...]
> > Source/WebCore/platform/spi/cf/CFNetworkSPI.h:50
> > +#ifdef __OBJC__
> > +// FIXME: As a workaround for <rdar://problem/18337182>, we conditionally enclose the header
> > +// in an extern "C" linkage block to make it suitable for C++ use.
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#import <CFNetwork/CFNSURLConnection.h>
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +#endif // __OBJC__
> 
> Is the #ifdef __OBJC__ check really needed here?  Aren't all the CF* APIs
> work in plain C?
> 

Yes, the #ifdef __OBJC__ is needed as a workaround for <rdar://problem/19033610>. I'll add the following comment above the #ifdef __OBJC__ line before landing:

FIXME: Remove the #ifdef __OBJC__-guard onnce we fix <rdar://problem/19033610>.


> Or does this header include Objective-C headers without their own guards?
> 

Yes, this header includes Objective-C headers without a __OBJC__ guard. See <rdar://problem/19033610> for more details.

> > Source/WebKit/mac/Misc/WebDownload.h:34
> > +#if __has_include(<Foundation/NSURLDownload.h>)
> >  #import <Foundation/NSURLDownload.h>
> > +#else
> > +@interface NSURLDownload : NSObject
> > +@end
> > +#endif
> 
> At minimum, it seems like we should add a comment here about why this is
> defined inline instead of an *SPI.h header so someone doesn't try to "clean
> it up" later.
> 

Will add a comment similar to the comment in WAKAppKitStubs.h(*):

We explicitly use __has_include() instead of the macro define WTF_USE_APPLE_INTERNAL_SDK as the condition for including the header Foundation/NSURLDownload.h to support internal Apple clients that build without header wtf/Platform.h.

(*) <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ios/wak/WAKAppKitStubs.h?rev=176105#L57>

> Is it worth filing a radar either (1) to get internal clients off this
> WebKit SPI header (#irony), or (2) to make this API for all clients?

I felt such a discussion is worth to have and filed <rdar://problem/19034131>.
Comment 11 Daniel Bates 2014-11-19 14:31:57 PST
Committed r176347: <http://trac.webkit.org/changeset/176347>