Bug 137371 – [iOS] Make WebKit2 build with public iOS SDK and more build fixes for DRT

• Component: WebKit2
• Status: RESOLVED FIXED

[Daniel Bates]

We should make WebKit2 build with the public iOS SDK.

[Daniel Bates]

Created attachment 239156 [details]
Work-in-progress patch

[Daniel Bates]

Created attachment 243652 [details]
Patch

[WebKit Commit Bot]

Attachment 243652 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/cocoa/ServersSPI.h:37:  name_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 243679 [details]
Patch

[WebKit Commit Bot]

Attachment 243679 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/cocoa/ServersSPI.h:37:  name_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/spi/cocoa/ServersSPI.h:43:  bootstrap_look_up is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/spi/cocoa/ServersSPI.h:44:  bootstrap_register2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 243679 [details]
Patch

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

> Source/WebCore/platform/spi/cocoa/ServersSPI.h:33
> +#include <servers/bootstrap.h>

Is this really SPI everywhere? I think at least on OS X, this file is API.

> Source/WebCore/platform/spi/cocoa/ServersSPI.h:44
> +kern_return_t bootstrap_register2(mach_port_t, name_t, mach_port_t, uint64_t);

I don't think you are including a header for this. It is defined in bootstrap_priv.h,

> Source/WebCore/platform/spi/ios/UIKitSPI.h:28
> +// FIXME: Move this header to the WebKit2 framework once we remove DumpRenderTree's
> +// dependence on it. It is a layering violation to include this header or any UIKit
> +// headers in a WebCore or WebKitLegacy file as UIKit depends on WebKitLegacy.

Why not just make DumpRenderTree get it from WebKit2?

[Daniel Bates]

