Bug 148118 – Introduce non-user-observable Promise functions to use Promises internally

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Spawned from https://bugs.webkit.org/show_bug.cgi?id=147876

[Yusuke Suzuki]

Created attachment 259230 [details]
Patch

[Saam Barati]

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 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]

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]

(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]

Committed r188603: <http://trac.webkit.org/changeset/188603>