Bug 139831 - [iOS] Add WebKitSystemInterface for iOS 8.1
Summary: [iOS] Add WebKitSystemInterface for iOS 8.1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad iOS 8.1
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on: 139829
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-19 12:59 PST by Daniel Bates
Modified: 2014-12-19 16:17 PST (History)
8 users (show)

See Also:


Attachments
Patch (164.44 KB, patch)
2014-12-19 13:00 PST, Daniel Bates
ap: 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-12-19 12:59:15 PST
Towards building iOS WebKit with the public iOS SDK we need to add WebKitSystemInterface for iOS.
Comment 1 Daniel Bates 2014-12-19 13:00:16 PST
Created attachment 243570 [details]
Patch
Comment 2 Andy Estes 2014-12-19 13:04:15 PST
Comment on attachment 243570 [details]
Patch

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

> WebKitLibraries/WebKitSystemInterfaceIOS.h:2
> + *  WebKitSystemInterfaceIOS.mm

Oops.
Comment 3 Daniel Bates 2014-12-19 15:10:50 PST
(In reply to comment #2)
> Comment on attachment 243570 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243570&action=review
> 
> > WebKitLibraries/WebKitSystemInterfaceIOS.h:2
> > + *  WebKitSystemInterfaceIOS.mm
> 
> Oops.

Will fix before landing. Will also remove extraneous whitespace from the file.
Comment 4 Alexey Proskuryakov 2014-12-19 15:15:17 PST
Comment on attachment 243570 [details]
Patch

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

I'm going to say r=me, but I don't understand why we need more WKSI. We wanted to get rid of it completely, not to add more flavors.

> Tools/Scripts/copy-webkitlibraries-to-product-directory:147
> +    if (willUseIOSDeviceSDKWhenBuilding()) {

Is there a downside to copying them all ,like we do for OS X? At least the code would be simpler.

> Tools/Scripts/copy-webkitlibraries-to-product-directory:174
>      dittoHeaders("WebKitLibraries/WebKitSystemInterface.h", "$productDir/usr/local/include/WebKitSystemInterface.h");
> +    dittoHeaders("WebKitLibraries/WebKitSystemInterfaceIOS.h", "$productDir/usr/local/include/WebKitSystemInterfaceIOS.h") if isIOSWebKit();

This setup is sort of weird. We have iOS ifdefs in WebKitSystemInterface.h, plus we also have a separate header. I don't know if it would be better to make a separate Mac file, or to put everything into WebKitSystemInterface.h, but what we have now is asymmetric.
Comment 5 Daniel Bates 2014-12-19 15:25:20 PST
(In reply to comment #4)
> I'm going to say r=me, but I don't understand why we need more WKSI. We
> wanted to get rid of it completely, not to add more flavors.
> 

In the long term we want to remove WKSI. For now, I'll upstream WKSI for iOS so as to expedite the bring up of iOS WebKit using the public iOS SDK.

> > Tools/Scripts/copy-webkitlibraries-to-product-directory:147
> > +    if (willUseIOSDeviceSDKWhenBuilding()) {
> 
> Is there a downside to copying them all ,like we do for OS X? At least the
> code would be simpler.
> 

Disregarding disk space (the files are small), there isn't a downside to copying both the device and simulator WKSI. Will simplify code to copy both the device and simulator WKSI.

> > Tools/Scripts/copy-webkitlibraries-to-product-directory:174
> >      dittoHeaders("WebKitLibraries/WebKitSystemInterface.h", "$productDir/usr/local/include/WebKitSystemInterface.h");
> > +    dittoHeaders("WebKitLibraries/WebKitSystemInterfaceIOS.h", "$productDir/usr/local/include/WebKitSystemInterfaceIOS.h") if isIOSWebKit();
> 
> This setup is sort of weird. We have iOS ifdefs in WebKitSystemInterface.h,
> plus we also have a separate header. I don't know if it would be better to
> make a separate Mac file, or to put everything into WebKitSystemInterface.h,
> but what we have now is asymmetric.

I agree. We can clean this up in another iteration or, even better, work to remove WKSI.
Comment 6 Daniel Bates 2014-12-19 16:17:44 PST
Committed r177603: <http://trac.webkit.org/changeset/177603>