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

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver 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 330417: patch

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




--- Comment #5 from Oliver Hunt <oliver at apple.com> ---
Comment on attachment 330417
  --> https://bugs.webkit.org/attachment.cgi?id=330417
patch

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

There's a lot of work needed for this. The Release functions should all be
single parameter, otherwise you're just requiring the developer to maintain two
separate object lifetimes.

Almost all const char*s should be JSStringRef, ditto for WAName, etc.  Have a
look at the rest of the JSC C API to get an idea of what things are char* vs.
jsstringref

> Source/JavaScriptCore/API/APICast.h:56
> +class ExecState;
> +class PropertyNameArray;
> +class VM;
> +class JSObject;
> +class JSValue;
> +namespace Wasm {
> +struct Context;
> +struct Export;
> +struct Import;
> +class Memory;
> +class Module;
> +class Signature;
> +class Table;
> +namespace API {
> +struct Exception;
> +struct Function;
> +struct Global;
> +struct ImportObject;
> +struct Instance;
> +struct Name;
> +}
> +}

r-

> Source/JavaScriptCore/API/WABase.h:75
> + at constant kWAExceptionAPIUsageFailure Failure wasn't due to WebAssembly, but
to usage of the API.
> + at constant kWAExceptionValidationError Validation error of a WebAssembly
Module.
> + at constant kWAExceptionCompileError	 Compile error during validation or
decoding of a WebAssembly Module.
> + at constant kWAExceptionLinkError	 Link error during instantiation of a
WebAssembly Module (other than traps from the start function).
> + at constant kWAExceptionRuntimeError	 Runtime error due to a WebAssembly
trap during execution.
> + at constant kWAExceptionStackOverflow	 Stack overflow.
> + at constant kWAExceptionOutOfMemory	 Out of memory.

I'm not sure I like the wording of the descriptions for these

> Source/JavaScriptCore/API/WABase.h:88
> +/*! @typedef A WebAssembly context holds global state which enables fast
execution. */

"fast execution"?

Why isn't this something like "A WAContextRef provides the environment for
execution a web assembly program"

> Source/JavaScriptCore/API/WABase.h:118
> +/*! @typedef A WebAssembly name, specified to be a valid UTF-8 byte
sequence. */

if this is a utf8 string, then this should just be const char*, but I'm fairly
sure you actually want it to be a ref counted string, so my inclination would
be to replace WANameRef with JSStringRef -- we don't need more string types.

> Source/JavaScriptCore/API/WAContext.h:44
> +JS_EXPORT WAContextRef WAContextCreate() CF_AVAILABLE(10_14, 12_0);

Are there any properties you could want?

This should probably take a versioned configuration struct, I would expect that
to at minimum include a name field, so people can identify their contexts.

> Source/JavaScriptCore/API/WAException.cpp:42
> +const char* WAExceptionGetReason(WAExceptionRef exception)

JSStringRef would be more consistent, but jsstringref is so terrible to work
with. ggaren may have feelings on this.

> Source/JavaScriptCore/API/WAException.h:64
> +JS_EXPORT const char* WAExceptionGetReason(WAExceptionRef exception)
CF_AVAILABLE(10_14, 12_0);

JSStringRef?

> Source/JavaScriptCore/API/WAExport.h:49
> +JS_EXPORT void WAExportRelease(WAModuleRef module, WAExportRef expor)
CF_AVAILABLE(10_14, 12_0);

Release functions don't generally take more than one argument -- if a
WAExportRef is strictly tied to a Module, then it should retain that
information internally. You would presumably need to do that anyway to guard
against corruptions due to misuse of the API.

(The fact that JSValue[Unp/P]rotect requires a context make JSC obtuse to use)

> Source/JavaScriptCore/API/WAExport.h:58
> +JS_EXPORT WANameRef WAExportGetField(WAModuleRef module, WAExportRef expor)
CF_AVAILABLE(10_14, 12_0);

GetFieldName? JSStringRef.

Remove the module parameter.

> Source/JavaScriptCore/API/WAExport.h:67
> +JS_EXPORT WAExternalKind WAExportGetKind(WAModuleRef module, WAExportRef
expor) CF_AVAILABLE(10_14, 12_0);

remove module

> Source/JavaScriptCore/API/WAFunction.h:46
> + */

my inclination here would be const void* and void*. I think that there should
also be a return type so you can report an error from the call back.

I would probably do (and making the typedef will be necessary for API review);


typedef int32_t (*WAFunctionCallBack)(WASignatureRef, const void* arguments,
void* return_value);

This also makes the layout ABI, and requires developers to understand how that
works. If you had

typedef struct OpaqueWAFunctionArguments *WAFunctionArgumentsRef;

WASignatureRef WAFunctionArgumentsGetSignature(WAFunctionArgumentsRef);

WAFunctionArgumentsGetArgument(WAFunctionArgumentsRef, WAValueType, void* out);

WAFunctionArgumentsSetReturn(WAFunctionArgumentsRef, WAValueType, void* in);

Having got to the end WAFunctionArguments should probably be something like
WAFunctionCallContext

> Source/JavaScriptCore/API/WAFunction.h:73
> +JS_EXPORT void WAFunctionCall(WAInstanceRef instance, const char
arguments[], char returns[], WAFunctionRef function, WAExceptionRef* exception)
CF_AVAILABLE(10_14, 12_0);

WACallContextRef :D

> Source/JavaScriptCore/API/WAImportObject.h:87
> +JS_EXPORT void WAImportObjectAddMemory(WAImportObjectRef importObject, const
char* module, size_t moduleLength, const char * field, size_t fieldLength,
WAMemoryRef memory) CF_AVAILABLE(10_14, 12_0);

Why is this not an array strings? is the '.' semantically important? What
happens if I do Add*(..., "foo.bar", "wibble", ...)? Does it need to be
separated? or can we just use .'s?

JSStringRef. :D

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

Am I allowed to compile without validating first? I assume so, but I want
confirmation (not in the documentation, just for review purposes)