Bug 237088 – [GLib] Expose ArrayBuffer in the public API

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Adrian Perez]

To allow passing binary data efficiently between the C/C++ world
and the JavaScript world, we want to expose the ArrayBuffer primitive
in the GLib public API.

This is related to bug #197535 and will be a building block for it.

[Adrian Perez]

Created attachment 452988 [details]
Draft Patch

Missing some documentation and API tests, will add them later.

[Adrian Perez]

Created attachment 453048 [details]
Draft patch v2

Now with documentation, tests still missing

[Alberto Garcia]

Comment on attachment 453048 [details]
Draft patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=453048&action=review

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1486
> + * Creates a new %ArrayBuffer from existing @data in memory.

It's implicit by the context, but you could still say that the new ArrayBuffer does not copy the data so the caller must keep the pointer valid while the object is alive.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1567
> + * Returns: pointer to memory.

(transfer none) ?

> Source/JavaScriptCore/API/glib/JSCValue.h:261
> +JSC_API JSCValue *
> +jsc_value_new_array_buffer                (JSCContext           *context,
> +                                           gpointer              data,
> +                                           size_t                length,
> +                                           gpointer              user_data,
> +                                           GDestroyNotify        destroy_notify);
> +
> +JSC_API gboolean
> +jsc_value_is_array_buffer                 (JSCValue             *value);
> +
> +JSC_API gpointer
> +jsc_value_array_buffer_get_data           (JSCValue             *value,
> +                                           gsize                *length);

You use size_t in one case and gsize in the other one.

[Patrick Griffis]

Comment on attachment 453048 [details]
Draft patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=453048&action=review

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1571
> +gpointer jsc_value_array_buffer_get_data(JSCValue* value, gsize* length)

Could there be a variant of `GBytes* jsc_value_array_buffer_get_data(JSCValue*)`?

It would be safer and nicer for bindings.

[Patrick Griffis]

