Bug 174503 - [ES] Add support finally to Promise
Summary: [ES] Add support finally to Promise
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: GSkachkov
URL:
Keywords: InRadar
Depends on:
Blocks: 174944 175015
  Show dependency treegraph
 
Reported: 2017-07-14 07:09 PDT by GSkachkov
Modified: 2017-08-01 11:19 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.83 KB, patch)
2017-07-14 07:15 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (9.47 KB, patch)
2017-07-17 13:27 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.07 MB, application/zip)
2017-07-17 14:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-elcapitan (1.21 MB, application/zip)
2017-07-17 14:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (2.13 MB, application/zip)
2017-07-17 14:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (962.45 KB, application/zip)
2017-07-17 14:56 PDT, Build Bot
no flags Details
Patch (12.82 KB, patch)
2017-07-26 14:30 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (15.20 KB, patch)
2017-07-27 12:18 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (12.98 MB, application/zip)
2017-07-27 14:02 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2017-07-14 07:09:07 PDT
There is a spec in progress https://tc39.github.io/proposal-promise-finally/#sec-promise-resolve  that add finally method to the Promise.
It exist in q and bluebird promise. I believe that it will be pass step 3, because it very useful feature.
Comment 1 GSkachkov 2017-07-14 07:15:18 PDT
Created attachment 315419 [details]
Patch

worked WIP, without tests
Comment 2 Jordan Harband 2017-07-14 10:39:35 PDT
Comment on attachment 315419 [details]
Patch

Is `function finally() {}` (the name "finally") actually allowed in webkit's internal JS? It's a syntax error in normal JS.

In my polyfill, I had to use a concise method in an object, and then extract it from the object.


I note you're calling `promise.@then` in the catchFinally and thenFinally functions - this seems like it's the internal then method, but the spec explicitly requires it be an observable call to `.then` on the `promise` value.
Comment 3 GSkachkov 2017-07-14 11:23:35 PDT
(In reply to Jordan Harband from comment #2)
> Comment on attachment 315419 [details]
> Patch
> 
> Is `function finally() {}` (the name "finally") actually allowed in webkit's
> internal JS? It's a syntax error in normal JS.
It is correct name for builtin (In generator we have `return` and `throw` :) ). I checked with following script:
```
Promise.resolve(1).finally(v=>print('finally:', v));
Promise.reject(1).finally(v=>print('finally:', v));
Promise.resolve(1).then(v=>{ print('fulfill:', v); return 'result'}).finally(v=>print('finally:', v));
```
> 
> In my polyfill, I had to use a concise method in an object, and then extract
> it from the object.
> 
> 
> I note you're calling `promise.@then` in the catchFinally and thenFinally
> functions - this seems like it's the internal then method, but the spec
> explicitly requires it be an observable call to `.then` on the `promise`
> value.
OK. I'll fix this soon.

Thanks for feedback!
Comment 4 GSkachkov 2017-07-17 13:27:33 PDT
Created attachment 315702 [details]
Patch

WiP + several simple tests
Comment 5 Jordan Harband 2017-07-17 14:01:17 PDT
I note that you're throwing TypeErrors inside the thenFinally/catchFinally function when `onFinally` isn't a function - however, that's an assertion, which shouldn't ever fail - I suspect Webkit has a notation for that?

Otherwise I think it looks good!
Comment 6 Build Bot 2017-07-17 14:10:42 PDT
Comment on attachment 315702 [details]
Patch

Attachment 315702 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4137345

New failing tests:
js/Promise-types.html
Comment 7 Build Bot 2017-07-17 14:10:43 PDT
Created attachment 315710 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-07-17 14:11:01 PDT
Comment on attachment 315702 [details]
Patch

Attachment 315702 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4137351

New failing tests:
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-no-ftl
Comment 9 Build Bot 2017-07-17 14:13:05 PDT
Comment on attachment 315702 [details]
Patch

Attachment 315702 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4137364

New failing tests:
security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html
js/Promise-types.html
Comment 10 Build Bot 2017-07-17 14:13:06 PDT
Created attachment 315711 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-07-17 14:33:35 PDT
Comment on attachment 315702 [details]
Patch

Attachment 315702 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4137377

New failing tests:
js/Promise-types.html
Comment 12 Build Bot 2017-07-17 14:33:37 PDT
Created attachment 315715 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 13 Build Bot 2017-07-17 14:55:59 PDT
Comment on attachment 315702 [details]
Patch

Attachment 315702 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4137426

New failing tests:
js/Promise-types.html
Comment 14 Build Bot 2017-07-17 14:56:00 PDT
Created attachment 315718 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 15 GSkachkov 2017-07-26 14:30:27 PDT
Created attachment 316481 [details]
Patch

