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.
Created attachment 452988 [details] Draft Patch Missing some documentation and API tests, will add them later.
Created attachment 453048 [details] Draft patch v2 Now with documentation, tests still missing
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.
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.
(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*)`
(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 :)
Created attachment 453665 [details] Patch
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).
(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.
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.
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!
Created attachment 454042 [details] Patch > "will be also" -> "will also be". Change applied, thanks for the suggestion.
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
(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!
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”.
Created attachment 454483 [details] Patch for landing
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].