Bug 164061 – [GTK][WPE] Initial implementation of JavaScriptCore glib bindings

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Estêvão Samuel Procópio Amaral]

Current API lacks a glib-like API for JavaScriptCore.
This is a simple initial implementation of a JavaScriptCore
bindings according to the discussion on webklit-gtk mailing
list thread[1].

[1] https://lists.webkit.org/pipermail/webkit-gtk/2016-October/002834.html

[Estêvão Samuel Procópio Amaral]

Created attachment 293034 [details]
Patch

[Estêvão Samuel Procópio Amaral]

Created attachment 293037 [details]
Patch

[Estêvão Samuel Procópio Amaral]

Created attachment 293039 [details]
Patch

[Estêvão Samuel Procópio Amaral]

Created attachment 293040 [details]
Patch

[Michael Catanzaro]

Comment on attachment 293040 [details]
Patch

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

Just a few initial observations:

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:40
> +jsc_boolean_class_init(JSCBooleanClass *)

Keep in mind that you need to follow WebKit coding style in the implementation files:

static void jscBooleanClassInit(JSCBooleanClass*)

The only exception is that API functions of course have to use underscores.

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:70
> +jsc_boolean_new(JSCContext *context, gboolean value)

You really can't do anything in these _new functions except call g_object_new, since the _new functions aren't exposed to introspection, the code will never be called from bindings. You want to make sure g_object_new works properly anyway. So everything needs to move to either _init or _constructed.

> Source/cmake/OptionsGTK.cmake:88
> +WEBKIT_OPTION_DEFINE(ENABLE_JSC_GLIB "Whether to enable support for JavaScriptCore glib bindings." PUBLIC OFF)

It should be alphabetized.

> Tools/Scripts/webkitpy/style/checker_unittest.py:289
> +           os.path.join('Source', 'JavaScriptCore', 'API', 'glib', 'JSCBoolean.h'),

This is just a unit test to make sure the style checker works, you shouldn't be modifying it.

[Estêvão Samuel Procópio Amaral]

Created attachment 293075 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 293075 [details]
Patch

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

Great job, it's a very good start, but still I have a lot of comments, most of them about coding style, WebKit practices, etc. it's normal in a first patch.

> Source/JavaScriptCore/ChangeLog:27
> +        * API/glib/jsc/JSCBoolean.h: Added.
> +        * API/glib/jsc/JSCBooleanPrivate.h: Added.
> +        * API/glib/jsc/JSCContext.h: Added.
> +        * API/glib/jsc/JSCContextPrivate.h: Added.
> +        * API/glib/jsc/JSCNumber.h: Added.
> +        * API/glib/jsc/JSCNumberPrivate.h: Added.
> +        * API/glib/jsc/JSCString.h: Added.
> +        * API/glib/jsc/JSCStringPrivate.h: Added.
> +        * API/glib/jsc/JSCValue.h: Added.
> +        * API/glib/jsc/JSCValuePrivate.h: Added.
> +        * API/glib/jsc/jsc.h: Added.

Don't do this, leave all headers an cpp files in the same directory, we will take careto install in the right jsc directory. Internally you can simply create a symlink in the forwarding headers directory. See how we do it for other apis in Source/WebKit2/PlatformGTK.cmake line 1423

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:25
> +

Remove this empty line, keep all includes in a single block.

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:40
> +jsc_boolean_class_init(JSCBooleanClass *)

JSCBooleanClass * -> JSCBooleanClass*

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:45
> +jsc_boolean_init(JSCBoolean *)

JSCBoolean * -> JSCBoolean*

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:49
> +JSCBoolean *

Ditto, and the same everywhere, I wonder why style checker didn't complain about this.

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:50
> +jscBooleanNewFromJSValue(JSCContext *context, JSValueRef jsvalue)

For internal new methods we normally use Create, so this could be just jscBooleanCreate()

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:54
> +    JSCBoolean* boolean;
> +
> +    boolean = (JSCBoolean *) g_object_new(JSC_TYPE_BOOLEAN, "context", context, NULL);

Use the same line, don't use C style casts, in this case you could use JSC_BOOLEAN() macro, and use nullptr instead of NULL.

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:70
> +jsc_boolean_new(JSCContext *context, gboolean value)

All jsc value new methods should return a JSCValue.

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:72
> +    JSContextRef jscontext;

jscontext -> jsContext

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:73
> +    JSValueRef jsvalue;

jsvalue -> jsValue

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:76
> +    jscontext = jscContextGetJSContext(context);
> +    jsvalue = JSValueMakeBoolean(jscontext, value);

Use the same line for the variable delcarations

JSContextRef jsContext = jscContextGetJSContext(context);
JSValueRef jsValuie = JSValueMakeBoolean(jsContext, value);

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:78
> +    return jscBooleanNewFromJSValue(context, jsvalue);

As Michael said, it's problematic to do all these things in a new method, but it's not easy to solve, because we don't want to expose JS values in our API and GObject doesn't have a way to declare private properties. So, I'm not sure how to create a value with a JScontext associated only with g_object_new if we can't use construct properties.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:29
> +

Remove this empty line.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:38
> + * A #JSCContext is the central class of the JavaScriptCore API. It is

Do not reference JSCContext here because it points to this block

> Source/JavaScriptCore/API/glib/JSCContext.cpp:46
> +    JSGlobalContextRef context;

Use smart pointers in the private struct. Then use the placement new syntax in the init method to call the constructor and call the destructor in finalize. We do this automatically in wk API, we could probably move the WEBKIT_DEFINE_TYPE macro to WTF, but for now do it manually. For the JSGlobalContextRef you should use JSRetainPtr<JSGlobalContextRef>

> Source/JavaScriptCore/API/glib/JSCContext.cpp:47
> +    JSObjectRef object;

What is this object? the global object? Call it globalObject if that is the case.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:49
> +    GList* values;

What's this? cached values? or what values? For internal implementation we usually prefer to use WTF types instead of glib types. So this should probably be Vector<GRefPtr<JSCValue>> or HashSet<GRefPtr<JSCValue>>

> Source/JavaScriptCore/API/glib/JSCContext.cpp:55
> +static void
> +jscContextDispose(GObject* obj)

Forgot to say before but this should be a single line, and the same for all other functions.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:59
> +    JSCContextPrivate* priv;
> +
> +    priv = (JSCContextPrivate*) jsc_context_get_instance_private(JSC_CONTEXT(obj));

Since line and don't use C style casts.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:67
> +    g_list_free_full(priv->values, g_object_unref);
> +
> +    if (!priv->context) {
> +        JSGlobalContextRelease(priv->context);
> +        priv->context = nullptr;
> +        priv->object = nullptr;
> +    }

And you don't need this with smart pointers.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:88
> +    priv->object = JSContextGetGlobalObject(priv->context);

Why do we need to keep this? Could we get it on demand when needed?

> Source/JavaScriptCore/API/glib/JSCContext.cpp:89
> +    priv->values = nullptr;

This is already nullptr, the private struct is 0 filles on allocation by glib.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:90
> +}

You added JSCContextPrivate *priv; to the instance struct, why don't you init that here and then you don't need to call jsc_context_get_instance_private everywhere? or remove the priv from the instance struct if this is the new way of handling the private struct.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:110
> +jsc_context_new(void)

(void) -> ()

> Source/JavaScriptCore/API/glib/JSCContext.cpp:132
> +    if (JSValueIsUndefined(priv->context, jsvalue) || JSValueIsNull(priv->context, jsvalue))
> +        return nullptr;

We need to handle this, null is not the same as undefined

> Source/JavaScriptCore/API/glib/JSCContext.cpp:134
> +    g_assert_not_reached();

Use wk asserts, ASSERT_NOT_REACHED();

> Source/JavaScriptCore/API/glib/JSCContext.cpp:145
> + * Returns: A JSCValue subtype representing the value of the requested property

JSCValue -> #JSCValue I would remove the "subtype", JSCValue is an abstract class.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:159
> +    JSStringRelease(strName);