(In reply to Patrick Griffis from comment #4)
> Could there be a variant of `GBytes*
> jsc_value_array_buffer_get_data(JSCValue*)`?

I meant `GBytes* jsc_value_array_buffer_get_bytes(JSCValue*)`

[Adrian Perez]

(In reply to Alberto Garcia from comment #3)
> Comment on attachment 453048 [details]
> Draft patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453048&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1486
> > + * Creates a new %ArrayBuffer from existing @data in memory.
> 
> It's implicit by the context, but you could still say that the new
> ArrayBuffer does not copy the data so the caller must keep the pointer valid
> while the object is alive.

Good idea, I'll rewrite the documentation to make this clear.

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1567
> > + * Returns: pointer to memory.
> 
> (transfer none) ?

Good catch, I'll add the annotation.

> > Source/JavaScriptCore/API/glib/JSCValue.h:261
> > +JSC_API JSCValue *
> > +jsc_value_new_array_buffer                (JSCContext           *context,
> > +                                           gpointer              data,
> > +                                           size_t                length,
> > +                                           gpointer              user_data,
> > +                                           GDestroyNotify        destroy_notify);
> > +
> > +JSC_API gboolean
> > +jsc_value_is_array_buffer                 (JSCValue             *value);
> > +
> > +JSC_API gpointer
> > +jsc_value_array_buffer_get_data           (JSCValue             *value,
> > +                                           gsize                *length);
> 
> You use size_t in one case and gsize in the other one.

In practice doesn't matter (they are the same type) but for the sake of
consistency let's use “gsize” as this is a GLib-style API header :)

[Adrian Perez]

Created attachment 453665 [details]
Patch

[Alberto Garcia]

Comment on attachment 453665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453665&action=review

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1484
> + * jsc_value_new_array_buffer:
> + * @context: A #JSCContext
> + * @data: Pointer to a region of memory.
> + * @length: Size in bytes of the memory region.
> + * @user_data: (closure): user data.
> + * @destroy_notify: (nullable): destroy notifier for @user_data.

I think that since @user_data is only used for the destroy notifier it should probably come *afterwards* in the list of parameters.

Compare g_bytes_new_with_free_func(), which has separate @data and @user_data like this one (https://docs.gtk.org/glib/ctor.Bytes.new_with_free_func.html) with g_source_set_callback(), where the same data pointer is also used for the destroy notifier (https://docs.gtk.org/glib/method.Source.set_callback.html).

[Adrian Perez]

(In reply to Alberto Garcia from comment #8)
> Comment on attachment 453665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453665&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1484
> > + * jsc_value_new_array_buffer:
> > + * @context: A #JSCContext
> > + * @data: Pointer to a region of memory.
> > + * @length: Size in bytes of the memory region.
> > + * @user_data: (closure): user data.
> > + * @destroy_notify: (nullable): destroy notifier for @user_data.
> 
> I think that since @user_data is only used for the destroy notifier it
> should probably come *afterwards* in the list of parameters.
> 
> Compare g_bytes_new_with_free_func(), which has separate @data and
> @user_data like this one
> (https://docs.gtk.org/glib/ctor.Bytes.new_with_free_func.html) with
> g_source_set_callback(), where the same data pointer is also used for the
> destroy notifier (https://docs.gtk.org/glib/method.Source.set_callback.html).

Good point, let's swap the order of the user_data and destroy_notify
parameters to make this more consistent with other GLib-style APIs.

[Adrian Perez]

Created attachment 453715 [details]
Patch


New version, with parameters for jsc_value_new_array_buffer() swapped as
suggested by Alberto, and documentation for jsc_value_array_buffer_get_data()
hopefully clearer.

[Alberto Garcia]

Comment on attachment 453715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453715&action=review

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1566
> + * will be also stored in the pointed location.

"will be also" -> "will also be".

The patch looks otherwise fine to me, thanks!

[Adrian Perez]

Created attachment 454042 [details]
Patch


> "will be also" -> "will also be".

Change applied, thanks for the suggestion.

[Carlos Garcia Campos]

Comment on attachment 454042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454042&action=review

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1510
> + * Returns: (transfer full): A #JSCValue

or %NULL in case of exception

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1517
> +    g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr);
> +

g_return_val_if_fail(data || !size, nullptr);

Assuming it's valid to create and empty array buffer, I would add a test case for that too.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1527
> +    auto* jsArrayBuffer = JSObjectMakeArrayBufferWithBytesNoCopy(jsContext, data, length, deallocatorContext ? jscArrayBufferDeallocate : nullptr, deallocatorContext, &exception);

Why not just always pass a  function and simply return early if the given context is nullptr? We could use a lambda.

> Source/JavaScriptCore/API/glib/JSCValue.cpp:1551
> +    auto jsTypedArrayType = JSValueGetTypedArrayType(jsContext, value->priv->jsValue, &exception);

This is confusing. I know this is how JSValueGetTypedArrayType is implemented, it checks if the given value is an ArrayBuffer and then returns kJSTypedArrayTypeArrayBuffer. I would add a comment here, or even better I prefer to be explicit here even if it's more code I would do the jsDynamicCast<JSArrayBuffer*>(vm, object) here

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #13)
> Comment on attachment 454042 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454042&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1510
> > + * Returns: (transfer full): A #JSCValue
> 
> or %NULL in case of exception

👌️

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1517
> > +    g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr);
> > +
> 
> g_return_val_if_fail(data || !size, nullptr);
> 
> Assuming it's valid to create and empty array buffer, I would add a test
> case for that too.

Yes, it's valid to have a zero-sized ArrayBuffer. I missed that
case when writing tests, I'll add it. 

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1527
> > +    auto* jsArrayBuffer = JSObjectMakeArrayBufferWithBytesNoCopy(jsContext, data, length, deallocatorContext ? jscArrayBufferDeallocate : nullptr, deallocatorContext, &exception);
> 
> Why not just always pass a  function and simply return early if the given
> context is nullptr? We could use a lambda.

Good idea, the code will look a bit less messy that way. 

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:1551
> > +    auto jsTypedArrayType = JSValueGetTypedArrayType(jsContext, value->priv->jsValue, &exception);
> 
> This is confusing. I know this is how JSValueGetTypedArrayType is
> implemented, it checks if the given value is an ArrayBuffer and then returns
> kJSTypedArrayTypeArrayBuffer. I would add a comment here, or even better I
> prefer to be explicit here even if it's more code I would do the
> jsDynamicCast<JSArrayBuffer*>(vm, object) here

Sure, I can put the jsDynamicCast directly here for the sake of clarity.

Thanks for all the comments!

[Adrian Perez]

Also, as per discusstion with Carlos Garcia in a private chat, we are
going to change _array_buffer_get_length() to be _get_size(), because
in GLib style APIs “size” is more commonly used for “amount of bytes”
and “length” for “number of elements”.

[Adrian Perez]

Created attachment 454483 [details]
Patch for landing

[EWS]

Committed r291194 (248350@main): <https://commits.webkit.org/248350@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454483 [details].