[webkit-reviews] review granted: [Bug 237088] [GLib] Expose ArrayBuffer in the public API : [Attachment 454042] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 237088: [GLib] Expose ArrayBuffer in the public API
https://bugs.webkit.org/show_bug.cgi?id=237088

Attachment 454042: Patch

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




--- Comment #13 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 454042
  --> https://bugs.webkit.org/attachment.cgi?id=454042
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