<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Initial implementation of JavaScriptCore glib bindings"
href="https://bugs.webkit.org/show_bug.cgi?id=164061">bug 164061</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #293075 Flags</td>
<td>review?, commit-queue?
</td>
<td>review-, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Initial implementation of JavaScriptCore glib bindings"
href="https://bugs.webkit.org/show_bug.cgi?id=164061#c7">Comment # 7</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Initial implementation of JavaScriptCore glib bindings"
href="https://bugs.webkit.org/show_bug.cgi?id=164061">bug 164061</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=293075&action=diff" name="attach_293075" title="Patch">attachment 293075</a> <a href="attachment.cgi?id=293075&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=293075&action=review">https://bugs.webkit.org/attachment.cgi?id=293075&action=review</a>
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.
<span class="quote">> 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.</span >
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
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:25
> +</span >
Remove this empty line, keep all includes in a single block.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:40
> +jsc_boolean_class_init(JSCBooleanClass *)</span >
JSCBooleanClass * -> JSCBooleanClass*
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:45
> +jsc_boolean_init(JSCBoolean *)</span >
JSCBoolean * -> JSCBoolean*
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:49
> +JSCBoolean *</span >
Ditto, and the same everywhere, I wonder why style checker didn't complain about this.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:50
> +jscBooleanNewFromJSValue(JSCContext *context, JSValueRef jsvalue)</span >
For internal new methods we normally use Create, so this could be just jscBooleanCreate()
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:54
> + JSCBoolean* boolean;
> +
> + boolean = (JSCBoolean *) g_object_new(JSC_TYPE_BOOLEAN, "context", context, NULL);</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:70
> +jsc_boolean_new(JSCContext *context, gboolean value)</span >
All jsc value new methods should return a JSCValue.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:72
> + JSContextRef jscontext;</span >
jscontext -> jsContext
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:73
> + JSValueRef jsvalue;</span >
jsvalue -> jsValue
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:76
> + jscontext = jscContextGetJSContext(context);
> + jsvalue = JSValueMakeBoolean(jscontext, value);</span >
Use the same line for the variable delcarations
JSContextRef jsContext = jscContextGetJSContext(context);
JSValueRef jsValuie = JSValueMakeBoolean(jsContext, value);
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:78
> + return jscBooleanNewFromJSValue(context, jsvalue);</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:29
> +</span >
Remove this empty line.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:38
> + * A #JSCContext is the central class of the JavaScriptCore API. It is</span >
Do not reference JSCContext here because it points to this block
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:46
> + JSGlobalContextRef context;</span >
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>
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:47
> + JSObjectRef object;</span >
What is this object? the global object? Call it globalObject if that is the case.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:49
> + GList* values;</span >
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>>
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:55
> +static void
> +jscContextDispose(GObject* obj)</span >
Forgot to say before but this should be a single line, and the same for all other functions.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:59
> + JSCContextPrivate* priv;
> +
> + priv = (JSCContextPrivate*) jsc_context_get_instance_private(JSC_CONTEXT(obj));</span >
Since line and don't use C style casts.
<span class="quote">> 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;
> + }</span >
And you don't need this with smart pointers.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:88
> + priv->object = JSContextGetGlobalObject(priv->context);</span >
Why do we need to keep this? Could we get it on demand when needed?
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:89
> + priv->values = nullptr;</span >
This is already nullptr, the private struct is 0 filles on allocation by glib.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:90
> +}</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:110
> +jsc_context_new(void)</span >
(void) -> ()
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:132
> + if (JSValueIsUndefined(priv->context, jsvalue) || JSValueIsNull(priv->context, jsvalue))
> + return nullptr;</span >
We need to handle this, null is not the same as undefined
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:134
> + g_assert_not_reached();</span >
Use wk asserts, ASSERT_NOT_REACHED();
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:145
> + * Returns: A JSCValue subtype representing the value of the requested property</span >
JSCValue -> #JSCValue I would remove the "subtype", JSCValue is an abstract class.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:159
> + JSStringRelease(strName);</span >
You can use JSRetainPtr with JSStringRef too.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:178
> +</span >
Forgot to say before, but you should use g_return macros in all public methods to check all passed in arguments.
<span class="quote">> 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);
> + }</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:189
> + JSObjectSetProperty(priv->context, priv->object, strName, jsvalue,
> + kJSPropertyAttributeReadOnly, nullptr);</span >
This could be a single line, we use 120 as a limit.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:190
> + JSStringRelease(strName);</span >
Use JSRetainPtr
<span class="quote">> Source/JavaScriptCore/API/glib/JSCNumber.cpp:61
> +jsc_number_new(JSCContext* context, gdouble value)</span >
new_double
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:44
> + JSCContext* jscContext;
> + JSGlobalContextRef context;</span >
Why do we need to keep both? we can get the JS one from the JSC one
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:48
> +G_DEFINE_TYPE_WITH_PRIVATE(JSCValue, jsc_value, G_TYPE_INITIALLY_UNOWNED)</span >
This should be abstract
<span class="quote">> 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;</span >
We should not allow to create a JSCValue without a JSCContext
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:98
> + priv->context = nullptr;
> + priv->value = nullptr;</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:126
> + (GParamFlags) (G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));</span >
use static_cast here.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:150
> + if (priv->value != nullptr) {</span >
Don't compare to nullptr, do if (priv->value)
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:151
> + JSValueUnprotect(priv->context, priv->value);</span >
How can this happen? Aren't wrappers inmutable?
<span class="quote">> 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)</span >
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
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:197
> + return g_strdup(buff);</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:206
> + * Returns: gboolean</span >
You could say at least A #gboolean instead of just gboolean
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:209
> +gboolean
> +jsc_value_get_boolean(JSCValue *value)</span >
Same here, this should be in JSCBoolean, jsc_boolean_get_boolean or get_value to avoid the bool_get_bool
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:230
> +jsc_value_get_double(JSCValue *value)</span >
jsc_number_get_double
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:250
> +gint</span >
gint -> gint32
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:260
> + return JSValueToNumber(priv->context, priv->value, nullptr);</span >
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
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:271
> +guint</span >
guint32
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:281
> + return (guint) JSValueToNumber(priv->context, priv->value, nullptr);</span >
Same comments here.
<span class="quote">> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:20
> +#pragma once</span >
We can't use #pragma once in public headers, use the #ifndef approach.
<span class="quote">> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:23
> +#include "JSCContext.h"
> +#include "JSCValue.h"</span >
Use always angle-bracket includes in public headers
<span class="quote">> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:25
> +#include <JavaScriptCore/JSBase.h></span >
This is exposing the JSC C API, don't do this.
<span class="quote">> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:48
> +JS_EXPORT GType
> +jsc_boolean_get_type (void);</span >
This should be a single line.
<span class="quote">> Source/JavaScriptCore/API/glib/jsc/JSCBoolean.h:51
> +JS_EXPORT JSCBoolean*
> +jsc_boolean_new (JSCContext*, gboolean value);</span >
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)
<span class="quote">> Source/JavaScriptCore/API/glib/jsc/JSCBooleanPrivate.h:25
> +JS_EXPORT JSCBoolean*
> +jscBooleanNewFromJSValue(JSCContext*, JSValueRef);</span >
Single line.
<span class="quote">> Source/JavaScriptCore/API/glib/jsc/JSCValue.h:66
> +/* JS_EXPORT JSCContext * */
> +/* jsc_value_get_context (JSCValue *value); */</span >
Do not include commented code like this.
<span class="quote">> Source/JavaScriptCore/PlatformGTK.cmake:52
> +if (ENABLE_JSC_GLIB)</span >
I don't think we should make this conditional. It doesn't add any new dependency and the api should always be available.
<span class="quote">> Source/JavaScriptCore/PlatformGTK.cmake:82
> + install(FILES
> + ${jscglib_HEADERS}
> + DESTINATION "${WEBKITGTK_HEADER_INSTALL_DIR}/jsc"</span >
Do not add any install command for now.
<span class="quote">> Tools/ChangeLog:18
> + * TestWebKitAPI/jsc-glib/JSCValuesTest.cpp: Added.
> + * TestWebKitAPI/jsc-glib/JSCValuesTest.h: Added.
> + * TestWebKitAPI/jsc-glib/main.cpp: Added.</span >
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
<span class="quote">> Tools/TestWebKitAPI/jsc-glib/JSCValuesTest.cpp:21
> +#include "JSCValuesTest.h"</span >
I would call it JSCValueTest not Values
<span class="quote">> Tools/TestWebKitAPI/jsc-glib/JSCValuesTest.cpp:26
> +ContextFixtureSetUp(ContextFixture *fixture, gconstpointer userData)</span >
ContextFixtureSetUp -> contextFixtureSetUp
<span class="quote">> 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)));</span >
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.
<span class="quote">> 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");</span >
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.
<span class="quote">> Tools/TestWebKitAPI/jsc-glib/main.cpp:31
> + g_test_add("/jsc-values/string-test", ContextFixture, NULL,</span >
Maybe we can start with g_test_add_func to make it simpler, and every test will create and destroy its context.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>