Bug 16029 – JavaScriptCore.h is not suitable for platforms other than Mac OS X

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alp Toker]

r21342 introduced JSStringRefCF.h to JavaScriptCore.h:

  http://trac.webkit.org/projects/webkit/changeset/21342

This breaks sources that include JavaScriptCore.h on platforms without CF.

<rdar://problem/4885130> mentions that the plan was to have applications include JSStringRefCF.h as needed, which sounds like the right thing to do, but r21342 goes against that.

I'm investigating switching a couple of bindings from the C++ API to the new C API. This is the only issue that's come up so far.

It seems the most elegant solution would actually be to create a new top-level header, JavaScriptCoreCF.h, which includes CF-specific headers in addition to the portable JavaScriptCore.h. This does not represent an ABI break so I suspect it can be implemented right now with minimal disruption.

[Darin Adler]

JavaScriptCore.h is intended to be a Mac OS X only header -- the "framework header" that any Mac OS X framework has, which includes all its other headers. I don't suggest using it on other platforms. WebKit.h is the same way.

We can't change the contents of JavaScriptCore.h on Mac OS X now that Leopard has shipped with it as public API.

On other platforms, it might work better to include the appropriate headers individually.

If other platforms really want a catch-all header like JavaScriptCore.h we could come up with a build system that creates a different version of the header as needed for various platforms. Or they could use a different name for it.

[Mark Rowe (bdash)]

My thought was that we could wrap the line in JavaScriptCore.h that includes JSStringRefCF.h in a #ifdef of some type and have the build systems filter JavaScriptCore.h through unifdef to produce an appropriately cleaned version for the target platform.

[Alp Toker]

(In reply to comment #1)
> JavaScriptCore.h is intended to be a Mac OS X only header -- the "framework
> header" that any Mac OS X framework has, which includes all its other headers.
> I don't suggest using it on other platforms. WebKit.h is the same way.
> 
> We can't change the contents of JavaScriptCore.h on Mac OS X now that Leopard
> has shipped with it as public API.
> 
> On other platforms, it might work better to include the appropriate headers
> individually.

This isn't possible since some of the individual header files also include JavaScriptCore.h.

[Mark Rowe (bdash)]

The two header files (JSNodeList.h and JSNode.h) that do include JavaScriptCore.h are not part of the public API.

[Alp Toker]

(In reply to comment #4)
> The two header files (JSNodeList.h and JSNode.h) that do include
> JavaScriptCore.h are not part of the public API.
> 

Could you confirm which of these files are meant to be built as part of the JavaScriptCore library, which headers are meant to be shipped and which files are meant only to be part of the test applications?

(I tried to derive this from JavaScriptCore.xcodeproj but it is exceptionally difficult to follow. Are there freely available tools to view xcode project files? I will probably hit this issue again as I continue to audit more of WebKit's build system in preparation for release.)

APICast.h                    JSContextRef.cpp     JSStringRef.cpp
JavaScriptCore.h             JSContextRef.h       JSStringRef.h
JSBase.cpp                   JSNode.c             JSValueRef.cpp
JSBase.h                     JSNode.h             JSValueRef.h
JSCallbackConstructor.cpp    JSNodeList.c         minidom.c
JSCallbackConstructor.h      JSNodeList.h         minidom.html
JSCallbackFunction.cpp       JSObjectRef.cpp      minidom.js
JSCallbackFunction.h         JSObjectRef.h        Node.c
JSCallbackObject.cpp         JSRetainPtr.h        Node.h
JSCallbackObjectFunctions.h  JSStringRefBSTR.cpp  NodeList.c
JSCallbackObject.h           JSStringRefBSTR.h    NodeList.h
JSClassRef.cpp               JSStringRefCF.cpp    testapi.c
JSClassRef.h                 JSStringRefCF.h      testapi.js

[Mark Rowe (bdash)]

Public headers:
JSBase.h
JSContextRef.h
JSObjectRef.h
JSStringRef.h
JSStringRefCF.h
JSStringRefBSTR.h
JSValueRef.h
JavaScriptCore.h

Files used only when compiling WebKit:
APICast.h
JSBase.cpp
JSCallbackConstructor.cpp
JSCallbackConstructor.h
JSCallbackFunction.cpp
JSCallbackFunction.h
JSCallbackObject.cpp
JSCallbackObject.h
JSCallbackObjectFunctions.h
JSClassRef.cpp
JSClassRef.h
JSContextRef.cpp
JSObjectRef.cpp
JSRetainPtr.h
JSStringRef.cpp
JSStringRefBSTR.cpp
JSStringRefBSTR.h
JSStringRefCF.cpp
JSValueRef.cpp

testapi files:
testapi.c
testapi.js

minidom files:
JSNode.c
JSNodeList.c
Node.c
NodeList.c
minidom.c
minidom.html
minidom.js
JSNode.h
JSNodeList.h
Node.h
NodeList.h

[Alp Toker]

Mark,

Thanks for the clarification with those files. I've updated javascriptcore-portable.

The trouble with shipping different versions of JavaScriptCore/JavaScriptCore.h on Mac/non-Mac platforms is that code written on non-Mac which does not use or expect to pull in CF break when you attempt to compile it on a Mac.

So if's absolutely not an option to modify JavaScriptCore/JavaScriptCore.h, we need to consider shipping a different header on all platforms, say JavaScriptCore/JavaScript.h which includes only the portable header files.

(Keep in mind that the goal is not just to provide a similar API on Windows, Mac and Linux but to make it identical, so software builds and runs out of the box on each of these platforms as long as it's careful with platform dependencies.)

[Mark Rowe (bdash)]

Why would "code written on non-Mac which does not use or expect to pull in CF break when you attempt to compile it on a Mac"?

[Alp Toker]

(In reply to comment #8)
> Why would "code written on non-Mac which does not use or expect to pull in CF
> break when you attempt to compile it on a Mac"?
> 

It seemed to me that implicitly pulling CF headers into an application using GLib or Qt may cause trouble. Perhaps it wouldn't. My understanding of best portability practices is that it's good to avoid this kind of situation.

[Alp Toker]

Holger mentioned that C++-style comments "//" aren't accepted by some C compilers (he mentioned MSVC but I don't remember that being the case). There are some comments like this in the API headers.

If this turns out to be an issue, we might want to change those comments to increase portability. Does anyone have the facts on this?

[Alp Toker]

Created attachment 18314 [details]
Indroduce portable JavaScriptCore/JavaScript.h header

This patch seems to be the best way of addressing the needs of non-CF ports without breaking existing CF applications using the JavaScriptCore API.

JavaScriptCore/JavaScript.h is a new header that should be used in newly written portable software. It's expected to work exactly the same on all platforms, unlike JavaScriptCore/JavaScriptCore.h which will continue to require CFStringRef.

[Darin Adler]

Comment on attachment 18314 [details]
Indroduce portable JavaScriptCore/JavaScript.h header

Looks fine. r=me

[Alp Toker]

Landed in r29234.

[Timothy Hatcher]

(In reply to comment #2)
> My thought was that we could wrap the line in JavaScriptCore.h that includes
> JSStringRefCF.h in a #ifdef of some type and have the build systems filter
> JavaScriptCore.h through unifdef to produce an appropriately cleaned version
> for the target platform.

I think this is the better approach to fixing this. The patch landed and broke the Mac build.

Why can't JavaScriptCore.h just have a #ifdef __APPLE__ around the CF include?

[Timothy Hatcher]

(In reply to comment #14)
> Why can't JavaScriptCore.h just have a #ifdef __APPLE__ around the CF include?

I guess because Safari for Windows with CF....

[Alp Toker]

I'd have preferred to remove JSStringRefCF.h from JavaScriptCore.h and instead require applications to include JSStringRefCF.h, JSStringRefBSTR.h or the GLib/Qt equivalents as needed.

Checking __APPLE__ isn't a great idea since it could limit developers using configurations we haven't considered yet (eg. CF on Linux). Better to keep it simple and require explicit includes, but if this isn't an option, my patch is probably the best compromise.

[Darin Adler]

(In reply to comment #16)
> I'd have preferred to remove JSStringRefCF.h from JavaScriptCore.h and instead
> require applications to include JSStringRefCF.h, JSStringRefBSTR.h or the
> GLib/Qt equivalents as needed.

I know you'd prefer that, but it would be unacceptable for Mac OS X -- JavaScriptCore.h is a public Mac OS X 10.5 header, and we can't change it in an incompatible way.

> Checking __APPLE__ isn't a great idea since it could limit developers using
> configurations we haven't considered yet (eg. CF on Linux). Better to keep it
> simple and require explicit includes, but if this isn't an option, my patch is
> probably the best compromise.

Yes, agreed. But we need a version of your patch that doesn't break Mac OS X. Tim took care of that now.

[Alp Toker]

Nice build fixing. Thanks timothy and aroben.

I wonder though if the Mac build should use ForwardingHeaders for the JSC API like the GTK+ build. This would help keep minidom buildable out-of-tree against the system-installed SDK (since it's a nice code sample for people learning the API to try building themselves).