Fix failed test & add more tests
Comment 16 Yusuke Suzuki 2017-07-27 01:34:31 PDT
Comment on attachment 316481 [details]
Patch

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

> Source/JavaScriptCore/builtins/PromisePrototype.js:100
> +        if (!@isConstructor(constructor))
> +            @throwTypeError("property is not a constructor");

According to the sepc, this should be `@assert(@isConstructor(constructor))`.

> Source/JavaScriptCore/builtins/PromisePrototype.js:109
> +        promise.then(valueThunk);

Should return `promise.then(valueThunk);`. And, could you add a test for this?

> Source/JavaScriptCore/builtins/PromisePrototype.js:124
> +        if (!@isConstructor(constructor))
> +            @throwTypeError("property is not a constructor");

According to the sepc, this should be `@assert(@isConstructor(constructor))`.

> Source/JavaScriptCore/builtins/PromisePrototype.js:133
> +        promise.then(thrower);

Should return `promise.then(thrower);`. And, could you add a test for this?
Comment 17 Yusuke Suzuki 2017-07-27 01:59:13 PDT
Comment on attachment 316481 [details]
Patch

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

Overall, looks good. I can r+ once the problems are fixed and tests are added :)

> Source/JavaScriptCore/builtins/PromisePrototype.js:97
> +        if (typeof onFinally !== "function")
> +            @throwTypeError("onFinally is not a function");

This also should be `@assert(typeof onFinally === "function")`.

> Source/JavaScriptCore/builtins/PromisePrototype.js:121
> +        if (typeof onFinally !== "function")
> +            @throwTypeError("onFinally is not a function");

This also should be `@assert(typeof onFinally === "function")`.
Comment 18 GSkachkov 2017-07-27 12:18:12 PDT
Created attachment 316563 [details]
Patch

Fix comments
Comment 19 GSkachkov 2017-07-27 12:19:57 PDT
Comment on attachment 316481 [details]
Patch

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

>> Source/JavaScriptCore/builtins/PromisePrototype.js:97
>> +            @throwTypeError("onFinally is not a function");
> 
> This also should be `@assert(typeof onFinally === "function")`.

Fixed

>> Source/JavaScriptCore/builtins/PromisePrototype.js:100
>> +            @throwTypeError("property is not a constructor");
> 
> According to the sepc, this should be `@assert(@isConstructor(constructor))`.

Fixed

>> Source/JavaScriptCore/builtins/PromisePrototype.js:109
>> +        promise.then(valueThunk);
> 
> Should return `promise.then(valueThunk);`. And, could you add a test for this?

Fixed

>> Source/JavaScriptCore/builtins/PromisePrototype.js:121
>> +            @throwTypeError("onFinally is not a function");
> 
> This also should be `@assert(typeof onFinally === "function")`.

Fixed

>> Source/JavaScriptCore/builtins/PromisePrototype.js:124
>> +            @throwTypeError("property is not a constructor");
> 
> According to the sepc, this should be `@assert(@isConstructor(constructor))`.

Fixed

>> Source/JavaScriptCore/builtins/PromisePrototype.js:133
>> +        promise.then(thrower);
> 
> Should return `promise.then(thrower);`. And, could you add a test for this?

Fixed
Comment 20 Build Bot 2017-07-27 14:02:45 PDT
Comment on attachment 316563 [details]
Patch

Attachment 316563 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4198056

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open.html
Comment 21 Build Bot 2017-07-27 14:02:47 PDT
Created attachment 316568 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 22 Yusuke Suzuki 2017-07-27 19:09:48 PDT
Comment on attachment 316563 [details]
Patch

r=me
Comment 23 WebKit Commit Bot 2017-07-27 19:38:44 PDT
Comment on attachment 316563 [details]
Patch

Clearing flags on attachment: 316563

Committed r219989: <http://trac.webkit.org/changeset/219989>
Comment 24 WebKit Commit Bot 2017-07-27 19:38:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2017-07-28 16:38:34 PDT
<rdar://problem/33601718>
Comment 26 Jordan Harband 2017-08-01 11:16:00 PDT
It'd be great if anyone on this thread could take a look at https://github.com/tc39/test262/pull/1156 and compare my test262 tests with Webkit's tests :-) Any feedback is helpful!
Comment 27 GSkachkov 2017-08-01 11:19:50 PDT
(In reply to Jordan Harband from comment #26)
> It'd be great if anyone on this thread could take a look at
> https://github.com/tc39/test262/pull/1156 and compare my test262 tests with
> Webkit's tests :-) Any feedback is helpful!

I'll look :-)