<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 #294417 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#c10">Comment # 10</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=294417&action=diff" name="attach_294417" title="Patch">attachment 294417</a> <a href="attachment.cgi?id=294417&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=294417&action=review">https://bugs.webkit.org/attachment.cgi?id=294417&action=review</a>
<span class="quote">> Source/JavaScriptCore/ChangeLog:9
> + according to the discussion on webklit-gtk mailing list thread</span >
webklit-gtk -> webkit-gtk
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:46
> +JSCBoolean* jscBooleanCreate(JSCContext* context, JSValueRef jsValue)</span >
I think it's easier if this also returns a JSCValue*
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:51
> + JSCBoolean* boolean = JSC_BOOLEAN(g_object_new(JSC_TYPE_BOOLEAN, "context", context, NULL));</span >
NULL -> nullptr
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:52
> + jscValueSetJSValue(JSC_VALUE(boolean), jsValue);</span >
If this returns a JSCValue, you only need to cast the g_object_new(), and here we don't need to cast again.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:68
> + g_return_val_if_fail(JSC_IS_CONTEXT(context), false);</span >
false -> nullptr
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:73
> + JSContextRef jsContext = jscContextGetJSContext(context);
> + JSValueRef jsValue = JSValueMakeBoolean(jsContext, value);
> +
> + return JSC_VALUE(jscBooleanCreate(context, jsValue));</span >
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)));
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:86
> + g_return_val_if_fail(JSC_IS_BOOLEAN(boolean), false);</span >
false -> FALSE
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:89
> + JSCContext* context;
> + g_object_get(G_OBJECT(context), "context", &context, nullptr);</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.cpp:95
> + return JSValueToBoolean(jsContext, jsValue);</span >
return JSValueToBoolean(jscContextGetJSContext(context), jscValueGetJSValue(JSC_VALUE(boolean));
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.h:43
> + JSCValueClass parentClass;</span >
parent_class
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.h:48
> +JSCValue* jsc_boolean_new (JSCContext* context,</span >
JSCContext* context -> JSCContext *context
In public headers we follow the GNOME style.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBoolean.h:51
> +gboolean jsc_boolean_get_value (JSCBoolean* value);</span >
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
<span class="quote">> Source/JavaScriptCore/API/glib/JSCBooleanPrivate.h:25
> +</span >
Remove this extra line
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:49
> +typedef HashMap<const gchar*, GRefPtr<JSCValue> > ValueMap;</span >
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 > >
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:55
> + _JSCContextPrivate()
> + {
> + context = JSRetainPtr<JSGlobalContextRef>(Adopt, JSGlobalContextCreate(nullptr));
> + }</span >
I think we can add a GObjectClass::constructed implementation and do all the initializations there.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:60
> + ~_JSCContextPrivate()
> + {
> + context.clear();
> + }</span >
This is done automatically, you don't need to explicitly clear the smart pointer.
<span class="quote">> 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();
> +}</span >
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().
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:89
> + JSCContextPrivate* priv = context->priv;
> +
> + return priv->context.get();</span >
return context->priv->context.get();
<span class="quote">> 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));</span >
You don't need all these JSC_VALUE casts if Create functions return a JSCValue
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:125
> + if (JSValueIsNull(priv->context.get(), jsValue))
> + return nullptr;</span >
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
<span class="quote">> 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);
> + }
> + }</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.cpp:189
> + JSObjectSetProperty(priv->context.get(), object, strName.get(), jsValue, kJSPropertyAttributeReadOnly, nullptr);</span >
Why kJSPropertyAttributeReadOnly? That means we can call this twice for the same name to update the value.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.h:51
> +JSCContext*</span >
JSCContext* -> JSCContext *
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.h:54
> +JSCValue*</span >
JSCValue* -> JSCValue *
<span class="quote">> Source/JavaScriptCore/API/glib/JSCContext.h:56
> +jsc_context_get_value (JSCContext* context,
> + const gchar* name);</span >
Same here about the *
<span class="quote">> Source/JavaScriptCore/API/glib/JSCNumber.cpp:58
> +JSCValue* jsc_number_new_double(JSCContext* context, gdouble value)</span >
This needs documentation
<span class="quote">> Source/JavaScriptCore/API/glib/JSCNumber.cpp:68
> +JSCValue* jsc_number_new_int32(JSCContext* context, gint32 value)</span >
Ditto.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCNumber.cpp:78
> +JSCValue* jsc_number_new_uint32(JSCContext* context, guint32 value)</span >
Ditto.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCNumber.cpp:110
> +gdouble jsc_number_get_double(JSCNumber* number)</span >
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
<span class="quote">> Source/JavaScriptCore/API/glib/JSCNumber.cpp:125
> +guint jsc_number_get_uint32(JSCNumber* number)</span >
guint -> guint32
<span class="quote">> Source/JavaScriptCore/API/glib/JSCNumber.cpp:140
> +gint jsc_number_get_int32(JSCNumber* number)</span >
gint -> gint32
<span class="quote">> Source/JavaScriptCore/API/glib/JSCNumber.h:62
> +jsc_number_get_double (JSCNumber*);</span >
Do not omit parameter names in public headers
<span class="quote">> Source/JavaScriptCore/API/glib/JSCPrivate.h:22
> +#define WEBKIT_DEFINE_SIMPLE_TYPE(TypeName, type_name, TYPE_PARENT) \</span >
Why is this needed? What's different compared to G_DEFINE_TYPE?
<span class="quote">> Source/JavaScriptCore/API/glib/JSCString.cpp:39
> +WEBKIT_DEFINE_SIMPLE_TYPE(JSCString, jsc_string, JSC_TYPE_VALUE)</span >
Can't we use G_DEFINE_TYPE here?
<span class="quote">> Source/JavaScriptCore/API/glib/JSCString.cpp:97
> + return g_strdup("the string value");</span >
What? I guess this should be return buff
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:53
> + kPROP_0,</span >
Do not use k prefix for this
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:58
> +static void jscValueGetProperty(GObject* obj, guint prop_id, GValue* value, GParamSpec* param_spec)</span >
obj ->object, prop_id -> propID, param_spec -> paramSpec
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:64
> + g_value_set_object(value, priv->context);</span >
g_value_set_object(value, JSC_VALUE(obj)->priv->context);
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:71
> +static void jscValueSetProperty(GObject* obj, guint prop_id, const GValue* value, GParamSpec* param_spec)</span >
obj ->object, prop_id -> propID, param_spec -> paramSpec
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:79
> + g_object_add_weak_pointer(G_OBJECT(context), (gpointer*)&priv->context);</span >
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.
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:123
> + (GParamFlags) (G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));</span >
static_cast<GParamFlags>()
<span class="quote">> Source/JavaScriptCore/API/glib/JSCValue.cpp:132
> + JSCValuePrivate* priv = value->priv;
> +
> + return priv->value;</span >
return value->priv->value;
<span class="quote">> Source/JavaScriptCore/API/glib/jsc.h:28
> +</span >
JSCUndefined is missing here.
<span class="quote">> Source/JavaScriptCore/PlatformGTK.cmake:52
> +if (ENABLE_JSC_GLIB)</span >
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.
<span class="quote">> Tools/ChangeLog:17
> + * TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCBoolean.cpp: Added.</span >
Since we already have a TestWebKitAPI/Tests/JavaScriptCore dir that is currently empty, I would add a glib subdir there for our tests.
<span class="quote">> 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"</span >
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
<span class="quote">> Tools/TestWebKitAPI/JavaScriptCoreGlib/main.cpp:34
> + g_test_add_func("/JavaScriptCoreGlib/JSCContext", testJSCContext);</span >
And for tests cases I would use /jsc/jsc-value/foo pattern
<span class="quote">> Tools/TestWebKitAPI/PlatformGTK.cmake:165
> + add_executable(TestJavaScriptCoreGlib</span >
This is created in bin directlry, it should be in ${TESTWEBKITAPI_RUNTIME_OUTPUT_DIRECTORY}/JavaScriptCore/glib, I think you need to use add_test()
<span class="quote">> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCBoolean.cpp:36
> + getValue = jsc_context_get_value(context, "true");</span >
JSCValue* getValue = jsc_context_get_value(context, "true");
<span class="quote">> 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);</span >
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.
<span class="quote">> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCNumber.cpp:83
> + g_assert_cmpfloat(jsc_number_get_uint32(JSC_NUMBER(n)), ==, 4294967295);</span >
g_assert_cmpuint
<span class="quote">> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCNumber.cpp:116
> + g_assert_cmpfloat(jsc_number_get_int32(JSC_NUMBER(n)), ==, -2147483648);</span >
g_assert_cmpint
<span class="quote">> Tools/TestWebKitAPI/Tests/JavaScriptCoreGlib/TestJSCUndefined.cpp:40
> + g_assert(JSC_IS_VALUE(nonExistant));
> + g_assert(JSC_IS_UNDEFINED(nonExistant));</span >
This is redundant, if it's UNDEFINED, it's VALUE</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>