Bug 137277 – Clean up: Move XPC forward declarations in JavaScriptCore to WTF SPI wrapper header

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Daniel Bates]

We should move the conditional inclusion of the XPC headers <xpc/xpc.h> and <xpc/private.h> and forward declarations of XPC functions in various JavaScriptCore and WebKit2 files to a centralized SPI wrapper header in WTF, say WTF/wtf/spi/cocoa/XPCSPI.h. Then we can remove duplicate code. This will also aid in making WebKit2 build for iOS with the public iOS SDK.

[Daniel Bates]

Created attachment 238978 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:38:  xpc_connection_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:50:  xpc_object_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:51:  xpc_connection_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:57:  xpc_type_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 13 files


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

[Daniel Bates]

Created attachment 239035 [details]
Patch

_xpc_error_connection_invalid and _xpc_error_termination_imminent are only const in OS X >= 10.9

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:38:  xpc_connection_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:50:  xpc_object_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:51:  xpc_connection_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:57:  xpc_type_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 13 files


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

[Alexey Proskuryakov]

Comment on attachment 239035 [details]
Patch

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

> Source/WTF/ChangeLog:14
> +        * wtf/spi/cocoa/XPCSPI.h: Added.

It is annoying to have this in a cocoa directory, because XPC has nothing to do with Cocoa. Maybe darwin?

> Source/WTF/wtf/spi/cocoa/XPCSPI.h:87
> +EXTERN_C CONST_ON_OR_AFTER_MAC_OS_X_VERSION_1090 struct _xpc_dictionary_s _xpc_error_connection_invalid;

Where does EXTERN_C come from? I'd prefer a plain extern "C" - it's no more verbose, but clearly less fragile.

> Source/WTF/wtf/spi/cocoa/XPCSPI.h:99
> +#if __BLOCKS__

We use "#ifdef __BLOCKS__" elsewhere in WebKit. Which is right?

> Source/WTF/wtf/spi/cocoa/XPCSPI.h:152
> +#if !defined(xpc_retain) && OS_OBJECT_USE_OBJC_RETAIN_RELEASE
> +#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; })
> +#endif
> +
> +#if !defined(xpc_release) && OS_OBJECT_USE_OBJC_RETAIN_RELEASE
> +#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; })
> +#endif

Does this compile in .cpp files?

[Daniel Bates]

(In reply to comment #5)
> (From update of attachment 239035 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239035&action=review
> 
> > Source/WTF/ChangeLog:14
> > +        * wtf/spi/cocoa/XPCSPI.h: Added.
> 
> It is annoying to have this in a cocoa directory, because XPC has nothing to do with Cocoa. Maybe darwin?
> 

Will move file to spi/darwin.

> > Source/WTF/wtf/spi/cocoa/XPCSPI.h:87
> > +EXTERN_C CONST_ON_OR_AFTER_MAC_OS_X_VERSION_1090 struct _xpc_dictionary_s _xpc_error_connection_invalid;
> 
> Where does EXTERN_C come from?

EXTERN_C is defined in wtf/Compiler.h, <http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Compiler.h?rev=174110#L160>. It was added in <http://trac.webkit.org/changeset/174110>. EXTERN_C is defined to be either 'extern "C"' or 'extern' depending on whether we are compiling a C++/Objective-C++ file or C/Objective-C file, respectively.

> I'd prefer a plain extern "C" - it's no more verbose, but clearly less fragile.

Notice that an extern "C"-block is only applicable in a C++/Objective-C++ source file since symbols declared in a C/Objective-C file have C-style linkage by default. So, we don't want 'extern "C"' to be in the source file. Otherwise, it will cause a compile error since "C" in 'extern "C"' is an unexpected string literal.

It seems reasonable to support including the header XPCSPI.h from a C, Objective-C file (especially the latter).

> 
> > Source/WTF/wtf/spi/cocoa/XPCSPI.h:99
> > +#if __BLOCKS__
> 
> We use "#ifdef __BLOCKS__" elsewhere in WebKit. Which is right?
> 

#ifdef __BLOCKS__ is a better form. I noticed that we also have COMPILER_SUPPORTS(BLOCKS). Unless you feel strongly to use #ifdef __BLOCKS__, I'll change this line to read:

#if COMPILER_SUPPORTS(BLOCKS)

This will make querying for the Block language extension consistent with how we query for other compiler extensions/features.

We should look to substitute COMPILER_SUPPORTS(BLOCKS) for __BLOCKS__ at other call sites. I suggest we do this in a separate patch.

> > Source/WTF/wtf/spi/cocoa/XPCSPI.h:152
> > +#if !defined(xpc_retain) && OS_OBJECT_USE_OBJC_RETAIN_RELEASE
> > +#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; })
> > +#endif
> > +
> > +#if !defined(xpc_release) && OS_OBJECT_USE_OBJC_RETAIN_RELEASE
> > +#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; })
> > +#endif
> 
> Does this compile in .cpp files?

Notice these macro defines are conditional on compiling a Objective-C/C++ file with a compiler that support ARC (by definition of OS_OBJECT_USE_OBJC_RETAIN_RELEASE in os/object.h). So, a C++ file that includes XPCSPI.h will compile (since these macros are not applicable).

[Daniel Bates]

Committed r174180: <http://trac.webkit.org/changeset/174180>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 137320

[Daniel Bates]

Committed r174220: <http://trac.webkit.org/changeset/174220>