Add Promise SPI
Created attachment 350327 [details] Patch
Created attachment 350328 [details] Patch
patched working https://bsnlspeedtests.in/
Created attachment 350354 [details] Patch
Attachment 350354 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1732: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1742: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350354 [details] Patch Attachment 350354 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9296059 Number of test failures exceeded the failure limit.
Created attachment 350363 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 350378 [details] Patch
Attachment 350378 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1732: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1742: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 350382 [details] Patch
Attachment 350382 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1732: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1742: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350382&action=review r=me with comments > Source/JavaScriptCore/ChangeLog:15 > + in/to that callback the promise is automagically rejected. The I don't know what you mean by "to that callback" here > Source/JavaScriptCore/ChangeLog:16 > + other methods create an pre-resolved or rejected promise as this an => a > Source/JavaScriptCore/ChangeLog:17 > + appears to be the most common way to initialize a promise. "the most common" => "a common" > Source/JavaScriptCore/ChangeLog:24 > + corrupt state if an Obj-C exception unwinds JS frames. unwinds => "unwinds through" or "unwinds past" > Source/JavaScriptCore/API/JSObjectRef.cpp:294 > + JSPromiseDeferred::DeferredData data = JSPromiseDeferred::createDeferredData(exec, globalObject, globalObject->promiseConstructor()); > + handleExceptionIfNeeded(scope, exec, nullptr); The semantics here seem weird, or at least non-obvious to me, if this actually throws. I think we should set the passed in exception if this throws. Otherwise, it's pretty weird that the code below just stores null. > Source/JavaScriptCore/API/JSValue.mm:163 > + JSObjectRef promise = JSObjectMakeDeferredPromise([context JSGlobalContextRef], &resolve, &reject, &exception); is it worth stating this can throw in the documentation of the SPI? When does this actually throw? > Source/JavaScriptCore/API/JSValue.mm:180 > + [rejection callWithArguments:@[context.exception]]; Does this automatically clear the exception? I don't think it does. Do we want to clear it? > Source/JavaScriptCore/API/JSValuePrivate.h:38 > + being initialized. The resolve and reject parameters are functions that > + can be called to notify any dependent promises about the state of the > + new promise JSValue. This explanation feels weird to me. Maybe we can state it less in terms of promises down the chain, and just in terms of resolve/reject? > Source/JavaScriptCore/API/JSValuePrivate.h:42 > + the resolve and reject callbacks each normally take a single result value, which they "result value" seems weird in the context of reject. Maybe just say "single value"? > Source/JavaScriptCore/API/JSValuePrivate.h:57 > ++ (JSValue *)valueWithNewResolvedPromiseWithResult:(id)result inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA), ios(JSC_IOS_TBA)); nit: This name feels overly verbose to me. Maybe: valueWithNewPromiseResolvedWith > Source/JavaScriptCore/API/JSValuePrivate.h:62 > + @param reason The result value to be passed to any reactions. ditto about "result" value here. Seems like we need some other way to state failure other than "result". How does TC39 distinguish between them in naming? > Source/JavaScriptCore/API/JSValuePrivate.h:67 > ++ (JSValue *)valueWithNewRejectedPromiseWithReason:(id)reason inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA), ios(JSC_IOS_TBA)); ditto > Source/JavaScriptCore/API/tests/testapi.c:1982 > + const char *scriptPath = "testapi.js"; "const char *" => "const char* " > Source/JavaScriptCore/API/tests/testapi.cpp:88 > + operator JSC::ExecState*() { return toJS(m_context); } 👌 > Source/JavaScriptCore/API/tests/testapi.mm:1729 > + checkResult(@"Exception set in callback should not propegate", !context.exception); propegate => propagate > Source/JavaScriptCore/API/tests/testapi.mm:1739 > + checkResult(@"Exception thrown in callback should not propegate", !context.exception); propegate => propagate > Source/JavaScriptCore/runtime/JSPromiseDeferred.h:48 > + WTF_FORBID_HEAP_ALLOCATION; style nit: Put this at the top > Source/WTF/wtf/mac/MainThreadMac.mm:65 > +static bool mainThreadEstablishedAsPthreadMain { false }; > +static pthread_t mainThreadPthread { nullptr }; > +static NSThread* mainThreadNSThread { nullptr }; Though I like this style more, I think WK style does say we don't do this for static variables.
Comment on attachment 350382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350382&action=review >> Source/JavaScriptCore/API/JSObjectRef.cpp:294 >> + handleExceptionIfNeeded(scope, exec, nullptr); > > The semantics here seem weird, or at least non-obvious to me, if this actually throws. I think we should set the passed in exception if this throws. Otherwise, it's pretty weird that the code below just stores null. Whoops, I was thinking this was like the RETURN_IF_EXCEPTION macro... I'll fix that. There's not actually a way to create an exception here other than via the watchdog right now. >> Source/JavaScriptCore/API/JSValue.mm:163 >> + JSObjectRef promise = JSObjectMakeDeferredPromise([context JSGlobalContextRef], &resolve, &reject, &exception); > > is it worth stating this can throw in the documentation of the SPI? When does this actually throw? It doesn't right now. I don't think the SPI explicitly states which parts can throw. AFAIK, it should be assumed that any call can set the exception state. >> Source/JavaScriptCore/API/JSValue.mm:180 >> + [rejection callWithArguments:@[context.exception]]; > > Does this automatically clear the exception? I don't think it does. Do we want to clear it? The exception property on the context isn't the same as the exception on the VM. When we return to Obj-C from JS we set the exception property on context and clear the VM. When we are calling the rejection we will enter JS code so the VM won't have a pending exception. If for what ever reason (I don't think it's possible right now) we called an Obj-C callback then we would enter a new CallbackData, which would stash the context's exception. >> Source/JavaScriptCore/API/JSValuePrivate.h:42 >> + the resolve and reject callbacks each normally take a single result value, which they > > "result value" seems weird in the context of reject. Maybe just say "single value"? Sure. >> Source/JavaScriptCore/API/JSValuePrivate.h:57 >> ++ (JSValue *)valueWithNewResolvedPromiseWithResult:(id)result inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA), ios(JSC_IOS_TBA)); > > nit: This name feels overly verbose to me. Maybe: valueWithNewPromiseResolvedWith OK, changed. >> Source/JavaScriptCore/API/JSValuePrivate.h:62 >> + @param reason The result value to be passed to any reactions. > > ditto about "result" value here. Seems like we need some other way to state failure other than "result". How does TC39 distinguish between them in naming? ditto. >> Source/JavaScriptCore/API/JSValuePrivate.h:67 >> ++ (JSValue *)valueWithNewRejectedPromiseWithReason:(id)reason inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA), ios(JSC_IOS_TBA)); > > ditto Ditto. >> Source/JavaScriptCore/API/tests/testapi.c:1982 >> + const char *scriptPath = "testapi.js"; > > "const char *" => "const char* " switched. >> Source/JavaScriptCore/API/tests/testapi.mm:1729 >> + checkResult(@"Exception set in callback should not propegate", !context.exception); > > propegate => propagate Fixed. I wish Xcode had a spell checker inside things it styles as a string! >> Source/JavaScriptCore/API/tests/testapi.mm:1739 >> + checkResult(@"Exception thrown in callback should not propegate", !context.exception); > > propegate => propagate Ditto. >> Source/JavaScriptCore/runtime/JSPromiseDeferred.h:48 >> + WTF_FORBID_HEAP_ALLOCATION; > > style nit: Put this at the top Ok, >> Source/WTF/wtf/mac/MainThreadMac.mm:65 >> +static NSThread* mainThreadNSThread { nullptr }; > > Though I like this style more, I think WK style does say we don't do this for static variables. Really? Where? I searched for "static" and "global" and didn't see anything in https://webkit.org/code-style-guidelines/...
Comment on attachment 350382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350382&action=review >> Source/JavaScriptCore/ChangeLog:15 >> + in/to that callback the promise is automagically rejected. The > > I don't know what you mean by "to that callback" here Oh, I mean if you called into JS that raises an exception and you don't clear the exception from the JSContext.
Created attachment 350413 [details] Patch for landing
Attachment 350413 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1732: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1742: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350413 [details] Patch for landing Clearing flags on attachment: 350413 Committed r236359: <https://trac.webkit.org/changeset/236359>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44692249>
This change broke the Windows build: C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\API\tests\testapi.cpp(488): error C3861: 'strcasestr': identifier not found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\shell\testapiLib.vcxproj] https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/12011/steps/compile-webkit/logs/stdio
Comment on attachment 350413 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=350413&action=review > Source/JavaScriptCore/API/tests/testapi.cpp:488 > + return !filter || !!strcasestr(testName, filter); This broke the Windows build.
Reverted r236359 for reason: Broke the Windows build. Committed r236371: <https://trac.webkit.org/changeset/236371>
relanded in <https://trac.webkit.org/changeset/236372>