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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Nov 12 04:23:52 PST 2016


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

--- Comment #11 from Estêvão Samuel Procópio Amaral <tevaum at gmail.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/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.

-- 
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/20161112/84842f92/attachment-0001.html>


More information about the webkit-unassigned mailing list