(In reply to comment #6)
> Comment on attachment 243679 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243679&action=review
> 
> > Source/WebCore/platform/spi/cocoa/ServersSPI.h:33
> > +#include <servers/bootstrap.h>
> 
> Is this really SPI everywhere? I think at least on OS X, this file is API.
> 

You're right! Will fix as this is API on OS X.

> > Source/WebCore/platform/spi/cocoa/ServersSPI.h:44
> > +kern_return_t bootstrap_register2(mach_port_t, name_t, mach_port_t, uint64_t);
> 
> I don't think you are including a header for this. It is defined in
> bootstrap_priv.h,
> 

Will include header bootstrap_priv.h when building with the Apple Internal SDK.

> > Source/WebCore/platform/spi/ios/UIKitSPI.h:28
> > +// FIXME: Move this header to the WebKit2 framework once we remove DumpRenderTree's
> > +// dependence on it. It is a layering violation to include this header or any UIKit
> > +// headers in a WebCore or WebKitLegacy file as UIKit depends on WebKitLegacy.
> 
> Why not just make DumpRenderTree get it from WebKit2?

I can do this. When I wrote the patch I thought it would be considered bad form to have a WebKit1-based application, DumpRenderTree, depend on a WebKit2 header and I did not want to add to the bad practices used by DumpRenderTree (including referencing headers from WebCore).

[Daniel Bates]

Created attachment 244190 [details]
Patch 1 of 4: Add new files

Adds SPI wrapper headers and consolidates the content of header _UIHighlightViewSPI.h into the newly added header UIKitSPI.h.

[Daniel Bates]

Created attachment 244192 [details]
Patch 2 of 4: WebCore and WebKit2 Xcode changes

[WebKit Commit Bot]

Attachment 244190 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/cocoa/ServersSPI.h:37:  name_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/spi/cocoa/ServersSPI.h:49:  bootstrap_look_up is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/spi/cocoa/ServersSPI.h:50:  bootstrap_register2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 244193 [details]
Patch 3 of 4: DumpRenderTree changes

[Daniel Bates]

Created attachment 244194 [details]
Patch 4 of 4: Substitute SPI wrapper headers for actual headers; add WTF EXTERN_C_{BEGIN, END}

[WebKit Commit Bot]

Attachment 244194 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/SmartMagnificationController.mm:43:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Comment on attachment 244190 [details]
Patch 1 of 4: Add new files

rs=me

[David Kilzer (:ddkilzer)]

Comment on attachment 244192 [details]
Patch 2 of 4: WebCore and WebKit2 Xcode changes

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 244193 [details]
Patch 3 of 4: DumpRenderTree changes

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

r=me

> Tools/ChangeLog:3
> +        [iOS] Make WebKit2 build with public iOS SDK

I know this is the title of the bug, but the patch actually makes DumpRenderTree build.  Maybe better to change "WebKit2" to "DumpRenderTree"?

[David Kilzer (:ddkilzer)]

Comment on attachment 244194 [details]
Patch 4 of 4: Substitute SPI wrapper headers for actual headers; add WTF EXTERN_C_{BEGIN, END}

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

r=me

> Source/WTF/wtf/Compiler.h:179
>  #ifdef __cplusplus
> +#define EXTERN_C_BEGIN extern "C" {
> +#define EXTERN_C_END }
> +#else
> +#define EXTERN_C_BEGIN
> +#define EXTERN_C_END
> +#endif

Not to be pedantic, but do these need to be named WTF_EXTERN_C_BEGIN and WTF_EXTERN_C_END so we don't conflict with someone else's headers?

> Source/WebKit2/ChangeLog:10
> +        * Configurations/BaseTarget.xcconfig: Append directory $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks to
> +        the list of directories to search for frameworks so that we the linker can find and link against iOS private
> +        frameworks (e.g. AssertionServices).

This was moved to a different patch, I think.

[Daniel Bates]

(In reply to comment #16)
> Comment on attachment 244193 [details]
> Patch 3 of 4: DumpRenderTree changes
> > Tools/ChangeLog:3
> > +        [iOS] Make WebKit2 build with public iOS SDK
> 
> I know this is the title of the bug, but the patch actually makes
> DumpRenderTree build.  Maybe better to change "WebKit2" to "DumpRenderTree"?

No, more build fixes are needed to make DumpRenderTree build with the public SDK.

[Daniel Bates]

(In reply to comment #17)
> > Source/WTF/wtf/Compiler.h:179
> >  #ifdef __cplusplus
> > +#define EXTERN_C_BEGIN extern "C" {
> > +#define EXTERN_C_END }
> > +#else
> > +#define EXTERN_C_BEGIN
> > +#define EXTERN_C_END
> > +#endif
> 
> Not to be pedantic, but do these need to be named WTF_EXTERN_C_BEGIN and
> WTF_EXTERN_C_END so we don't conflict with someone else's headers?
> 

Will rename EXTERN_C_BEGIN and EXTERN_C_END to WTF_EXTERN_C_BEGIN and WTF_EXTERN_C_END, respectively.

> > Source/WebKit2/ChangeLog:10
> > +        * Configurations/BaseTarget.xcconfig: Append directory $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks to
> > +        the list of directories to search for frameworks so that we the linker can find and link against iOS private
> > +        frameworks (e.g. AssertionServices).
> 
> This was moved to a different patch, I think.

This change is in "Patch 2 of 4: WebCore and WebKit2 Xcode changes" (attachment 244192 [details]).

[Daniel Bates]

Committed r178068: <http://trac.webkit.org/changeset/178068>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 140235

[Tim Horton]

Comment on attachment 244194 [details]
Patch 4 of 4: Substitute SPI wrapper headers for actual headers; add WTF EXTERN_C_{BEGIN, END}

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

> Source/WTF/wtf/Compiler.h:181
> +// FIXME: Remove this once we have transitioned to EXTERN_C_BEGIN/EXTERN_C_END.

This file *has* to use /* */ style comments because it gets included in the sandbox profile and the sandbox profile compiler doesn't understand // comments.

[Daniel Bates]

Committed r178080: <http://trac.webkit.org/changeset/178080>

[Daniel Bates]

(In reply to comment #22)
> Comment on attachment 244194 [details]
> Patch 4 of 4: Substitute SPI wrapper headers for actual headers; add WTF
> EXTERN_C_{BEGIN, END}
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244194&action=review
> 
> > Source/WTF/wtf/Compiler.h:181
> > +// FIXME: Remove this once we have transitioned to EXTERN_C_BEGIN/EXTERN_C_END.
> 
> This file *has* to use /* */ style comments because it gets included in the
> sandbox profile and the sandbox profile compiler doesn't understand //
> comments.

Fixed in <http://trac.webkit.org/changeset/178104>.