Unify MozPromise::ThenPromise() and MozPromise::Then()

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
We want the syntax to be as close to the JS counterpart as possible so we can do:

RefPtr<SomePromise> promise;
MozPromiseRequestHolder<SomePromise> requestHolder;
requestHolder.Begin(promise->Then()->Then());
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Depends on: 1321471
Priority: -- → P3
Please be sure to think carefully about the interaction between this and disconnection. Right now we can make strong guarantees that a promise callback is either dispatched or the RequestHolder has been disconnected (indicating that the caller doesn't care). We should make sure this property is preserved.
(Assignee)

Comment 2

2 years ago
Sure. The assertion will still be reserved.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8816958 - Flags: review?(bobbyholley)
Attachment #8816959 - Flags: review?(bobbyholley)
Attachment #8816960 - Flags: review?(bobbyholley)
(Assignee)

Updated

2 years ago
Attachment #8816958 - Flags: review?(bobbyholley) → review?(gsquelart)
Attachment #8816959 - Flags: review?(bobbyholley) → review?(gsquelart)
Attachment #8816960 - Flags: review?(bobbyholley) → review?(gsquelart)
(Assignee)

Comment 6

2 years ago
Hope Gerald has more cycles for review.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1322964

Comment 10

2 years ago
mozreview-review
Comment on attachment 8816958 [details]
Bug 1321744. Part 1 - re-implement  MozPromise::Then() using the command pattern.

https://reviewboard.mozilla.org/r/97442/#review98708
Attachment #8816958 - Flags: review?(gsquelart) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8816959 [details]
Bug 1321744. Part 2 - add the ability of promise chaining to Then().

https://reviewboard.mozilla.org/r/97444/#review98710
Attachment #8816959 - Flags: review?(gsquelart) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8816960 [details]
Bug 1321744. Part 3 - remove ThenPromise and replace its use with Then.

https://reviewboard.mozilla.org/r/97446/#review98712

Tentative r+, looks good to me...
But your latest Try shows a few failures, can you please ensure that they are not due to this code?
And the main documentation should be updated, thank you.

::: xpcom/threads/MozPromise.h:80
(Diff revision 2)
>   * A MozPromise is ThreadSafe, and may be ->Then()ed on any thread. The Then()
>   * call accepts resolve and reject callbacks, and returns a MozPromise::Request.
>   * The Request object serves several purposes for the consumer.

I think this documentation needs updating to reflect the new reality of what ->Then() returns.
Attachment #8816960 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8816960 [details]
Bug 1321744. Part 3 - remove ThenPromise and replace its use with Then.

https://reviewboard.mozilla.org/r/97446/#review98712

Which revision shows Try failures?
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8816960 [details]
Bug 1321744. Part 3 - remove ThenPromise and replace its use with Then.

https://reviewboard.mozilla.org/r/97446/#review98712

> I think this documentation needs updating to reflect the new reality of what ->Then() returns.

P4 to fix the comments.
(Assignee)

Updated

2 years ago
Attachment #8818435 - Flags: review?(gsquelart)
(Assignee)

Comment 17

2 years ago
(In reply to Gerald Squelart [:gerald] from comment #16)
> Comment on attachment 8816960 [details]
> Bug 1321744. Part 3 - remove ThenPromise and replace its use with Then.
> 
> https://reviewboard.mozilla.org/r/97446/#review98712
> 
> I was looking at this one:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b81deb55571e52af2d19ac564b46e8b973620dcb

The oranges are caused by applying the patches of bug 1319992. :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7b8090d80e5ebf4c53e2e2254ffed7213761331
This is my working revision.

Comment 18

2 years ago
mozreview-review
Comment on attachment 8818435 [details]
Bug 1321744. Part 4 - fix comments.

https://reviewboard.mozilla.org/r/98486/#review98758

"Magic object", eh? :-)
I can't think of a better word anyway.

Thank you for this update.
Attachment #8818435 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 19

2 years ago
Thanks for the review!

Comment 20

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c68bbe05ad5
Part 1 - re-implement  MozPromise::Then() using the command pattern. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6c0acb9b43c3
Part 2 - add the ability of promise chaining to Then(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/79db424875d8
Part 3 - remove ThenPromise and replace its use with Then. r=gerald
https://hg.mozilla.org/integration/autoland/rev/2f1c20022097
Part 4 - fix comments. r=gerald

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c68bbe05ad5
https://hg.mozilla.org/mozilla-central/rev/6c0acb9b43c3
https://hg.mozilla.org/mozilla-central/rev/79db424875d8
https://hg.mozilla.org/mozilla-central/rev/2f1c20022097
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

2 years ago
Depends on: 1371982
You need to log in before you can comment on or make changes to this bug.