[webkit-reviews] review denied: [Bug 181262] WebAssembly: add C API and shell : [Attachment 330501] patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied JF Bastien
<jfbastien at apple.com>'s request for review:
Bug 181262: WebAssembly: add C API and shell
https://bugs.webkit.org/show_bug.cgi?id=181262

Attachment 330501: patch

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




--- Comment #8 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 330501
  --> https://bugs.webkit.org/attachment.cgi?id=330501
patch

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

> Source/JavaScriptCore/API/WAContext.h:51
> +JS_EXPORT void WAContextRelease(WAContextRef context) CF_AVAILABLE(10_14,
12_0);

WAContextDestroy?

> Source/JavaScriptCore/API/WAModule.h:61
> +JS_EXPORT WAModuleRef WAModuleCreateFromText(WAContextRef context, const
char* module, size_t byteLength, WAExceptionRef* exception) CF_AVAILABLE(10_14,
12_0);

We can create a module from binary or from text.  Will anyone want to have a
getter for the text form or binary form?

> Source/JavaScriptCore/API/WAModule.h:87
> +JS_EXPORT void WAModuleCompile(WAContextRef context, WAModuleRef module,
WAExceptionRef* exception) CF_AVAILABLE(10_14, 12_0);

I don't think we need parameter names like context, module, and exception.

Do we always want to compile synchronously?  Will we ever want a callback once
the compiling is done on a different thread?

> Source/JavaScriptCore/API/WAModule.h:104
> +JS_EXPORT void WAModuleForEachExport(WAModuleRef module, void
(*action)(WAModuleRef, WAExportRef)) CF_AVAILABLE(10_14, 12_0);

I think all of these need a const void* that you give to the function that it
gives back in the callback so we can pass in a structure and have access to
things inside of the callback from outside of the callback.  We do something
similar in WKPageNavigationClient.  const void*.

> Source/JavaScriptCore/API/WAName.cpp:44
> +size_t WANameGetSize(WANameRef name)

This looks a lot like JSStringRef, which has "Length" instead of "Size".
Also, is the size always the same as the utf8 size even if you have characters
that need encoding?

JSStringRef has comparison functions.

Will we ever have anything else in WebAssembly that we use Strings for?

> Source/JavaScriptCore/API/WAName.h:57
> +JS_EXPORT void WANameRelease(WANameRef name) CF_AVAILABLE(10_14, 12_0);

Destroy?

> Source/JavaScriptCore/API/WAOptions.cpp:34
> +void WAEnableFastMemory()

Is there a way to disable fast memory?

> Source/JavaScriptCore/API/WAOptions.h:46
> + @discussion Fast memories aren't enabled by default because they require
large virtual allocations for each memory (slightly above 4GiB per), and
require installing signal handlers to handle out-of-bounds memory accesses.
Fast memory provides on the order of 20% performance improvement over explicit
bounds-check.

I'm not sure we should be making performance claims in the API.

> Source/JavaScriptCore/API/WASignature.h:48
> +JS_EXPORT void WASignatureRelease(WASignatureRef signature)
CF_AVAILABLE(10_14, 12_0);

There's no corresponding retain.

> Source/JavaScriptCore/API/WATableDescriptor.h:54
> +JS_EXPORT void WATableDescriptorRelease(WATableRef table)
CF_AVAILABLE(10_14, 12_0);

This has no implementation and no corresponding retain.