[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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

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

--- Comment #21 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 335929
  --> https://bugs.webkit.org/attachment.cgi?id=335929
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.

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.

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.

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

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

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

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

JSCExceptionHandlerCallback?

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

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

Use VERBATIM

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

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

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

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