[webkit-reviews] review denied: [Bug 220323] [WASM-References] Add optional default value parameter for Table.grow : [Attachment 416994] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Dmitry <dbezhetskov at igalia.com>'s
request for review:
Bug 220323: [WASM-References] Add optional default value parameter for
Table.grow
https://bugs.webkit.org/show_bug.cgi?id=220323

Attachment 416994: Patch

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




--- Comment #2 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 416994
  --> https://bugs.webkit.org/attachment.cgi?id=416994
Patch

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

r- since I found a bug. We also need description more about the change.

> Source/JavaScriptCore/ChangeLog:8
> +	   Introduce the Table.grow optional default value.

ChangeLog is super important in WebKit project.
Please add more comments about changes in ChangeLog.
We need more description about the change in particular, for feature change.

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:100
> +    JSValue defaultValue = callFrame->argument(1);
> +    if (defaultValue.isUndefined())
> +	   defaultValue = jsNull();
>      uint32_t oldLength = table->length();
>  
> -    if (!table->grow(delta))
> +    if (!table->grow(delta, defaultValue))
>	   return JSValue::encode(throwException(globalObject, throwScope,
createRangeError(globalObject, "WebAssembly.Table.prototype.grow could not grow
the table"_s)));

I think this has a bug.
If table is function ref table, then values that can be configured to the table
is limited.
Can you align the implementation to the spec's change?
https://webassembly.github.io/reference-types/js-api/index.html#dom-table-grow

5. If value is missing,
5.1. Let ref be DefaultValue(elementType).
6. Otherwise,
6.1. Let ref be ? ToWebAssemblyValue(value, elementType).