Update JSScript SPI from feedback
Created attachment 361720 [details] Patch
*** Bug 194574 has been marked as a duplicate of this bug. ***
*** Bug 194573 has been marked as a duplicate of this bug. ***
Created attachment 361924 [details] WIP
rdar://problem/47712611
Spoke with Keith. I'm going to take this over.
Created attachment 362351 [details] WIP it compiles
Comment on attachment 362351 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=362351&action=review > Source/JavaScriptCore/API/JSContext.mm:109 > + JSValueRef result = JSEvaluateScript(context->m_context, source.get(), nullptr, url.get(), 0, &exceptionValue); We need to make this actually load the cached bytecode script instead of re-evaluating the script.
Created attachment 362364 [details] WIP
Created attachment 362365 [details] WIP We should actually be running the cached bytecode now, but I haven't tested it yet.
Created attachment 362414 [details] WIP
Created attachment 362464 [details] WIP Need to add tests and clean up some code, then it should be done.
Created attachment 362473 [details] WIP
Created attachment 362579 [details] WIP
Created attachment 362626 [details] patch
Attachment 362626 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/JSAPIGlobalObject.h:60: The parameter name "script" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/API/JSScript.mm:215: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393: Declaration has space between * and variable name in char* newCWD [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2276: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 362635 [details] patch Attempt build fixes
Attachment 362635 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393: Declaration has space between * and variable name in char* newCWD [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2285: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 362644 [details] patch Try to fix more builds
Attachment 362644 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393: Declaration has space between * and variable name in char* newCWD [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2287: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 362635 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362635&action=review > Source/JavaScriptCore/ChangeLog:33 > + When writing tests, I also discovered that someone previously broke > + testapi. This patch also fixes those failures. They were broken when > + we switched to using a testapiScripts directory to hold our test .js > + scripts. Whoops! > Source/JavaScriptCore/API/JSAPIGlobalObject.mm:80 > + URL referrerURL(URL(), referrerString->value(exec)); Don't you need an exception check here? > Source/JavaScriptCore/API/JSContextPrivate.h:81 > + @discussion If the provided JSScript was created with JSScriptTypeProgram, the script will run synchronously and return the result of evaluation. Typo: should be kJSScriptTypeProgram, I think > Source/JavaScriptCore/API/JSContextPrivate.h:83 > + Otherwise, if the script was created with JSScriptTypeModule, the module will be run asynchronously and will return a promise resolved when the module and any transitive dependencies are loaded. The module loader will treat the script as if it had been returned from a delegate call to moduleLoaderDelegate. This mirrors the JavaScript dynamic import operation. Ditto for kJSScriptTypeModule. > Source/JavaScriptCore/API/JSScript.h:62 > ++ (nullable instancetype)scriptWithSource:(NSString *)source inVirtualMachine:(JSVirtualMachine *)vm; > + > +/*! > + This SPI is deprecated and should not be used. Use "scriptOfType:inVirtualMachine:memoryMappedFromASCIIFile:withSourceURL:andBytecodeCache:error:" instead. > + */ > ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; > + > +/*! > + 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; Can you add deprecation warnings to these methods? > Source/JavaScriptCore/API/JSScript.h:76 > ++ (nullable instancetype)scriptOfType:(JSScriptType)type inVirtualMachine:(JSVirtualMachine *)vm withSourceURL:(NSURL *)sourceURL andSource:(NSString *)source andBytecodeCache:(nullable NSURL *)cachePath error:(out NSError * _Nullable * _Nullable)error; Why does this one have source at the end but the method below have it right after the VM? I also think that the parts of the method most likely to impact the version you choose should come earlier, which is why I put the source/filePath first originally. > Source/JavaScriptCore/API/tests/testapi.mm:2066 > + CRASH(); I think we usually just do a check here. > Source/JavaScriptCore/API/tests/testapi.mm:2071 > + RELEASE_ASSERT(result); > + RELEASE_ASSERT([result isNumber]); ditto. > Source/JavaScriptCore/API/tests/testapi.mm:2098 > + test(kJSScriptTypeProgram); > + test(kJSScriptTypeModule); Can we make this a parameter and add two lines to the filter? > Source/JavaScriptCore/API/tests/testapi.mm:2217 > + size_t afterCount = proc_pidinfo(getpid(), PROC_PIDLISTFDS, 0, 0, 0); You need another #ifdef here.
Comment on attachment 362635 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362635&action=review >> Source/JavaScriptCore/API/JSScript.h:76 >> ++ (nullable instancetype)scriptOfType:(JSScriptType)type inVirtualMachine:(JSVirtualMachine *)vm withSourceURL:(NSURL *)sourceURL andSource:(NSString *)source andBytecodeCache:(nullable NSURL *)cachePath error:(out NSError * _Nullable * _Nullable)error; > > Why does this one have source at the end but the method below have it right after the VM? > > I also think that the parts of the method most likely to impact the version you choose should come earlier, which is why I put the source/filePath first originally. I thought ObjC style was to include similar parameters in the same argument index where possible. I could be wrong though, I'm not very familiar with ObjC API style design. I moved the "source code" parameter to be at the same index for these two new APIs. >> Source/JavaScriptCore/API/tests/testapi.mm:2066 >> + CRASH(); > > I think we usually just do a check here. That gets super cumbersome. Also, maybe crashing this script will actually make people realized they broke things. All other comments will be fixed.
Created attachment 362653 [details] patch
Comment on attachment 362653 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362653&action=review > Source/JavaScriptCore/API/tests/testapi.mm:2216 > + checkResult(@"JSScript should not hold a file descriptor", count == afterCount); oops, this needs to go above.
Created attachment 362654 [details] patch
Attachment 362654 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393: Declaration has space between * and variable name in char* newCWD [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2283: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 362654 [details] 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:andBytecodeCache: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:andBytecodeCache: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:andBytecodeCache: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.
Created attachment 362656 [details] patch
Attachment 362656 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393: Declaration has space between * and variable name in char* newCWD [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2283: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 362657 [details] patch for landing
Attachment 362657 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393: Declaration has space between * and variable name in char* newCWD [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2283: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 362663 [details] patch for landing
Attachment 362663 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393: Declaration has space between * and variable name in char* newCWD [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2283: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 362663 [details] patch for landing Clearing flags on attachment: 362663 Committed r241929: <https://trac.webkit.org/changeset/241929>
All reviewed patches have been landed. Closing bug.