bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 16 20:34:26 PDT 2018
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
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.
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.
> + 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.
> + *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.
> +#if !defined(__JSC_H_INSIDE__) && !defined(JSC_COMPILATION)
> +#error "Only <jsc/jsc.h> can be included directly."
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.
> +typedef void (* JSCExceptionHandler) (JSCContext *context,
> +#ifdef G_OS_WIN32
> +# define JSC_API __declspec(dllexport)
> +# else
> +# define JSC_API __declspec(dllimport)
> +# endif
> +# define JSC_API __attribute__((visibility("default")))
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.
> + 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?
> +inline void Heap::releaseSoon(std::unique_ptr<JSCGLibWrapperObject>&& object)
> + 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.
> + 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...
More information about the webkit-unassigned