New issue
Advanced search Search tips

Issue 912848 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 902633



Sign in to add a comment

Make v8::Promise::Then two functions, not one

Project Member Reported by yhirano@chromium.org, Dec 7

Issue description

The current interface is:

  V8_WARN_UNUSED_RESULT MaybeLocal<Promise> Then(Local<Context> context,
                                                 Local<Function> handler);

We should have two handlers, like the spec.

  Promise.prototype.then ( onFulfilled , onRejected )




 
You could use Promise::Catch. Admittedly that allocates one more Promise object though, if you chain ::Then and ::Catch.
The semantics of chaining Then and Catch are different from the two-argument form of then. If the onFulfilled function throws on purpose then getting the semantics to match would be tricky.
That's a good point. How about chaining ::Catch and ::Then in that order?
I'm concerned about extra microtasks. Streams API is fairly imperative and explicit, so I want to avoid extra microtasks caused by extra calls.


To emurate p.then(f,g):

q = p.catch(g).then(f) doesn't work because |f| will get the result of |g| when |p| is rejected.

p.catch(g); q = p.then(f) doesn't work because |q| will always be rejected when |p| is rejected.
sgtm. let's do this :)
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 13

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/228b362a5250966b1526b307daaf268504efb621

commit 228b362a5250966b1526b307daaf268504efb621
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Thu Dec 13 08:41:55 2018

Introduce v8::Promise::Then(context, on_fulfilled, on_rejected)

Currently v8::Promise::Then takes only one handler. It should take two handlers,
on_fulfilled and on_rejected like ECMAScript Promise.

Bug:  chromium:912848 
Change-Id: I08a20990a27b3f8621225fad42a8de1dad67796f
Reviewed-on: https://chromium-review.googlesource.com/c/1375509
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58204}
[modify] https://crrev.com/228b362a5250966b1526b307daaf268504efb621/include/v8.h
[modify] https://crrev.com/228b362a5250966b1526b307daaf268504efb621/src/api.cc
[modify] https://crrev.com/228b362a5250966b1526b307daaf268504efb621/test/cctest/test-api.cc

Status: Fixed (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de8815bed5463b9d59d976b4c8b676c223dfbee8

commit de8815bed5463b9d59d976b4c8b676c223dfbee8
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Tue Dec 18 08:50:34 2018

Fix ScriptPromise::Then

ScriptPromise::Then(on_fulfilled, on_rejected) is defined as

promise.Then(on_fulfilled).Catch(on_rejected)

but it is not what developers would expect. When |on_fulfilled| throws,
|on_rejected| will be called with the current implementation. Fix it.

Bug:  912848 
Change-Id: Ie736f70f7d3570987767c4a11dd5744513157a53
Reviewed-on: https://chromium-review.googlesource.com/c/1369563
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617420}
[modify] https://crrev.com/de8815bed5463b9d59d976b4c8b676c223dfbee8/third_party/blink/renderer/bindings/core/v8/script_promise.cc
[modify] https://crrev.com/de8815bed5463b9d59d976b4c8b676c223dfbee8/third_party/blink/renderer/bindings/core/v8/script_promise.h
[modify] https://crrev.com/de8815bed5463b9d59d976b4c8b676c223dfbee8/third_party/blink/renderer/bindings/core/v8/script_promise_test.cc
[modify] https://crrev.com/de8815bed5463b9d59d976b4c8b676c223dfbee8/third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56df86d96488cf5646f470349f95818f250a0b69

commit 56df86d96488cf5646f470349f95818f250a0b69
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Dec 27 05:48:14 2018

service worker: Remove workaround for old ScriptPromise::Then behavior.

This reverts r604535 which is no longer needed after r617420, as
now throwing in the fulfilled functions no longer invokes the
rejected function.

Bug:  912848 
Change-Id: I06951e9d9b2909fbad627e81efbb945ae92bc533
Reviewed-on: https://chromium-review.googlesource.com/c/1390779
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619009}
[modify] https://crrev.com/56df86d96488cf5646f470349f95818f250a0b69/third_party/blink/renderer/modules/service_worker/wait_until_observer.cc

Sign in to add a comment