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

[bugzilla-daemon at webkit.org]

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

--- Comment #22 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #21)
> Comment on attachment 335929 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335929&action=review
> 
> Impressive. I skimmed over about half of it... it's a lot.
> 
> I'd try asking Yusuke to review the API... it seems good to me, though.
> Except you're missing autoptr support, which is important. So please add
> that.

I'm missing a lot of things, this is a work in progress patch. It lacks documentation, gobject introspection and even functionality like jsc_value_equal.

> Once you add doc comments, we'll need to carefully test introspection to
> e.g. ensure it's possible to create and use a JSCClass from... say,
> JavaScript. I think it should be fine, but needs a little demo proof of
> concept to be sure. Of course we don't have to commit that.

Yes, the patch also needs a introspection compatible function for several methods that use varargs, for example. I started to work on this right after branching to have the whole release cycle to properly test this API.

> Also, perhaps we should not allow subclassing the public API GObjects? E.g.
> JSCClass. Subclassing that seems weird. Just because the rest of our API
> allows subclassing all the public types doesn't mean it's necessarily a good
> idea to follow that practice here... I can recommend some best-practice
> changes if you decide to make the public GObjects final instead of derivable.

That's a good point, specially for classes where there isn't new methods, it's not possible to subclass, because new instances are always created internally.

> > Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:50
> > +    JSC::JSAPIWrapperObject* wrapperObject = static_cast<JSC::JSAPIWrapperObject*>(handle.get().asCell());
> 
> I prefer to use auto* when doing a cast, because the type is written clearly
> on the rhs anyway.

Yes, me too.

> > Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:84
> > +        *exception = toRef(JSC::createTypeError(toJS(jsContext), ASCIILiteral("cannot call a class constructor without |new|")));
> 
> These errors should be translatable, right? Even normal GError messages we
> normally mark for translation.

I don't think javascript exceptions are translated at all. I don't think a message like "cannot call a class constructor without |new|" is useful for the end users either, it's the developer who made a mistake, and they surely understands the message. I think we can avoid translations in jsc.

> > Source/JavaScriptCore/API/glib/JSCClass.h:22
> > +#if !defined(__JSC_H_INSIDE__) && !defined(JSC_COMPILATION)
> > +#error "Only <jsc/jsc.h> can be included directly."
> > +#endif
> 
> I think we should move this underneath the include guards. It's a
> microoptimization, but it annoys me. We can change the existing public APIs
> later.

This is what we do everywhere else, I think. This won't be included twice in any case, since it will fail, no?

> > Source/JavaScriptCore/API/glib/JSCContext.h:47
> > +typedef void (* JSCExceptionHandler) (JSCContext   *context,
> 
> JSCExceptionHandlerCallback?

Isn't Handler and Callback kind of redundant?

> > Source/JavaScriptCore/API/glib/JSCDefines.h:43
> > +#ifdef G_OS_WIN32
> > +#    if defined(BUILDING_JavaScriptCore) || defined(STATICALLY_LINKED_WITH_JavaScriptCore)
> > +#        define JSC_API __declspec(dllexport)
> > +#    else
> > +#        define JSC_API __declspec(dllimport)
> > +#    endif
> > +#else
> > +#    define JSC_API __attribute__((visibility("default")))
> > +#endif
> 
> Seems like the G_OS_WIN32 condition creates the false hope that this code
> might be expected to work on Windows.

I don't see why not. Doesn't jsc compile in windows?

> I do think we should go ahead and define and use JSC_API, but we both know
> it's a no-op because we default to the default visibility anyway... real
> shame that I failed to fix that. Which reminds me: this will be broken for
> WPE, because the API symbols will all get dropped by WPE's linker version
> script. So that script will need to be updated.

I think it's good to have those just in case we are able to fix it at some point, but yes, I agree.

> > Source/JavaScriptCore/PlatformGTK.cmake:91
> > +add_custom_command(
> 
> Use VERBATIM

Ok.

> > Source/JavaScriptCore/heap/Heap.h:280
> > +    void releaseSoon(std::unique_ptr<JSCGLibWrapperObject>&&);
> 
> std::unique_ptr<JSCGLibWrapperObject>&& is a weird API, because it implies
> that the object was allocated using system malloc, rather than g_malloc() or
> bmalloc. Doesn't seem right. Can you explain?

I don't understand what you mean, std::unique_ptr will use delete, the object was allocated with new. It has nothing to do with system malloc vs bmalloc. Actually, JSCGLibWrapperObject is fast allocated so it will always be using bmalloc unless disabled.

> > Source/JavaScriptCore/heap/HeapInlines.h:171
> > +inline void Heap::releaseSoon(std::unique_ptr<JSCGLibWrapperObject>&& object)
> 
> Here too.
> 
> > Tools/TestWebKitAPI/PlatformGTK.cmake:141
> > +    WEBKIT_ADD_TARGET_CXX_FLAGS(TestJSC -Wno-sign-compare
> > +                                        -Wno-undef
> > +                                        -Wno-unused-parameter)
> 
> Please don't cargo cult this; instead, ensure your new tests don't trigger
> these warnings... I guess -Wno-unused-parameter is OK if you prefer that,
> because that warning is controversial in WebKit, but your code should really
> not be triggering -Wsign-compare and especially not -Wundef. I think we have
> these suppressed here because the gtest headers trigger them.

It's not my code what causes tons of warnings, it's the gtest third party code.

> > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:47
> > +        g_assert(m_watchedObjects.isEmpty());
> 
> Very minor: since these are new tests, I suggest we start using GLib's
> testing assertions, rather than normal g_assert(). This would become
> g_assert_true(). See https://bugzilla.gnome.org/show_bug.cgi?id=793856 for
> rationale.

Sure.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180317/4431fcaa/attachment.html>