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

[bugzilla-daemon at webkit.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cgarcia at igalia.com

--- Comment #12 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #11)
> Comment on attachment 294417 [details]
> 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...

I'm not sure, TBH. Maybe at some point we need to distinguish between a function returning nullptr to express an error and returning nullptr as a the JavaScript null value. So, I don't know, maybe we can start using nullptr and add the object later of needed to make things easier.

> >> 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*) ;)

Don't worry too much about leaking in the tests for now, we will fix everything eventually once we have the liefcycle management implemented.

> 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...

Not easy for me to give an opinion about this, because I don't fully understand how to the wrapped api objects and internal objects work in JavaScriptCore. We need to understand how things work in JSC to make a decision.

> 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 think so.

> 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.

I'm not sure, this is just a convenient way to expose a value to the global object, if you need to do anything special, I guess you will be able to evaluate (const foo = 'bar') to make it const, no?

> >> 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.

It's not a problem.

> >> 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...

The thing is that the C API works that way in any case, I think. Protected JSValues prevent the context from being destroyed by the garbage collector. So, the values have a strong reference to the context anyway internally.

> 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

For now, it's enough with not installing the headers, the symbols won't be exposed in production builds anyway, until we use JSC_API like macro in the headers.

> >> 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/

Not really. In GTK+ we have a "library" with support classes shared by the tests. Those are in Tools/TestWebKitAPI/gtk/WebKit2Gtk/ because they are not tests themselves. Tools/TestWebKitAPI/gtk/main.cpp is the common main used by the google tests based tests. In our case we are not adding tests helper classes, but for now only tests, so they should go to Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/

> And I splited the files for readability... but I'll merge them.

We split "test suites" in files, with all the tests cases of the suite in the same file.

-- 
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/20161114/c5303c80/attachment.html>