[Webkit-unassigned] [Bug 184041] [GLIB] Add JSCWeakValue to JavaScriptCore GLib API

[bugzilla-daemon at webkit.org]

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

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 336585 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336585&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:193
> > +        g_signal_emit(weakValue, signals[CLEARED], 0, nullptr);
> 
> I'm hesitant about this signal. Wouldn't it be better for the user to pass
> in a GWeakNotify to be called instead of using a signal? The signal makes it
> hard to use in performance-sensitive code.

This is not a weak reference to itself like GWeakNotify. GWeakNotify receives a GObject, what do we pass there? we don't really keep the JSCValue, so we don't have anything to pass there. The JSCWeakValue is still alive, so it doesn't make sense to pass the weak value either.

> I even thought about suggesting that JSCWeakValue be a GBoxed or even an
> opaque struct instead of a GObject, but I suppose since JSCValue is a
> GObject it's better to parallel that.

What's the point? The main advantage of being a GObject is that we can receive the JSCValue as a write-only construct-only parameter.

> > Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:260
> > +            static_cast<GParamFlags>(WEBKIT_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)));
> 
> Might want to add a warning comment as to why it must never be made readable.
> 
> > Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:299
> > + * Returns: (transfer full): a new #JSCValue or %NULL if @weak_value was cleared.
> 
> I was worried about the JSCValue being destroyed on the GC thread, but I
> suppose that cannot ever happen because you take a lock here and return a
> strong ref. That's the main thing I was looking to see in this review. Good.
> 
> > Source/JavaScriptCore/API/glib/JSCWeakValue.h:54
> > +    void (*_jsc_reserved0) (void);
> 
> It has a signal, why not expose it here?

The signal is just a notification, we don't have a default impl, and I don't expect subclasses need to override it at all. Actually I don't expect subclasses at all, but I don't want to limit it for no reason.

-- 
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/20180328/974cf3fb/attachment-0001.html>