You can use JSRetainPtr with JSStringRef too.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:178
> +

Forgot to say before, but you should use g_return macros in all public methods to check all passed in arguments.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:184
> +    if (g_object_is_floating(value)) {
> +        g_object_ref_sink(value);
> +        priv->values = g_list_append(priv->values, value);
> +    }

Ok, so we need to keep the values because we are taking the ownership. This is what I said in the mailing list that we need to discuss about memory management, objects lifecycle, etc. Because here we take the ownership and we don't release the object until the context is destroyed. In this particular case we should release the wrapper if the js object is garbage collected, but it will never be garbage collected because we have protected it. Also, even though it's not common, nothing prevents the user to take an extra ref after passing the floating one to the context, in that case we want to protect the value until the user releases its ref. This is probably the most complex part of the API that we need to think carefully. In any case, with smart pointers, and assuming we use a HashSet for this, because we don't need to keep the order, this would be something like:

priv->cachedValues.add(value);

We don't need to worry about sinking the floating ref, because GRefPtr already does that.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:189
> +    JSObjectSetProperty(priv->context, priv->object, strName, jsvalue,
> +        kJSPropertyAttributeReadOnly, nullptr);

This could be a single line, we use 120 as a limit.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:190
> +    JSStringRelease(strName);

Use JSRetainPtr

> Source/JavaScriptCore/API/glib/JSCNumber.cpp:61
> +jsc_number_new(JSCContext* context, gdouble value)

new_double

> Source/JavaScriptCore/API/glib/JSCValue.cpp:44
> +    JSCContext* jscContext;
> +    JSGlobalContextRef context;

Why do we need to keep both? we can get the JS one from the JSC one

> Source/JavaScriptCore/API/glib/JSCValue.cpp:48
> +G_DEFINE_TYPE_WITH_PRIVATE(JSCValue, jsc_value, G_TYPE_INITIALLY_UNOWNED)

This should be abstract

> Source/JavaScriptCore/API/glib/JSCValue.cpp:81
> +        JSCContext* context = JSC_CONTEXT(g_value_get_object(value));
> +        priv->jscContext = context;
> +        priv->context = context ? jscContextGetJSContext(context) : nullptr;

We should not allow to create a JSCValue without a JSCContext

> Source/JavaScriptCore/API/glib/JSCValue.cpp:98
> +        priv->context = nullptr;
> +        priv->value = nullptr;

So, we are not taking reference of the context, I guess it's because the context has a ref of all values, but the user can take an extra ref of the value, then the context is destroyed releasing all the cached values, and we end up with a value pointing to a destroyed context.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:126
> +            (GParamFlags) (G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));

