RESOLVED FIXED Bug 148118
Introduce non-user-observable Promise functions to use Promises internally
https://bugs.webkit.org/show_bug.cgi?id=148118
Summary Introduce non-user-observable Promise functions to use Promises internally
Yusuke Suzuki
Reported 2015-08-17 20:17:04 PDT
Attachments
Patch (25.09 KB, patch)
2015-08-17 20:35 PDT, Yusuke Suzuki
saam: review+
Yusuke Suzuki
Comment 1 2015-08-17 20:35:00 PDT
Saam Barati
Comment 2 2015-08-18 11:18:58 PDT
Comment on attachment 259230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259230&action=review r=me with suggestions. > Source/JavaScriptCore/builtins/PromiseConstructor.js:158 > + var remainingElementsCount = 1; I think this would be easier to read if we started this at 0 and then check if the array length is 0 explicitly. Below, we can do: If (array length is zero): "promiseCapability.@resolve.@call(undefined, values);" else: for (....) { } (or switch the if/else) > Source/JavaScriptCore/runtime/JSPromiseDeferred.cpp:75 > +static void callFunction(ExecState* exec, JSValue function, JSValue value) This seems like it'd get inlined but maybe you should explicitly mark it as inline.
Yusuke Suzuki
Comment 3 2015-08-18 11:32:58 PDT
Comment on attachment 259230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259230&action=review Thank you for your review, Saam! >> Source/JavaScriptCore/builtins/PromiseConstructor.js:158 >> + var remainingElementsCount = 1; > > I think this would be easier to read if we started this at 0 and then check if the array length is 0 explicitly. > Below, we can do: > If (array length is zero): "promiseCapability.@resolve.@call(undefined, values);" > else: for (....) { } > > (or switch the if/else) That's great idea! Originally, this implementation corresponds to the spec's Promise.all implementation. In Promise.all implementation, since we need to iterate the taken iterable parameter (this may not be an array, so we cannot perform iterable.length) by for-of (this is user-observable), we and the spec need to perform the current code. But this privateAll does not have such a limitation; All access should not be user-observable. So we can just do so; checking the array.length === 0 before iterating the given value! >> Source/JavaScriptCore/runtime/JSPromiseDeferred.cpp:75 >> +static void callFunction(ExecState* exec, JSValue function, JSValue value) > > This seems like it'd get inlined but maybe you should explicitly mark it as inline. Sounds nice. I'll add `inline` to it.
Yusuke Suzuki
Comment 4 2015-08-18 12:38:36 PDT
After seeing the code, I've still found the problem; trapping the operation from the user side. To avoid the problem, I'll + land this patch without @then. The problem of then will be solved in the different patch + create the new constructor/prototype instance for the internal promises https://bugs.webkit.org/show_bug.cgi?id=148136 So, at first, I'll land this patch without @then.
Yusuke Suzuki
Comment 5 2015-08-18 13:44:02 PDT
(In reply to comment #4) > After seeing the code, I've still found the problem; trapping the operation > from the user side. > To avoid the problem, I'll > > + land this patch without @then. The problem of then will be solved in the > different patch > + create the new constructor/prototype instance for the internal promises > https://bugs.webkit.org/show_bug.cgi?id=148136 > > So, at first, I'll land this patch without @then. I discussed with Saam offline and we decide to do so.
Yusuke Suzuki
Comment 6 2015-08-18 15:32:18 PDT
Note You need to log in before you can comment on or make changes to this bug.