WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Spawned from
https://bugs.webkit.org/show_bug.cgi?id=147876
Attachments
Patch
(25.09 KB, patch)
2015-08-17 20:35 PDT
,
Yusuke Suzuki
saam
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2015-08-17 20:35:00 PDT
Created
attachment 259230
[details]
Patch
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
Committed
r188603
: <
http://trac.webkit.org/changeset/188603
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug