Bug 174503 – [ES] Add support finally to Promise

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

[GSkachkov]

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.

[GSkachkov]

Created attachment 315419 [details]
Patch

worked WIP, without tests

[Jordan Harband]

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.

[GSkachkov]

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

[GSkachkov]

Created attachment 315702 [details]
Patch

WiP + several simple tests

[Jordan Harband]

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!

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[GSkachkov]

Created attachment 316481 [details]
Patch

Fix failed test & add more tests

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

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")`.

[GSkachkov]

Created attachment 316563 [details]
Patch

Fix comments

[GSkachkov]

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

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Comment on attachment 316563 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 316563 [details]
Patch

Clearing flags on attachment: 316563

Committed r219989: <http://trac.webkit.org/changeset/219989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33601718>

[Jordan Harband]

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!

[GSkachkov]

(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 :-)