use static_cast here.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:150
> +    if (priv->value != nullptr) {

Don't compare to nullptr, do if (priv->value)

> Source/JavaScriptCore/API/glib/JSCValue.cpp:151
> +        JSValueUnprotect(priv->context, priv->value);

How can this happen? Aren't wrappers inmutable?

> Source/JavaScriptCore/API/glib/JSCValue.cpp:179
> +/**
> + * jsc_value_get_string:
> + * @value: the #JSCValue subtype containing the value
> + *
> + * Retrieves the string from #JSCValue
> + *
> + * Returns: gchar*
> + */
> +const gchar*
> +jsc_value_get_string(JSCValue* value)

So in the end you are mixing the idea of having subtypes with the idea of having a single value object. If we use subtypes, this doesn't belong here, but to JSCValueString

> Source/JavaScriptCore/API/glib/JSCValue.cpp:197
> +    return g_strdup(buff);

The function returns a const char, but here you are allocating the string, leaking it and then returning a copy that the user is not expected to free, so 2 leaks here. Here again we need to decide, if we want to return a const char * to make ti easuier to handle for the user we need to keep the string somewhere and free it, otherwise we return a new allocated srtring and document that the user should free it.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:206
> + * Returns: gboolean

You could say at least A #gboolean instead of just gboolean

> Source/JavaScriptCore/API/glib/JSCValue.cpp:209
> +gboolean
> +jsc_value_get_boolean(JSCValue *value)

Same here, this should be in JSCBoolean, jsc_boolean_get_boolean or get_value to avoid the bool_get_bool

> Source/JavaScriptCore/API/glib/JSCValue.cpp:230
> +jsc_value_get_double(JSCValue *value)

jsc_number_get_double

> Source/JavaScriptCore/API/glib/JSCValue.cpp:250
> +gint

gint -> gint32

> Source/JavaScriptCore/API/glib/JSCValue.cpp:260
> +    return JSValueToNumber(priv->context, priv->value, nullptr);

JSValueToNumber always returns a double, you need to convert the value to a int32 properly ensuring it doesn't overflow. You can use JSC::toInt32 from MathCommon.h for that

> Source/JavaScriptCore/API/glib/JSCValue.cpp:271
> +guint

guint32

> Source/JavaScriptCore/API/glib/JSCValue.cpp:281
> +    return (guint) JSValueToNumber(priv->context, priv->value, nullptr);

Same comments here.

> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:20
> +#pragma once

We can't use #pragma once in public headers, use the #ifndef approach.

> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:23
> +#include "JSCContext.h"
> +#include "JSCValue.h"

Use always angle-bracket includes in public headers

> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:25
> +#include <JavaScriptCore/JSBase.h>

This is exposing the JSC C API, don't do this.

> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:48
> +JS_EXPORT GType
> +jsc_boolean_get_type (void);

This should be a single line.

> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:51
> +JS_EXPORT JSCBoolean*
> +jsc_boolean_new      (JSCContext*, gboolean value);

Single line too. 
JSCBoolean* -> JSCBoolean *
Parameters goes in new line and names are lined up. See all other public headers. And follow the same for all others (only the public ones not Private.h)

> Source/JavaScriptCore/API/glib/jsc/JSCBooleanPrivate.h:25
> +JS_EXPORT JSCBoolean*
> +jscBooleanNewFromJSValue(JSCContext*, JSValueRef);

Single line.

> Source/JavaScriptCore/API/glib/jsc/JSCValue.h:66
> +/* JS_EXPORT JSCContext * */
> +/* jsc_value_get_context (JSCValue *value); */

Do not include commented code like this.

> Source/JavaScriptCore/PlatformGTK.cmake:52
> +if (ENABLE_JSC_GLIB)

I don't think we should make this conditional. It doesn't add any new dependency and the api should always be available.

> Source/JavaScriptCore/PlatformGTK.cmake:82
> +    install(FILES
> +        ${jscglib_HEADERS}
> +            DESTINATION "${WEBKITGTK_HEADER_INSTALL_DIR}/jsc"

Do not add any install command for now.

> Tools/ChangeLog:18
> +        * TestWebKitAPI/jsc-glib/JSCValuesTest.cpp: Added.
> +        * TestWebKitAPI/jsc-glib/JSCValuesTest.h: Added.
> +        * TestWebKitAPI/jsc-glib/main.cpp: Added.

Don't use jsc-glib for the directory name, either use use jsc/glib/ or JavaScriptCoreGlib. also the tests should be in the Tests directory

> Tools/TestWebKitAPI/jsc-glib/JSCValuesTest.cpp:21
> +#include "JSCValuesTest.h"

I would call it JSCValueTest not Values

> Tools/TestWebKitAPI/jsc-glib/JSCValuesTest.cpp:26
> +ContextFixtureSetUp(ContextFixture *fixture, gconstpointer userData)

ContextFixtureSetUp -> contextFixtureSetUp

> Tools/TestWebKitAPI/jsc-glib/JSCValuesTest.cpp:38
> +    fixture->context = jsc_context_new();
> +    jsc_context_set_value(fixture->context, "huvs",  JSC_VALUE(jsc_string_new(fixture->context, "nivs")));
> +    jsc_context_set_value(fixture->context, "true",  JSC_VALUE(jsc_boolean_new(fixture->context, true)));
> +    jsc_context_set_value(fixture->context, "false", JSC_VALUE(jsc_boolean_new(fixture->context, false)));
> +    jsc_context_set_value(fixture->context, "number-z", JSC_VALUE(jsc_number_new(fixture->context, 0.0)));
> +    jsc_context_set_value(fixture->context, "number-p", JSC_VALUE(jsc_number_new(fixture->context, 1.1)));
> +    jsc_context_set_value(fixture->context, "number-n", JSC_VALUE(jsc_number_new(fixture->context, -1.1)));
> +    // jsc_context_set_value(fixture->context, "int0", JSC_VALUE(jsc_boolean_new(fixture->context, false)));
> +    // jsc_context_set_value(fixture->context, "int1", JSC_VALUE(jsc_boolean_new(fixture->context, false)));
> +    // jsc_context_set_value(fixture->context, "int-1", JSC_VALUE(jsc_boolean_new(fixture->context, false)));
> +    // jsc_context_set_value(fixture->context, "uintff", JSC_VALUE(jsc_boolean_new(fixture->context, false)));

I think all tests cases should be independent to each other, so add the valyes you need in every test case instead of adding all of them here.

> Tools/TestWebKitAPI/jsc-glib/JSCValuesTest.cpp:52
> +    JSCValue* value = jsc_context_get_value(fixture->context, "huvs");
> +    g_assert_true(JSC_IS_STRING(value));
> +    g_assert_cmpstr(jsc_value_get_string(value), ==, "nivs");

We need to test a lot more things, and a edge cases. Create more than one context, check what happens if you ask for a value that doesn't exist, etc. And we should also check the in tests the memory manage ment and objects lifecycle.

> Tools/TestWebKitAPI/jsc-glib/main.cpp:31
> +    g_test_add("/jsc-values/string-test", ContextFixture, NULL,

Maybe we can start with g_test_add_func to make it simpler, and every test will create and destroy its context.

[Estêvão Samuel Procópio Amaral]

Created attachment 294417 [details]
Patch

[Estêvão Samuel Procópio Amaral]

Some considerations:

* Copied Source/WebKit2/UIProcess/API/gtk/WebKitPrivate.h to Source/JavaScriptCore/API/glib/JSCPrivate.h and added WEBKIT_DEFINE_SIMPLE_TYPE which defines a type without private structure (couldn't find a better name. Maybe when movind this stuff to WTF we sould use WEBKIT_DEFINE_TYPE and WEBKIT_DEFIINE_TYPE_WITH_PRIVATE?

* JSCValue stores a weak reference of the context it belongs to.

* Values are now cached in a HashMap inside JSCContext object and freed on context destruction.

* GRefPtrs and JSRetainPtrs were used to automatically ref/unref objects.

* Some object lifecycle tests were implemented to check if objects were really deallocated.

Hope this time I get less comments :P

[Carlos Garcia Campos]

Comment on attachment 294417 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        according to the discussion on webklit-gtk mailing list thread

webklit-gtk -> webkit-gtk

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:46
> +JSCBoolean* jscBooleanCreate(JSCContext* context, JSValueRef jsValue)

I think it's easier if this also returns a JSCValue*

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:51
> +    JSCBoolean* boolean = JSC_BOOLEAN(g_object_new(JSC_TYPE_BOOLEAN, "context", context, NULL));

NULL -> nullptr

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:52
> +    jscValueSetJSValue(JSC_VALUE(boolean), jsValue);

If this returns a JSCValue, you only need to cast the g_object_new(), and here we don't need to cast again.

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:68
> +    g_return_val_if_fail(JSC_IS_CONTEXT(context), false);

false -> nullptr

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:73
> +    JSContextRef jsContext = jscContextGetJSContext(context);
> +    JSValueRef jsValue = JSValueMakeBoolean(jsContext, value);
> +
> +    return JSC_VALUE(jscBooleanCreate(context, jsValue));

If Create returns a JSCValue you don't need to cast here once again. I think this could be a single line:

return jscBooleanCreate(context, JSValueMakeBoolean(jscContextGetJSContext(context), value)));

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:86
> +    g_return_val_if_fail(JSC_IS_BOOLEAN(boolean), false);

false -> FALSE

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:89
> +    JSCContext* context;
> +    g_object_get(G_OBJECT(context), "context", &context, nullptr);

You are getting the context from the context here, I guess it should be boolean instead of context. But don't use g_object_get for this, add a private method to JSCValue to get its context without taking any extra reference.

> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:95
> +    return JSValueToBoolean(jsContext, jsValue);

return JSValueToBoolean(jscContextGetJSContext(context), jscValueGetJSValue(JSC_VALUE(boolean));

> Source/JavaScriptCore/API/glib/JSCBoolean.h:43
> +    JSCValueClass parentClass;

parent_class

> Source/JavaScriptCore/API/glib/JSCBoolean.h:48
> +JSCValue* jsc_boolean_new       (JSCContext* context,

JSCContext* context -> JSCContext *context

In public headers we follow the GNOME style.

> Source/JavaScriptCore/API/glib/JSCBoolean.h:51
> +gboolean  jsc_boolean_get_value (JSCBoolean* value);

JSCBoolean* value -> JSCBoolean *value

I'm not sure value is the best name here, it's a bit weird that the function is get_value and receives a value parameter. And in _new() value refers to the gboolean too, not the object. I would use boolean instead, like in the implementation

> Source/JavaScriptCore/API/glib/JSCBooleanPrivate.h:25
> +

Remove this extra line

> Source/JavaScriptCore/API/glib/JSCContext.cpp:49
> +typedef HashMap<const gchar*, GRefPtr<JSCValue> > ValueMap;

const char*? Who is the owner of that pointer? How do you know it will be alive for the life of the map? Use CString better. Also you don't need the extra space between > >

> Source/JavaScriptCore/API/glib/JSCContext.cpp:55
> +    _JSCContextPrivate()
> +    {
> +        context = JSRetainPtr<JSGlobalContextRef>(Adopt, JSGlobalContextCreate(nullptr));
> +    }

I think we can add a GObjectClass::constructed implementation and do all the initializations there.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:60
> +    ~_JSCContextPrivate()
> +    {
> +        context.clear();
> +    }

This is done automatically, you don't need to explicitly clear the smart pointer.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:74
> +static void jscContextDispose(GObject* obj)
> +{
> +    JSCContextPrivate* priv = JSC_CONTEXT(obj)->priv;
> +
> +    if (priv->cachedValues.size() > 0)
> +        priv->cachedValues.clear();
> +}

We don't need this unless we really want to explicitly clear the cache on dispose. This will be done automatically by the JSCContextPrivate destructor on finalize().

> Source/JavaScriptCore/API/glib/JSCContext.cpp:89
> +    JSCContextPrivate* priv = context->priv;
> +
> +    return priv->context.get();

return context->priv->context.get();

> Source/JavaScriptCore/API/glib/JSCContext.cpp:122
> +        return JSC_VALUE(jscStringCreate(context, jsValue));
> +
> +    if (JSValueIsBoolean(priv->context.get(), jsValue))
> +        return JSC_VALUE(jscBooleanCreate(context, jsValue));
> +
> +    if (JSValueIsNumber(priv->context.get(), jsValue))
> +        return JSC_VALUE(jscNumberCreate(context, jsValue));
> +
> +    if (JSValueIsUndefined(priv->context.get(), jsValue))
> +        return JSC_VALUE(jscUndefinedCreate(context, jsValue));

You don't need all these JSC_VALUE casts if Create functions return a JSCValue

> Source/JavaScriptCore/API/glib/JSCContext.cpp:125
> +    if (JSValueIsNull(priv->context.get(), jsValue))
> +        return nullptr;

I'm not sure if we will need a null value implementation too. In any case, you are handling here the null value, but null is not allowed in jsc_context_set_value

> Source/JavaScriptCore/API/glib/JSCContext.cpp:165
> +    if (!cachedValue) {
> +        // If we don't have a cached value, we cache it
> +        cachedValue = jsValueToJSCValue(context, jsValue);
> +        priv->cachedValues.set(name, cachedValue);
> +    } else {
> +        // If we already have a cached value for this property, we check if the value has changed
> +        JSValueRef cachedJSValue = jscValueGetJSValue(cachedValue);
> +        if (!JSValueIsStrictEqual(priv->context.get(), jsValue, cachedJSValue)) {
> +            cachedValue = jsValueToJSCValue(context, jsValue);
> +            priv->cachedValues.set(name, cachedValue);
> +        }
> +    }

This is not how we want to cache the values, I think. We should cache all wrapped values, not only the ones added to the global object. As I said in a previous review, let's forget about objects lifecycle for now, do not cache anything, to make this simpler, and then we can discuss, design and implement the way we want to handle objects lifecycle.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:189
> +    JSObjectSetProperty(priv->context.get(), object, strName.get(), jsValue, kJSPropertyAttributeReadOnly, nullptr);

Why kJSPropertyAttributeReadOnly? That means we can call this twice for the same name to update the value.

> Source/JavaScriptCore/API/glib/JSCContext.h:51
> +JSCContext*

JSCContext* -> JSCContext *

> Source/JavaScriptCore/API/glib/JSCContext.h:54
> +JSCValue*

JSCValue* -> JSCValue *

> Source/JavaScriptCore/API/glib/JSCContext.h:56
> +jsc_context_get_value (JSCContext*  context,
> +                       const gchar* name);

Same here about the *

> Source/JavaScriptCore/API/glib/JSCNumber.cpp:58
> +JSCValue* jsc_number_new_double(JSCContext* context, gdouble value)

This needs documentation

> Source/JavaScriptCore/API/glib/JSCNumber.cpp:68
> +JSCValue* jsc_number_new_int32(JSCContext* context, gint32 value)

Ditto.

> Source/JavaScriptCore/API/glib/JSCNumber.cpp:78
> +JSCValue* jsc_number_new_uint32(JSCContext* context, guint32 value)

Ditto.

> Source/JavaScriptCore/API/glib/JSCNumber.cpp:110
> +gdouble jsc_number_get_double(JSCNumber* number)

Since all other JSCValue implementations use the pattern jsc_foo_get_value, maybe for numbr we could use jsc_number_get_value_as_double|int32|uint32

> Source/JavaScriptCore/API/glib/JSCNumber.cpp:125
> +guint jsc_number_get_uint32(JSCNumber* number)

guint -> guint32

> Source/JavaScriptCore/API/glib/JSCNumber.cpp:140
> +gint jsc_number_get_int32(JSCNumber* number)

gint -> gint32

> Source/JavaScriptCore/API/glib/JSCNumber.h:62
> +jsc_number_get_double (JSCNumber*);

Do not omit parameter names in public headers

> Source/JavaScriptCore/API/glib/JSCPrivate.h:22
> +#define WEBKIT_DEFINE_SIMPLE_TYPE(TypeName, type_name, TYPE_PARENT) \

Why is this needed? What's different compared to G_DEFINE_TYPE?

> Source/JavaScriptCore/API/glib/JSCString.cpp:39
> +WEBKIT_DEFINE_SIMPLE_TYPE(JSCString, jsc_string, JSC_TYPE_VALUE)

Can't we use G_DEFINE_TYPE here?

> Source/JavaScriptCore/API/glib/JSCString.cpp:97
> +    return g_strdup("the string value");

What? I guess this should be return buff

> Source/JavaScriptCore/API/glib/JSCValue.cpp:53
> +    kPROP_0,

Do not use k prefix for this

> Source/JavaScriptCore/API/glib/JSCValue.cpp:58
> +static void jscValueGetProperty(GObject* obj, guint prop_id, GValue* value, GParamSpec* param_spec)

obj ->object, prop_id -> propID, param_spec -> paramSpec

> Source/JavaScriptCore/API/glib/JSCValue.cpp:64
> +        g_value_set_object(value, priv->context);

g_value_set_object(value, JSC_VALUE(obj)->priv->context);

> Source/JavaScriptCore/API/glib/JSCValue.cpp:71
> +static void jscValueSetProperty(GObject* obj, guint prop_id, const GValue* value, GParamSpec* param_spec)

obj ->object, prop_id -> propID, param_spec -> paramSpec

> Source/JavaScriptCore/API/glib/JSCValue.cpp:79
> +        g_object_add_weak_pointer(G_OBJECT(context), (gpointer*)&priv->context);

Use C++ style cast. I'm not sure I understand this weak pointer, since we only null check the context in dispose method. I think the context should never be released if it still contains values, so values should take a reference to the context.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:123
> +            (GParamFlags) (G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));

static_cast<GParamFlags>()

> Source/JavaScriptCore/API/glib/JSCValue.cpp:132
> +    JSCValuePrivate* priv = value->priv;
> +
> +    return priv->value;

return value->priv->value;

> Source/JavaScriptCore/API/glib/jsc.h:28
> +

JSCUndefined is missing here.

> Source/JavaScriptCore/PlatformGTK.cmake:52
> +if (ENABLE_JSC_GLIB)

As I said in my previous review: I don't think we should make this conditional. It doesn't add any new dependency and the api should always be available.

> Tools/ChangeLog:17
> +        * TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCBoolean.cpp: Added.

Since we already have a TestWebKitAPI/Tests/JavaScriptCore dir that is currently empty, I would add a glib subdir there for our tests.

> Tools/TestWebKitAPI/JavaScriptCoreGlib/main.cpp:26
> +#include "Tests/JavaScriptCoreGlib/TestJSCBoolean.h"
> +#include "Tests/JavaScriptCoreGlib/TestJSCContext.h"
> +#include "Tests/JavaScriptCoreGlib/TestJSCNumber.h"
> +#include "Tests/JavaScriptCoreGlib/TestJSCString.h"
> +#include "Tests/JavaScriptCoreGlib/TestJSCUndefined.h"

The main should be in Tests/JavaScriptCoreGlib/ too. Do not add a file for every test case, move them all here to the same file and rename this as TestJSCValue for example

> Tools/TestWebKitAPI/JavaScriptCoreGlib/main.cpp:34
> +    g_test_add_func("/JavaScriptCoreGlib/JSCContext",   testJSCContext);

And for tests cases I would use /jsc/jsc-value/foo pattern

> Tools/TestWebKitAPI/PlatformGTK.cmake:165
> +    add_executable(TestJavaScriptCoreGlib

This is created in bin directlry, it should be in ${TESTWEBKITAPI_RUNTIME_OUTPUT_DIRECTORY}/JavaScriptCore/glib, I think you need to use add_test()

> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCBoolean.cpp:36
> +    getValue = jsc_context_get_value(context, "true");

JSCValue* getValue = jsc_context_get_value(context, "true");

> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCContext.cpp:35
> +    JSCContext* context = jsc_context_new();
> +    g_object_add_weak_pointer(G_OBJECT(context), (gpointer*)&context);
> +
> +    g_object_ref(context);
> +    g_object_unref(context);
> +    g_assert(context);
> +
> +    g_object_unref(context);
> +    g_assert(!context);

Here you are actually testing GObject weak references, since you are checking that GObject is setting the pointer to null when destroyed. I don't think we need a specific test case for contexts, since they ar eused everywhere, they will be tested in all other tests.

> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCNumber.cpp:83
> +    g_assert_cmpfloat(jsc_number_get_uint32(JSC_NUMBER(n)), ==, 4294967295);

g_assert_cmpuint

> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCNumber.cpp:116
> +    g_assert_cmpfloat(jsc_number_get_int32(JSC_NUMBER(n)), ==, -2147483648);

g_assert_cmpint

> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCUndefined.cpp:40
> +    g_assert(JSC_IS_VALUE(nonExistant));
> +    g_assert(JSC_IS_UNDEFINED(nonExistant));

This is redundant, if it's UNDEFINED, it's VALUE

[Estêvão Samuel Procópio Amaral]

Comment on attachment 294417 [details]
Patch

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

>> Source/JavaScriptCore/API/glib/JSCContext.cpp:125
>> +        return nullptr;
> 
> I'm not sure if we will need a null value implementation too. In any case, you are handling here the null value, but null is not allowed in jsc_context_set_value

Yes.. I thought about that too... Would it be too ugly if we accept nullptr as a valid input for jsc_context_set_value and create the null JSValue internally? :P
Nah... I think a JSCNull will be better...

>> Source/JavaScriptCore/API/glib/JSCContext.cpp:165
>> +    }
> 
> This is not how we want to cache the values, I think. We should cache all wrapped values, not only the ones added to the global object. As I said in a previous review, let's forget about objects lifecycle for now, do not cache anything, to make this simpler, and then we can discuss, design and implement the way we want to handle objects lifecycle.

Agreed. Will postpone that and change all the tests to unref every value (*ouch*) ;)

I see 2 ways of caching every value: 
1) we can make JSCContext a factory of values or 
2) we can make add a privte jscContextCacheValue and call it on constructed of JSCValue...

Just don't know what will be the best structure to hold those values... is that a good practice to use JSValueRef as the key of the hashmap?

I did some tests and found out the following:
when you create a JSValueRef, set it in the global object and retrieve it back, it points to the same structure.
If you evaluate a script to change the value, the retrieved JSValueRef is differrent from the set one. A new value is created. This logic allows us use JSValueRef as the hashmap key, but I'm not sure if it's a good practice...

>> Source/JavaScriptCore/API/glib/JSCContext.cpp:189
>> +    JSObjectSetProperty(priv->context.get(), object, strName.get(), jsValue, kJSPropertyAttributeReadOnly, nullptr);
> 
> Why kJSPropertyAttributeReadOnly? That means we can call this twice for the same name to update the value.

No valid reason for this. Already changed to kJSPropertyAttributeNone. :P

I think that we'll need a way to change that parameter though... Some use cases may not want JavaScript to change a value or delete it... I think we can add another parameter to jsc_context_set_value or maybe to JSCValue.

I think that adding this info to JSCValue will be better, because we can still use the same jsc_context_set_value to expose callbacks and full objects. Depending on the type, we retrieve different properties and use them internally.

>> Source/JavaScriptCore/API/glib/JSCPrivate.h:22
>> +#define WEBKIT_DEFINE_SIMPLE_TYPE(TypeName, type_name, TYPE_PARENT) \
> 
> Why is this needed? What's different compared to G_DEFINE_TYPE?

It's just to explicitly remove type_name_init and type_name_class_init from cpp files in subtypes and avoid style-checker issues. if that's not a problem, I'll switch back to G_DEFINE_TYPE.

>> Source/JavaScriptCore/API/glib/JSCString.cpp:97
>> +    return g_strdup("the string value");
> 
> What? I guess this should be return buff

Of course it is! I was testing something and forgot about that. Sorry! :P

>> Source/JavaScriptCore/API/glib/JSCValue.cpp:79
>> +        g_object_add_weak_pointer(G_OBJECT(context), (gpointer*)&priv->context);
> 
> Use C++ style cast. I'm not sure I understand this weak pointer, since we only null check the context in dispose method. I think the context should never be released if it still contains values, so values should take a reference to the context.

Hrm... if we do this, we won't be able to simply g_object_unref the context. We'll need to add an API to clear the values before unreffing the context, so that it will be correctly destroyed. That's why I used a weak ref...

And I got an error when using static_cast here...

>> Source/JavaScriptCore/PlatformGTK.cmake:52
>> +if (ENABLE_JSC_GLIB)
> 
> As I said in my previous review: I don't think we should make this conditional. It doesn't add any new dependency and the api should always be available.

I'm just afraid of breaking things up :P

>> Tools/TestWebKitAPI/JavaScriptCoreGlib/main.cpp:26
>> +#include "Tests/JavaScriptCoreGlib/TestJSCUndefined.h"
> 
> The main should be in Tests/JavaScriptCoreGlib/ too. Do not add a file for every test case, move them all here to the same file and rename this as TestJSCValue for example

I can do that. But I followed gtk:

Tools/TestWebKitAPI/gtk/main.cpp
Tools/TestWebKitAPI/Tests/WebKit2Gtk/

And I splited the files for readability... but I'll merge them.

[Carlos Garcia Campos]

(In reply to comment #11)
> Comment on attachment 294417 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294417&action=review
> 
> >> Source/JavaScriptCore/API/glib/JSCContext.cpp:125
> >> +        return nullptr;
> > 
> > I'm not sure if we will need a null value implementation too. In any case, you are handling here the null value, but null is not allowed in jsc_context_set_value
> 
> Yes.. I thought about that too... Would it be too ugly if we accept nullptr
> as a valid input for jsc_context_set_value and create the null JSValue
> internally? :P
> Nah... I think a JSCNull will be better...

I'm not sure, TBH. Maybe at some point we need to distinguish between a function returning nullptr to express an error and returning nullptr as a the JavaScript null value. So, I don't know, maybe we can start using nullptr and add the object later of needed to make things easier.

> >> Source/JavaScriptCore/API/glib/JSCContext.cpp:165
> >> +    }
> > 
> > This is not how we want to cache the values, I think. We should cache all wrapped values, not only the ones added to the global object. As I said in a previous review, let's forget about objects lifecycle for now, do not cache anything, to make this simpler, and then we can discuss, design and implement the way we want to handle objects lifecycle.
> 
> Agreed. Will postpone that and change all the tests to unref every value
> (*ouch*) ;)

