Bug 137277 - Clean up: Move XPC forward declarations in JavaScriptCore to WTF SPI wrapper header
Summary: Clean up: Move XPC forward declarations in JavaScriptCore to WTF SPI wrapper ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on: 137320
Blocks:
  Show dependency treegraph
 
Reported: 2014-09-30 17:11 PDT by Daniel Bates
Modified: 2014-10-02 09:24 PDT (History)
9 users (show)

See Also:


Attachments
Patch (19.91 KB, patch)
2014-09-30 17:14 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (20.19 KB, patch)
2014-10-01 11:19 PDT, 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-09-30 17:11:34 PDT
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.
Comment 1 Daniel Bates 2014-09-30 17:14:00 PDT
Created attachment 238978 [details]
Patch
Comment 2 WebKit Commit Bot 2014-09-30 17:15:20 PDT
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.
Comment 3 Daniel Bates 2014-10-01 11:19:58 PDT
Created attachment 239035 [details]
Patch

_xpc_error_connection_invalid and _xpc_error_termination_imminent are only const in OS X >= 10.9
Comment 4 WebKit Commit Bot 2014-10-01 11:21:44 PDT
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.
Comment 5 Alexey Proskuryakov 2014-10-01 13:54:55 PDT
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?
Comment 6 Daniel Bates 2014-10-01 15:21:01 PDT
(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).
Comment 7 Daniel Bates 2014-10-01 15:38:39 PDT
Committed r174180: <http://trac.webkit.org/changeset/174180>
Comment 8 WebKit Commit Bot 2014-10-01 16:24:24 PDT
Re-opened since this is blocked by bug 137320
Comment 9 Daniel Bates 2014-10-02 09:24:25 PDT
Committed r174220: <http://trac.webkit.org/changeset/174220>