Bug 137536 - [iOS] NSGeometry data types are not available in the public SDK
Summary: [iOS] NSGeometry data types are not available in the public SDK
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks: 136487
  Show dependency treegraph
 
Reported: 2014-10-08 13:19 PDT by Daniel Bates
Modified: 2014-11-13 16:34 PST (History)
13 users (show)

See Also:


Attachments
Patch (17.36 KB, patch)
2014-10-08 13:23 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2014-11-12 17:23 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-10-08 13:19:42 PDT
As a step towards making WebKit build with the public iOS SDK, add a SPI wrapper header for NSGeometry SPI.

Note, I chose to separate out the adding of an NSGeometry SPI wrapper header into its own bug from the patch for bug #136487 so as to make it straightforward to review its correctness.
Comment 1 Daniel Bates 2014-10-08 13:23:37 PDT
Created attachment 239490 [details]
Patch
Comment 2 Daniel Bates 2014-10-08 13:24:36 PDT
I am open to suggestions on this patch, including how best to support building WebKitLegacy apps with the Apple internal SDK and without the header wtf/Platform.h.
Comment 3 Andy Estes 2014-10-09 15:05:27 PDT
Comment on attachment 239490 [details]
Patch

r- since I don't like how this patch makes NSGeometrySPI.h a Private header in WebKitLegacy. These headers should only be for our own use. I talked with Dan in person and we think we devised a way to avoid making this header SPI.
Comment 4 Daniel Bates 2014-11-12 17:23:05 PST
Created attachment 241455 [details]
Patch
Comment 5 David Kilzer (:ddkilzer) 2014-11-13 15:16:00 PST
Comment on attachment 241455 [details]
Patch

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

r- to move the NSGeometry.h items into their own NSGeometrySPI.h header.

> Source/WebCore/platform/ios/wak/WAKAppKitStubs.h:64
> +// FIXME: <rdar://problem/6669434> Switch from using NSGeometry methods to CGGeometry methods
> +//
> +// We explicitly use __has_include() instead of the macro define WTF_USE_APPLE_INTERNAL_SDK as
> +// the condition for including the header Foundation/NSGeometry.h to support internal Apple
> +// clients that build without header wtf/Platform.h.
> +#if __has_include(<Foundation/NSGeometry.h>)
> +
> +#import <Foundation/NSGeometry.h>
> +
> +#else

I do not want to add more includes of WAKAppKitStubs.h.  I want to get rid of WAKAppKitStubs.h.

Please put this code into its own NSGeometrySPI.h header and include that instead of WAKAppKitStubs.h in files were WAKAppKitStubs.h was newly included.  (It's fine to include NSGeometrySPI.h here, though, so you don't have to change existing clients.)
Comment 6 David Kilzer (:ddkilzer) 2014-11-13 15:27:58 PST
Comment on attachment 241455 [details]
Patch

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

>> Source/WebCore/platform/ios/wak/WAKAppKitStubs.h:64
>> +#else
> 
> I do not want to add more includes of WAKAppKitStubs.h.  I want to get rid of WAKAppKitStubs.h.
> 
> Please put this code into its own NSGeometrySPI.h header and include that instead of WAKAppKitStubs.h in files were WAKAppKitStubs.h was newly included.  (It's fine to include NSGeometrySPI.h here, though, so you don't have to change existing clients.)

After talking to Dan, I remember why we did it this way--some internal clients still use WAKAppKitStubs.h and if we'd have to make NSGeometrySPI.h a private header (along with WAKAppKitStubs.h) so that we wouldn't break them.

It's fine to do this for now since we can easily clean up the new WAKAppKitStubs.h includes because they are all in WebKit itself.
Comment 7 Daniel Bates 2014-11-13 16:34:30 PST
Committed r176105: <http://trac.webkit.org/changeset/176105>