Don't worry too much about leaking in the tests for now, we will fix everything eventually once we have the liefcycle management implemented.

> I see 2 ways of caching every value: 
> 1) we can make JSCContext a factory of values or 
> 2) we can make add a privte jscContextCacheValue and call it on constructed
> of JSCValue...

Not easy for me to give an opinion about this, because I don't fully understand how to the wrapped api objects and internal objects work in JavaScriptCore. We need to understand how things work in JSC to make a decision.

> Just don't know what will be the best structure to hold those values... is
> that a good practice to use JSValueRef as the key of the hashmap?

I think so.

> I did some tests and found out the following:
> when you create a JSValueRef, set it in the global object and retrieve it
> back, it points to the same structure.
> If you evaluate a script to change the value, the retrieved JSValueRef is
> differrent from the set one. A new value is created. This logic allows us
> use JSValueRef as the hashmap key, but I'm not sure if it's a good
> practice...
> 
> >> Source/JavaScriptCore/API/glib/JSCContext.cpp:189
> >> +    JSObjectSetProperty(priv->context.get(), object, strName.get(), jsValue, kJSPropertyAttributeReadOnly, nullptr);
> > 
> > Why kJSPropertyAttributeReadOnly? That means we can call this twice for the same name to update the value.
> 
> No valid reason for this. Already changed to kJSPropertyAttributeNone. :P
> 
> I think that we'll need a way to change that parameter though... Some use
> cases may not want JavaScript to change a value or delete it... I think we
> can add another parameter to jsc_context_set_value or maybe to JSCValue.
> 
> I think that adding this info to JSCValue will be better, because we can
> still use the same jsc_context_set_value to expose callbacks and full
> objects. Depending on the type, we retrieve different properties and use
> them internally.

