[Webkit-unassigned] [Bug 164061] [GTK] Initial implementation of JavaScriptCore glib bindings

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 28 01:08:32 PDT 2016 

https://bugs.webkit.org/show_bug.cgi?id=164061

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #293075|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 293075
  --> https://bugs.webkit.org/attachment.cgi?id=293075
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161028/e88bd414/attachment-0001.html>

More information about the webkit-unassigned mailing list