[Webkit-unassigned] [Bug 164061] [GTK] Initial implementation of JavaScriptCore glib bindings
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Nov 11 03:00:33 PST 2016
https://bugs.webkit.org/show_bug.cgi?id=164061
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #294417|review?, commit-queue? |review-, commit-queue-
Flags| |
--- Comment #10 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 294417
--> https://bugs.webkit.org/attachment.cgi?id=294417
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
--
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/20161111/e9fa462c/attachment-0001.html>
More information about the webkit-unassigned
mailing list