I'm not sure, this is just a convenient way to expose a value to the global object, if you need to do anything special, I guess you will be able to evaluate (const foo = 'bar') to make it const, no?

> >> Source/JavaScriptCore/API/glib/JSCPrivate.h:22
> >> +#define WEBKIT_DEFINE_SIMPLE_TYPE(TypeName, type_name, TYPE_PARENT) \
> > 
> > Why is this needed? What's different compared to G_DEFINE_TYPE?
> 
> It's just to explicitly remove type_name_init and type_name_class_init from
> cpp files in subtypes and avoid style-checker issues. if that's not a
> problem, I'll switch back to G_DEFINE_TYPE.

It's not a problem.

> >> Source/JavaScriptCore/API/glib/JSCString.cpp:97
> >> +    return g_strdup("the string value");
> > 
> > What? I guess this should be return buff
> 
> Of course it is! I was testing something and forgot about that. Sorry! :P

:-)

> >> Source/JavaScriptCore/API/glib/JSCValue.cpp:79
> >> +        g_object_add_weak_pointer(G_OBJECT(context), (gpointer*)&priv->context);
> > 
> > Use C++ style cast. I'm not sure I understand this weak pointer, since we only null check the context in dispose method. I think the context should never be released if it still contains values, so values should take a reference to the context.
> 
> Hrm... if we do this, we won't be able to simply g_object_unref the context.
> We'll need to add an API to clear the values before unreffing the context,
> so that it will be correctly destroyed. That's why I used a weak ref...

