[webkit-reviews] review granted: [Bug 194517] Update JSScript SPI based on feedback : [Attachment 362654] patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 194517: Update JSScript SPI based on feedback
https://bugs.webkit.org/show_bug.cgi?id=194517

Attachment 362654: patch

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




--- Comment #27 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 362654
  --> https://bugs.webkit.org/attachment.cgi?id=362654
patch

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

r=me with comments.

> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:193
> +	       args.append(createTypeError(exec, makeString("The JSScript that
was provided did not have expected type of kJSScriptTypeModule.")));
> +	       call(exec, deferredPromise->JSPromiseDeferred::reject(), args,
"This should never be seen...");
> +	       return encodedJSUndefined();

Nit: you could probably make a lambda taking the error message that calls
reject and returns undefined.

> Source/JavaScriptCore/API/JSScript.h:62
> ++ (nullable instancetype)scriptWithSource:(NSString *)source
inVirtualMachine:(JSVirtualMachine *)vm JSC_API_DEPRECATED("Use
+scriptOfType:inVirtualMachine:withSource:andSourceURL:andBytecodeCache:error:
instead.", macosx(JSC_MAC_TBA, JSC_MAC_TBA), ios(JSC_IOS_TBA, JSC_IOS_TBA));
> +
> +/*!
> + This SPI is deprecated and should not be used. Use
"scriptOfType:inVirtualMachine:memoryMappedFromASCIIFile:withSourceURL:andBytec
odeCache:error:" instead.
> + */
> ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath
inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL
*)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath
JSC_API_DEPRECATED("Use
+scriptOfType:inVirtualMachine:memoryMappedFromASCIIFile:withSourceURL:andBytec
odeCache:error: instead.", macosx(JSC_MAC_TBA, JSC_MAC_TBA), ios(JSC_IOS_TBA,
JSC_IOS_TBA));
> +
> +/*!
> + This API is deprecated and should not be used.
> + */
> ++ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath
inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL
*)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath
JSC_API_DEPRECATED("Do not use this. Use
+scriptOfType:inVirtualMachine:memoryMappedFromASCIIFile:withSourceURL:andBytec
odeCache:error: or
+scriptOfType:inVirtualMachine:withSource:andSourceURL:andBytecodeCache:error:
instead", macosx(JSC_MAC_TBA, JSC_MAC_TBA), ios(JSC_IOS_TBA, JSC_IOS_TBA));

Can you make sure you update the deprecation warnings for the new method names?

> Source/JavaScriptCore/API/JSScript.h:76
> ++ (nullable instancetype)scriptOfType:(JSScriptType)type
inVirtualMachine:(JSVirtualMachine *)vm withSource:(NSString *)source
andSourceURL:(NSURL *)sourceURL andBytecodeCache:(nullable NSURL *)cachePath
error:(out NSError * _Nullable * _Nullable)error;

Per offline discussion we should move inVirtualMachine to the second to last
argument. Since, that's not a particularly interesting parameter when choosing
between the "overloads".

> Source/JavaScriptCore/API/JSScript.h:92
> ++ (nullable instancetype)scriptOfType:(JSScriptType)type
inVirtualMachine:(JSVirtualMachine *)vm memoryMappedFromASCIIFile:(NSURL
*)filePath withSourceURL:(NSURL *)sourceURL andBytecodeCache:(nullable NSURL
*)cachePath error:(out NSError * _Nullable * _Nullable)error;

ditto.