Bug 197600 - Implement Promise.allSettled
Summary: Implement Promise.allSettled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL: https://github.com/tc39/proposal-prom...
Keywords: InRadar, WebExposed
: 196332 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-05-05 14:07 PDT by Dean Jackson
Modified: 2019-05-29 16:04 PDT (History)
9 users (show)

See Also:


Attachments
WIP (2.74 KB, patch)
2019-05-05 15:33 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.60 MB, application/zip)
2019-05-05 17:45 PDT, Build Bot
no flags Details
WIP 2 (2.80 KB, patch)
2019-05-06 11:38 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (13.95 MB, application/zip)
2019-05-06 13:00 PDT, Build Bot
no flags Details
Patch (11.04 KB, patch)
2019-05-28 15:17 PDT, Dean Jackson
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2019-05-05 14:07:33 PDT
Implement Promise.allSettled
Comment 1 Dean Jackson 2019-05-05 14:17:31 PDT
https://github.com/tc39/proposal-promise-allSettled/

Shipping in Firefox since version 68.
Shipping in V8 since https://chromium.googlesource.com/v8/v8.git/+/1f6d27e8df819b448712dface6ad367fb8de426b
Comment 2 Radar WebKit Bug Importer 2019-05-05 14:17:49 PDT
<rdar://problem/50483885>
Comment 3 Radar WebKit Bug Importer 2019-05-05 14:17:51 PDT
<rdar://problem/50483886>
Comment 4 Dean Jackson 2019-05-05 15:33:59 PDT
Created attachment 369095 [details]
WIP
Comment 5 Build Bot 2019-05-05 17:45:10 PDT
Comment on attachment 369095 [details]
WIP

Attachment 369095 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12107506

New failing tests:
http/tests/css/filters-on-iframes.html
Comment 6 Build Bot 2019-05-05 17:45:12 PDT
Created attachment 369096 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 7 Dean Jackson 2019-05-06 11:38:41 PDT
Created attachment 369149 [details]
WIP 2
Comment 8 Keith Miller 2019-05-06 11:45:27 PDT
Comment on attachment 369149 [details]
WIP 2

View in context: https://bugs.webkit.org/attachment.cgi?id=369149&action=review

