Bug 222625 – [CMake] JavaScriptCore GLib headers should be copies

• Component: CMake
• Status: REOPENED

[Don Olmstead]

...

[Don Olmstead]

Created attachment 422011 [details]
WIP Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Don Olmstead]

Created attachment 422030 [details]
WIP Patch

[Don Olmstead]

Created attachment 422033 [details]
WIP Patch

[Don Olmstead]

Created attachment 422034 [details]
WIP Patch

[Don Olmstead]

Created attachment 422035 [details]
WIP Patch

[Don Olmstead]

Created attachment 422038 [details]
WIP Patch

[Don Olmstead]

Created attachment 422040 [details]
WIP Patch

[Don Olmstead]

Created attachment 422043 [details]
WIP Patch

[Don Olmstead]

Created attachment 422044 [details]
WIP Patch

[Don Olmstead]

Created attachment 422046 [details]
Patch

[EWS]

Committed r273814: <https://commits.webkit.org/r273814>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422046 [details].

[Michael Catanzaro]

Unfortunately it looks like this broke gobject-introspection. I will try to investigate, but we need to revert in the meantime, sorry.

[715/2392] Generating ../../JavaScriptCore-4.0.gir
../../../../Source/JavaScriptCore/API/glib/JSCClass.cpp:644: Warning: JavaScriptCore: jsc_class_add_constructorv: return value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCClass.cpp:688: Warning: JavaScriptCore: jsc_class_add_constructor_variadic: return value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:620: Warning: JavaScriptCore: jsc_context_get_virtual_machine: return value: Unresolved type: 'JSCVirtualMachine*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:635: Warning: JavaScriptCore: jsc_context_get_exception: return value: Unresolved type: 'JSCException*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:716: Warning: JavaScriptCore: jsc_context_throw_exception: argument exception: Unresolved type: 'JSCException*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:827: Warning: JavaScriptCore: jsc_context_evaluate: return value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:853: Warning: JavaScriptCore: jsc_context_evaluate_with_source_uri: return value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:868: Warning: JavaScriptCore: jsc_context_evaluate_in_object: argument object: Unresolved type: 'JSCValue**'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:885: Warning: JavaScriptCore: jsc_context_evaluate_in_object: return value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:930: Warning: JavaScriptCore: jsc_context_check_syntax: argument exception: Unresolved type: 'JSCException**'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:1024: Warning: JavaScriptCore: jsc_context_get_global_object: return value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:1033: Warning: JavaScriptCore: jsc_context_set_value: argument value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:1058: Warning: JavaScriptCore: jsc_context_get_value: return value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCContext.cpp:600: Warning: JavaScriptCore: jsc_context_new_with_virtual_machine: argument vm: Unresolved type: 'JSCVirtualMachine*'
../../../../Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:184: Warning: JavaScriptCore: jsc_weak_value_get_value: return value: Unresolved type: 'JSCValue*'
../../../../Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:163: Warning: JavaScriptCore: jsc_weak_value_new: argument value: Unresolved type: 'JSCValue*'

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #13)
> Unfortunately it looks like this broke gobject-introspection. I will try to
> investigate, but we need to revert in the meantime, sorry.

Don and I tried a lot of different things but couldn't figure out how to fix this. It's strange.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 222676

[Don Olmstead]

Created attachment 422228 [details]
GIR

[Don Olmstead]

Comment on attachment 422228 [details]
GIR

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

Going through step by step to see what's going on with the .gir build.

> Source/JavaScriptCore/PlatformGTK.cmake:90
> -            -I${CMAKE_SOURCE_DIR}/Source
> -            -I${JAVASCRIPTCORE_DIR}
> +            -I${JAVASCRIPTCORE_DIR}/API/glib
>              -I${DERIVED_SOURCES_JAVASCRIPCOREGTK_DIR}
>              -I${FORWARDING_HEADERS_DIR}/JavaScriptCore/glib

Feels like you'd just need access to the `<jsc/Foo.h>` headers which is in the `${FORWARDING_HEADERS_DIR}/JavaScriptCore/glib` and then the `${JAVASCRIPTCORE_DIR}/API/glib` which handles the `"Foo.h"` headers. The `${DERIVED_SOURCES_JAVASCRIPCOREGTK_DIR}
` is there for the `<jsc/JSCVersion.h>`.

This patch built with introspection so it seems sensible.

[Don Olmstead]

Created attachment 422235 [details]
GIR + JSC*Private API headers should not include JSC API headers

[Don Olmstead]

Created attachment 422236 [details]
GIR + JSC*Private API headers should not include JSC API headers

[Don Olmstead]

Created attachment 422255 [details]
GIR + JSC*Private API headers should not include JSC API headers

[Don Olmstead]

Created attachment 422257 [details]
GIR + JSC*Private API headers should not include JSC API headers

[Don Olmstead]

Created attachment 422259 [details]
GIR + previous + use <jsc/> includes in private headers

[Don Olmstead]

Comment on attachment 422257 [details]
GIR + JSC*Private API headers should not include JSC API headers

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

> Source/JavaScriptCore/API/glib/JSCContextPrivate.h:33
> -#include "APICast.h"
>  #include "JSCContext.h"
>  #include "JSCValue.h"
> -#include "JSContextRef.h"
>  #include <wtf/glib/GRefPtr.h>
>  
> +namespace JSC {
> +class JSObject;
> +}
> +
> +typedef struct OpaqueJSClass* JSClassRef;
> +typedef struct OpaqueJSContext* JSGlobalContextRef;
> +typedef const struct OpaqueJSValue* JSValueRef;
> +typedef struct OpaqueJSValue* JSObjectRef;

This version of the patch removes any headers outside of the GLib JSC directory and instead uses forward declarations. This only affects the JSC*Private.h includes which are additional C API's used in WebKit/WebProcess/InjectedBundle/API/glib/**/*.cpp files.

[Don Olmstead]

Comment on attachment 422259 [details]
GIR + previous + use <jsc/> includes in private headers

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

> Source/JavaScriptCore/API/glib/JSCClassPrivate.h:24
> -#include "JSCClass.h"
> -#include "JSCContext.h"
> -#include "JSCValue.h"
> +#include <jsc/JSCClass.h>
> +#include <jsc/JSCContext.h>
> +#include <jsc/JSCValue.h>

This next patch goes further and includes those files as <jsc/JSC*.h> rather than "JSC*.h".

This fits with how Apple's private JSC API headers work. They include <JavaScriptCore/*.h> rather than "*.h". This is because on disk the headers are stored separately.

See https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/API/JSBasePrivate.h as an example.

My personal preference is to do it this way but I am not a GLib maintainer. I think I want to split this bug along so one is focused on the API and one is focused on the CMake.

Would like to know GLib maintainers preference on which patch.

[Michael Catanzaro]

Comment on attachment 422259 [details]
GIR + previous + use <jsc/> includes in private headers

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

As far as I can tell, this is a misunderstanding of how GLib private headers relate to Apple headers. You've explained to me on IRC that Apple's private headers are actually installed and used by Apple apps. GLib private headers are not. These headers are only for use by WebKit itself, when building WebKit. So there should be no restrictions on what headers can be #included. In particular, there's no reason to try to avoid #include "APICast.h". Ideally GLib API private headers should be treated exactly the same as any other random file in JavaScriptCore, say B3ArgumentRegValue.h.

> Source/JavaScriptCore/PlatformGTK.cmake:73
> +            --warn-error

Good. :P

[Radar WebKit Bug Importer]

<rdar://problem/75238500>