The thing is that the C API works that way in any case, I think. Protected JSValues prevent the context from being destroyed by the garbage collector. So, the values have a strong reference to the context anyway internally.

> And I got an error when using static_cast here...
> 
> >> Source/JavaScriptCore/PlatformGTK.cmake:52
> >> +if (ENABLE_JSC_GLIB)
> > 
> > As I said in my previous review: I don't think we should make this conditional. It doesn't add any new dependency and the api should always be available.
> 
> I'm just afraid of breaking things up :P

For now, it's enough with not installing the headers, the symbols won't be exposed in production builds anyway, until we use JSC_API like macro in the headers.

> >> Tools/TestWebKitAPI/JavaScriptCoreGlib/main.cpp:26
> >> +#include "Tests/JavaScriptCoreGlib/TestJSCUndefined.h"
> > 
> > The main should be in Tests/JavaScriptCoreGlib/ too. Do not add a file for every test case, move them all here to the same file and rename this as TestJSCValue for example
> 
> I can do that. But I followed gtk:
> 
> Tools/TestWebKitAPI/gtk/main.cpp
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/

Not really. In GTK+ we have a "library" with support classes shared by the tests. Those are in Tools/TestWebKitAPI/gtk/WebKit2Gtk/ because they are not tests themselves. Tools/TestWebKitAPI/gtk/main.cpp is the common main used by the google tests based tests. In our case we are not adding tests helper classes, but for now only tests, so they should go to Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/

> And I splited the files for readability... but I'll merge them.

We split "test suites" in files, with all the tests cases of the suite in the same file.

[Estêvão Samuel Procópio Amaral]

Created attachment 295299 [details]
Patch

[Estêvão Samuel Procópio Amaral]

Created attachment 295368 [details]
Patch

[Michael Gratton]

This would be great to have for the WK2 port for Geary, which uses the JSC API both from WebKitJavascriptResult on the app side (and for unit tests) and from a WebKitWebExtension on the WebProcess side, so thanks heaps for working on it!

FWIW, from the app's perspective, the patch takes care of all the actual use cases, so as long as there's also a bridge from WebKitJavascriptResult to this API, it looks like it would be fine as-is.

It falls a little bit short for the web extension however: We are obtaining a JSGlobalContextRef from webkit_frame_get_javascript_global_context(), and are calling JSEvaluateScript on it to execute JS in the web process. This is a hard requirement at the moment.

Some other things that would be nice to have in some ideal future include: JS exception handling, being able to cast arbitrary JSValue objects to strings (e.g. by calling JSValueToStringCopy on them) — very handy for debugging, JSCheckScriptSyntax support for unit testing, and I have been experimenting with JSObjectMakeFunction, JSObjectHasProperty, JSObjectHasProperty and JSObjectGetProperty at various times, also.

[Brady Eidson]

Comment on attachment 295368 [details]
Patch

Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form.

If this patch is still important please rebase it and post it for review again.

[Carlos Garcia Campos]

Created attachment 334449 [details]
WIP patch

I've started to work on this again. This is a WIP patch of what I have so far. I changed my mind with regard to several decisions we made in the past:

 - Use a single JSCValue class: the hierarchy we had didn't make sense in the end. Any value can be converted to other value types, it's perfectly valid to do something like 

value = jsc_value_new_string(context, "25");
g_assert_cmpint(jsc_value_to_int32(value), ==, 25);

And it's also true that an array is also an object so:

value = jsc_value_new_array(context, G_TYPE_NONE);
g_assert(jsc_value_is_array(value));
g_assert(jsc_value_is_object(value));

So, I decided to leave a single JSCValue (only using a dedicated type for exceptions even when they are values too, just for convenience) with is() and to() methods. The API that is expected to be called on a specific value type uses the pattern jsc_value_foo_do_whatever, for example jsc_value_object_set_property().

 - No floating references. For now JSCValue is a normal GObject and we return transfer full and never adopt parameters. We can change this in the future to adopt in some cases to be able to do things like:

jsc_value_object_set_property(object, "foo", jsc_value_new_number(context, 25));

This is indeed convenient, but makes other cases inconvenient, so we'll see.

 - Values are cached by the context, but not owned, they are removed from the cache when destroyed.

These are the main differences. I've added API to support most of the types and convenient methods to handle them. There's also API to evaluate scripts, get/set values to global object and to create new classes implemented by a C object/struct. There are still a lot of things to do:

 - Documentation.
 - Introspection friendly, I'm using varargs in several places without the alternative for bindings.
 - Make it installable.
 - Add support for == and ===
 - Add support for dates
 - Add support for more glib types (strv, GHashTable, etc.).
 - Add convenient API on top of current JSCClass to easily export GObjects.
 - And more things I don't remember now.

Then, on the WebKit side:

 - We need to deprecate all WebKit API using the JSC C API types and provide alternatives using the new glib API
 - We could also deprecate the DOM bindings and add a bridge API between DOM and JSC.

[Carlos Garcia Campos]

Created attachment 335108 [details]
WIP patch

[Carlos Garcia Campos]

Created attachment 335292 [details]
WIP patch

[Carlos Garcia Campos]

Created attachment 335929 [details]
WIP patch

[Michael Catanzaro]

Comment on attachment 335929 [details]
WIP patch

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

Impressive. I skimmed over about half of it... it's a lot.

I'd try asking Yusuke to review the API... it seems good to me, though. Except you're missing autoptr support, which is important. So please add that.

Once you add doc comments, we'll need to carefully test introspection to e.g. ensure it's possible to create and use a JSCClass from... say, JavaScript. I think it should be fine, but needs a little demo proof of concept to be sure. Of course we don't have to commit that.

Also, perhaps we should not allow subclassing the public API GObjects? E.g. JSCClass. Subclassing that seems weird. Just because the rest of our API allows subclassing all the public types doesn't mean it's necessarily a good idea to follow that practice here... I can recommend some best-practice changes if you decide to make the public GObjects final instead of derivable.

> Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:50
> +    JSC::JSAPIWrapperObject* wrapperObject = static_cast<JSC::JSAPIWrapperObject*>(handle.get().asCell());

I prefer to use auto* when doing a cast, because the type is written clearly on the rhs anyway.

> Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:84
> +        *exception = toRef(JSC::createTypeError(toJS(jsContext), ASCIILiteral("cannot call a class constructor without |new|")));

These errors should be translatable, right? Even normal GError messages we normally mark for translation.

> Source/JavaScriptCore/API/glib/JSCClass.h:22
> +#if !defined(__JSC_H_INSIDE__) && !defined(JSC_COMPILATION)
> +#error "Only <jsc/jsc.h> can be included directly."
> +#endif

I think we should move this underneath the include guards. It's a microoptimization, but it annoys me. We can change the existing public APIs later.

> Source/JavaScriptCore/API/glib/JSCContext.h:47
> +typedef void (* JSCExceptionHandler) (JSCContext   *context,

JSCExceptionHandlerCallback?

> Source/JavaScriptCore/API/glib/JSCDefines.h:43
> +#ifdef G_OS_WIN32
> +#    if defined(BUILDING_JavaScriptCore) || defined(STATICALLY_LINKED_WITH_JavaScriptCore)
> +#        define JSC_API __declspec(dllexport)
> +#    else
> +#        define JSC_API __declspec(dllimport)
> +#    endif
> +#else
> +#    define JSC_API __attribute__((visibility("default")))
> +#endif

Seems like the G_OS_WIN32 condition creates the false hope that this code might be expected to work on Windows.

I do think we should go ahead and define and use JSC_API, but we both know it's a no-op because we default to the default visibility anyway... real shame that I failed to fix that. Which reminds me: this will be broken for WPE, because the API symbols will all get dropped by WPE's linker version script. So that script will need to be updated.

> Source/JavaScriptCore/PlatformGTK.cmake:91
> +add_custom_command(

Use VERBATIM

> Source/JavaScriptCore/heap/Heap.h:280
> +    void releaseSoon(std::unique_ptr<JSCGLibWrapperObject>&&);

std::unique_ptr<JSCGLibWrapperObject>&& is a weird API, because it implies that the object was allocated using system malloc, rather than g_malloc() or bmalloc. Doesn't seem right. Can you explain?

> Source/JavaScriptCore/heap/HeapInlines.h:171
> +inline void Heap::releaseSoon(std::unique_ptr<JSCGLibWrapperObject>&& object)

Here too.

> Tools/TestWebKitAPI/PlatformGTK.cmake:141
> +    WEBKIT_ADD_TARGET_CXX_FLAGS(TestJSC -Wno-sign-compare
> +                                        -Wno-undef
> +                                        -Wno-unused-parameter)

Please don't cargo cult this; instead, ensure your new tests don't trigger these warnings... I guess -Wno-unused-parameter is OK if you prefer that, because that warning is controversial in WebKit, but your code should really not be triggering -Wsign-compare and especially not -Wundef. I think we have these suppressed here because the gtest headers trigger them.

> Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:47
> +        g_assert(m_watchedObjects.isEmpty());

Very minor: since these are new tests, I suggest we start using GLib's testing assertions, rather than normal g_assert(). This would become g_assert_true(). See https://bugzilla.gnome.org/show_bug.cgi?id=793856 for rationale.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #21)
> Comment on attachment 335929 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335929&action=review
> 
> Impressive. I skimmed over about half of it... it's a lot.
> 
> I'd try asking Yusuke to review the API... it seems good to me, though.
> Except you're missing autoptr support, which is important. So please add
> that.

I'm missing a lot of things, this is a work in progress patch. It lacks documentation, gobject introspection and even functionality like jsc_value_equal.

> Once you add doc comments, we'll need to carefully test introspection to
> e.g. ensure it's possible to create and use a JSCClass from... say,
> JavaScript. I think it should be fine, but needs a little demo proof of
> concept to be sure. Of course we don't have to commit that.

Yes, the patch also needs a introspection compatible function for several methods that use varargs, for example. I started to work on this right after branching to have the whole release cycle to properly test this API.

> Also, perhaps we should not allow subclassing the public API GObjects? E.g.
> JSCClass. Subclassing that seems weird. Just because the rest of our API
> allows subclassing all the public types doesn't mean it's necessarily a good
> idea to follow that practice here... I can recommend some best-practice
> changes if you decide to make the public GObjects final instead of derivable.

That's a good point, specially for classes where there isn't new methods, it's not possible to subclass, because new instances are always created internally.

> > Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:50
> > +    JSC::JSAPIWrapperObject* wrapperObject = static_cast<JSC::JSAPIWrapperObject*>(handle.get().asCell());
> 
> I prefer to use auto* when doing a cast, because the type is written clearly
> on the rhs anyway.

Yes, me too.

> > Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:84
> > +        *exception = toRef(JSC::createTypeError(toJS(jsContext), ASCIILiteral("cannot call a class constructor without |new|")));
> 
> These errors should be translatable, right? Even normal GError messages we
> normally mark for translation.

I don't think javascript exceptions are translated at all. I don't think a message like "cannot call a class constructor without |new|" is useful for the end users either, it's the developer who made a mistake, and they surely understands the message. I think we can avoid translations in jsc.

> > Source/JavaScriptCore/API/glib/JSCClass.h:22
> > +#if !defined(__JSC_H_INSIDE__) && !defined(JSC_COMPILATION)
> > +#error "Only <jsc/jsc.h> can be included directly."
> > +#endif
> 
> I think we should move this underneath the include guards. It's a
> microoptimization, but it annoys me. We can change the existing public APIs
> later.

This is what we do everywhere else, I think. This won't be included twice in any case, since it will fail, no?

> > Source/JavaScriptCore/API/glib/JSCContext.h:47
> > +typedef void (* JSCExceptionHandler) (JSCContext   *context,
> 
> JSCExceptionHandlerCallback?

Isn't Handler and Callback kind of redundant?

> > Source/JavaScriptCore/API/glib/JSCDefines.h:43
> > +#ifdef G_OS_WIN32
> > +#    if defined(BUILDING_JavaScriptCore) || defined(STATICALLY_LINKED_WITH_JavaScriptCore)
> > +#        define JSC_API __declspec(dllexport)
> > +#    else
> > +#        define JSC_API __declspec(dllimport)
> > +#    endif
> > +#else
> > +#    define JSC_API __attribute__((visibility("default")))
> > +#endif
> 
> Seems like the G_OS_WIN32 condition creates the false hope that this code
> might be expected to work on Windows.

I don't see why not. Doesn't jsc compile in windows?

> I do think we should go ahead and define and use JSC_API, but we both know
> it's a no-op because we default to the default visibility anyway... real
> shame that I failed to fix that. Which reminds me: this will be broken for
> WPE, because the API symbols will all get dropped by WPE's linker version
> script. So that script will need to be updated.

I think it's good to have those just in case we are able to fix it at some point, but yes, I agree.

> > Source/JavaScriptCore/PlatformGTK.cmake:91
> > +add_custom_command(
> 
> Use VERBATIM

Ok.

> > Source/JavaScriptCore/heap/Heap.h:280
> > +    void releaseSoon(std::unique_ptr<JSCGLibWrapperObject>&&);
> 
> std::unique_ptr<JSCGLibWrapperObject>&& is a weird API, because it implies
> that the object was allocated using system malloc, rather than g_malloc() or
> bmalloc. Doesn't seem right. Can you explain?

I don't understand what you mean, std::unique_ptr will use delete, the object was allocated with new. It has nothing to do with system malloc vs bmalloc. Actually, JSCGLibWrapperObject is fast allocated so it will always be using bmalloc unless disabled.

> > Source/JavaScriptCore/heap/HeapInlines.h:171
> > +inline void Heap::releaseSoon(std::unique_ptr<JSCGLibWrapperObject>&& object)
> 
> Here too.
> 
> > Tools/TestWebKitAPI/PlatformGTK.cmake:141
> > +    WEBKIT_ADD_TARGET_CXX_FLAGS(TestJSC -Wno-sign-compare
> > +                                        -Wno-undef
> > +                                        -Wno-unused-parameter)
> 
> Please don't cargo cult this; instead, ensure your new tests don't trigger
> these warnings... I guess -Wno-unused-parameter is OK if you prefer that,
> because that warning is controversial in WebKit, but your code should really
> not be triggering -Wsign-compare and especially not -Wundef. I think we have
> these suppressed here because the gtest headers trigger them.

It's not my code what causes tons of warnings, it's the gtest third party code.

> > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:47
> > +        g_assert(m_watchedObjects.isEmpty());
> 
> Very minor: since these are new tests, I suggest we start using GLib's
> testing assertions, rather than normal g_assert(). This would become
> g_assert_true(). See https://bugzilla.gnome.org/show_bug.cgi?id=793856 for
> rationale.

Sure.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #22)
> > > Source/JavaScriptCore/API/glib/JSCClass.h:22
> > > +#if !defined(__JSC_H_INSIDE__) && !defined(JSC_COMPILATION)
> > > +#error "Only <jsc/jsc.h> can be included directly."
> > > +#endif
> > 
> > I think we should move this underneath the include guards. It's a
> > microoptimization, but it annoys me. We can change the existing public APIs
> > later.
> 
> This is what we do everywhere else, I think. This won't be included twice in
> any case, since it will fail, no?

It's a very very small microoptimization: if the file starts with the include guards, GCC will avoid opening the file repeatedly. We should change our existing headers. Yes, there's no behavioral difference.

> > > Source/JavaScriptCore/API/glib/JSCContext.h:47
> > > +typedef void (* JSCExceptionHandler) (JSCContext   *context,
> > 
> > JSCExceptionHandlerCallback?
> 
> Isn't Handler and Callback kind of redundant?

Kind of. This one is iffy IMO.

> > > Source/JavaScriptCore/API/glib/JSCDefines.h:43
> > > +#ifdef G_OS_WIN32
> > > +#    if defined(BUILDING_JavaScriptCore) || defined(STATICALLY_LINKED_WITH_JavaScriptCore)
> > > +#        define JSC_API __declspec(dllexport)
> > > +#    else
> > > +#        define JSC_API __declspec(dllimport)
> > > +#    endif
> > > +#else
> > > +#    define JSC_API __attribute__((visibility("default")))
> > > +#endif
> > 
> > Seems like the G_OS_WIN32 condition creates the false hope that this code
> > might be expected to work on Windows.
> 
> I don't see why not. Doesn't jsc compile in windows?

I guess it could be useful there... OK.

[Yusuke Suzuki]

Comment on attachment 335929 [details]
WIP patch

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

Early review. I have one question about the design of this patch: why not just accepting GObject for a wrapped object?

> Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:54
> +    JSC::Heap::heap(wrapperObject)->releaseSoon(std::unique_ptr<JSC::JSCGLibWrapperObject>(static_cast<JSC::JSCGLibWrapperObject*>(wrapperObject->wrappedObject())));

If a wrapped object is GObject, we can just do adoptGRef here, which is well-aligned to the objective C implementation.

> Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:94
> +    m_wrappedObject = wrappedObject;

And do g_object_ref here, which is aligned to the Objective C implementation.

> Source/JavaScriptCore/API/glib/JSCContext.h:124
> +                                      GDestroyNotify      destroy_function);

What is the purpose of destroy_function? Why not just calling g_object_unref when the wrapper is dead?
I think the current objective-c implementation does this.

> Source/JavaScriptCore/API/glib/JSCGLibWrapperObject.h:47
> +class JSCGLibWrapperObject {
> +    WTF_MAKE_FAST_ALLOCATED;
> +public:
> +    JSCGLibWrapperObject(gpointer object, GDestroyNotify destroyFunction)
> +        : m_object(object)
> +        , m_destroyFunction(destroyFunction)
> +    {
> +    }
> +
> +    ~JSCGLibWrapperObject()
> +    {
> +        if (m_destroyFunction)
> +            m_destroyFunction(m_object);
> +    }
> +
> +    gpointer object() const { return m_object; }
> +
> +private:
> +    gpointer m_object { nullptr };
> +    GDestroyNotify m_destroyFunction { nullptr };
> +};

I think just wrapping gobject instead of having this class is cleaner and well-aligned to objective C implementation.

[Carlos Garcia Campos]

(In reply to Yusuke Suzuki from comment #24)
> Comment on attachment 335929 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335929&action=review
> 
> Early review. I have one question about the design of this patch: why not
> just accepting GObject for a wrapped object?

We don't want to limit this to GObjects, we want to allow using a boxed type for example, or simply any C struct. On top of this patch, I plan to add convenience API specific for GType system, making it easier to expose GObjects directly, automatically handling things like the prototype hierarchy.

> > Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:54
> > +    JSC::Heap::heap(wrapperObject)->releaseSoon(std::unique_ptr<JSC::JSCGLibWrapperObject>(static_cast<JSC::JSCGLibWrapperObject*>(wrapperObject->wrappedObject())));
> 
> If a wrapped object is GObject, we can just do adoptGRef here, which is
> well-aligned to the objective C implementation.

In Objective C everything is an Object, that's no the case of GLib world.

> > Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:94
> > +    m_wrappedObject = wrappedObject;
> 
> And do g_object_ref here, which is aligned to the Objective C implementation.
> 
> > Source/JavaScriptCore/API/glib/JSCContext.h:124
> > +                                      GDestroyNotify      destroy_function);
> 
> What is the purpose of destroy_function? Why not just calling g_object_unref
> when the wrapper is dead?
> I think the current objective-c implementation does this.

In case of using a boxed type or any other custom struct here we would pass my_boxed_type_unref or my_custom_struct_free. See the example in the unit tests.

> > Source/JavaScriptCore/API/glib/JSCGLibWrapperObject.h:47
> > +class JSCGLibWrapperObject {
> > +    WTF_MAKE_FAST_ALLOCATED;
> > +public:
> > +    JSCGLibWrapperObject(gpointer object, GDestroyNotify destroyFunction)
> > +        : m_object(object)
> > +        , m_destroyFunction(destroyFunction)
> > +    {
> > +    }
> > +
> > +    ~JSCGLibWrapperObject()
> > +    {
> > +        if (m_destroyFunction)
> > +            m_destroyFunction(m_object);
> > +    }
> > +
> > +    gpointer object() const { return m_object; }
> > +
> > +private:
> > +    gpointer m_object { nullptr };
> > +    GDestroyNotify m_destroyFunction { nullptr };
> > +};
> 
> I think just wrapping gobject instead of having this class is cleaner and
> well-aligned to objective C implementation.

I agree it would be easier and cleaner, but would limit the API.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #23)
> (In reply to Carlos Garcia Campos from comment #22)
> > > > Source/JavaScriptCore/API/glib/JSCClass.h:22
> > > > +#if !defined(__JSC_H_INSIDE__) && !defined(JSC_COMPILATION)
> > > > +#error "Only <jsc/jsc.h> can be included directly."
> > > > +#endif
> > > 
> > > I think we should move this underneath the include guards. It's a
> > > microoptimization, but it annoys me. We can change the existing public APIs
> > > later.
> > 
> > This is what we do everywhere else, I think. This won't be included twice in
> > any case, since it will fail, no?
> 
> It's a very very small microoptimization: if the file starts with the
> include guards, GCC will avoid opening the file repeatedly. We should change
> our existing headers. Yes, there's no behavioral difference.

But that would allow to do something like:

#include <jsc/jsc.h>
#include <jsc/JSCFoo.h>

We never want to allow including headers other than jsc.h even jsc.h has already been included.

[Carlos Garcia Campos]

Created attachment 336112 [details]
Patch

Addressed review comments, added documentation, gobject-introspection and auto cleanups. I think this is enough for an initial patch.

[EWS Watchlist]

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


WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/jsc.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCValue.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCWrapperMap.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCDefines.h"
ERROR: Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:911:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCContext.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCGLibWrapperObject.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCVirtualMachine.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCClass.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCException.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCAutocleanups.h"
Total errors found: 1 in 52 files


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

[Carlos Garcia Campos]

Created attachment 336113 [details]
Patch

[EWS Watchlist]

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


WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/jsc.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCValue.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCWrapperMap.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCDefines.h"
ERROR: Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:911:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCContext.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCGLibWrapperObject.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCVirtualMachine.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCClass.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCException.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCAutocleanups.h"
Total errors found: 1 in 52 files


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

[Carlos Garcia Campos]

Btw, I added gobject introspection support, but I know there are still several functions that are not introspectable. I'll mark those as skip and add istrospectable versions in a follow up patch.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #26)
> But that would allow to do something like:
> 
> #include <jsc/jsc.h>
> #include <jsc/JSCFoo.h>
> 
> We never want to allow including headers other than jsc.h even jsc.h has
> already been included.

Good point. I never considered that.

[Carlos Garcia Campos]

Committed r229798: <https://trac.webkit.org/changeset/229798>