> Source/JavaScriptCore/builtins/PromiseConstructor.js:87
> +    return @Promise.all.@call(this, @Array.from(iterable, function (item) {

I don't think this is correct. You can't use properties of the private globals unless the spec says to do so since they can be changed by the client and thus can be observed and/or changed.
Comment 9 Build Bot 2019-05-06 13:00:30 PDT
Comment on attachment 369149 [details]
WIP 2

Attachment 369149 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12114097

New failing tests:
legacy-animation-engine/compositing/reflections/load-video-in-reflection.html
Comment 10 Build Bot 2019-05-06 13:00:32 PDT
Created attachment 369161 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 11 Ross Kirsling 2019-05-10 15:41:05 PDT
*** Bug 196332 has been marked as a duplicate of this bug. ***
Comment 12 Yusuke Suzuki 2019-05-10 16:50:22 PDT
Comment on attachment 369149 [details]
WIP 2

View in context: https://bugs.webkit.org/attachment.cgi?id=369149&action=review

Some early comment.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:88
> +        var promiseCapability = @newPromiseCapability(outerThis);

You can just use arrow function if necessary. But seems that the spec requires manual iteration. So I think we should do here.
Comment 13 Mathias Bynens 2019-05-11 18:57:20 PDT
(In reply to Ross Kirsling from comment #11)
> *** Bug 196332 has been marked as a duplicate of this bug. ***

Slightly off-topic, but why is it that whenever I file a WebKit bug, it ultimately gets closed as a duplicate of another bug that was filed *after* my initial report? This happened more than a few times already. Should I just give up?
Comment 14 Saam Barati 2019-05-13 12:16:19 PDT
(In reply to Mathias Bynens from comment #13)
> (In reply to Ross Kirsling from comment #11)
> > *** Bug 196332 has been marked as a duplicate of this bug. ***
> 
> Slightly off-topic, but why is it that whenever I file a WebKit bug, it
> ultimately gets closed as a duplicate of another bug that was filed *after*
> my initial report? This happened more than a few times already. Should I
> just give up?

No, you should not give up.

We're not super picky about marking the first filed bug as the "original" bug. We usually just pick whichever bug is most convenient.
Comment 15 Keith Miller 2019-05-13 12:59:35 PDT
(In reply to Saam Barati from comment #14)
> (In reply to Mathias Bynens from comment #13)
> > (In reply to Ross Kirsling from comment #11)
> > > *** Bug 196332 has been marked as a duplicate of this bug. ***
> > 
> > Slightly off-topic, but why is it that whenever I file a WebKit bug, it
> > ultimately gets closed as a duplicate of another bug that was filed *after*
> > my initial report? This happened more than a few times already. Should I
> > just give up?
> 
> No, you should not give up.
> 
> We're not super picky about marking the first filed bug as the "original"
> bug. We usually just pick whichever bug is most convenient.

I personally just use whichever bug I see first. I also sometimes forget to associate the bug with my patch so I end up creating a new bug and duping the old one.
Comment 16 Dean Jackson 2019-05-16 10:29:46 PDT
Sorry Matthias. This was my fault. I didn't search for bugs before filing.
Comment 17 Dean Jackson 2019-05-28 15:17:57 PDT
Created attachment 370789 [details]
Patch
Comment 18 Keith Miller 2019-05-28 20:29:56 PDT
Comment on attachment 370789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370789&action=review

r=me.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:150
> +            promiseCapability.@resolve.@call(@undefined, values);

Why does this need to be passed undefined as the this value? Isn't @resolve an internal method? Seems like the this value shouldn't matter.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:152
> +        promiseCapability.@reject.@call(@undefined, error);

Ditto.

> JSTests/test262/expectations.yaml:1789
> +test/intl402/Collator/ignore-invalid-unicode-ext-values.js:
> +  default: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'
> +  strict mode: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'

What? Does this test use promise.allSettled?

Was this already failing?

> JSTests/test262/expectations.yaml:1819
> +test/intl402/fallback-locales-are-supported.js:
> +  default: "Test262Error: Locale zh-Hans-CN is supported, but fallback zh-Hans isn't. Expected SameValue(ë-1û, ë-1û) to be false (Testing with Collator.)"
> +  strict mode: "Test262Error: Locale zh-Hans-CN is supported, but fallback zh-Hans isn't. Expected SameValue(ë-1û, ë-1û) to be false (Testing with Collator.)"

Ditto...
Comment 19 Dean Jackson 2019-05-29 14:15:41 PDT
Comment on attachment 370789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370789&action=review

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:150
>> +            promiseCapability.@resolve.@call(@undefined, values);
> 
> Why does this need to be passed undefined as the this value? Isn't @resolve an internal method? Seems like the this value shouldn't matter.

From https://tc39.github.io/proposal-promise-allSettled/

14. If remainingElementsCount.[[Value]] is 0, then
    14.a Let valuesArray be ! CreateArrayFromList(values).
    14.b Return ? Call(promiseCapability.[[Resolve]], undefined, « valuesArray »).

I don't understand why :)

>> JSTests/test262/expectations.yaml:1789
>> +  strict mode: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'
> 
> What? Does this test use promise.allSettled?
> 
> Was this already failing?

I think it might be a NMOS thing.

Ooooh... it's actually an en-AU thing!!!! Curse you foreigners with your non USA locales!
Comment 20 Dean Jackson 2019-05-29 14:16:29 PDT
Comment on attachment 370789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370789&action=review

>>> JSTests/test262/expectations.yaml:1789
>>> +  strict mode: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'
>> 
>> What? Does this test use promise.allSettled?
>> 
>> Was this already failing?
> 
> I think it might be a NMOS thing.
> 
> Ooooh... it's actually an en-AU thing!!!! Curse you foreigners with your non USA locales!

Just realised that I forgot to add a :)
Comment 21 Dean Jackson 2019-05-29 14:28:26 PDT
Committed r245869: <https://trac.webkit.org/changeset/245869>
Comment 22 Keith Miller 2019-05-29 14:57:29 PDT
Comment on attachment 370789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370789&action=review

>>>> JSTests/test262/expectations.yaml:1789
>>>> +  strict mode: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'
>>> 
>>> What? Does this test use promise.allSettled?
>>> 
>>> Was this already failing?
>> 
>> I think it might be a NMOS thing.
>> 
>> Ooooh... it's actually an en-AU thing!!!! Curse you foreigners with your non USA locales!
> 
> Just realised that I forgot to add a :)

How dare you have the audacity to test an Internationalization API with a non-USA system locale!

In all seriousness though this seems like an unfortunate bug... It's kinda weird that these results would depend on the locale though.