Closed Bug 1321744 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(4 files)

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: 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.
Sure. The assertion will still be reserved.
Attachment #8816958 - Flags: review?(bobbyholley)
Attachment #8816959 - Flags: review?(bobbyholley)
Attachment #8816960 - Flags: review?(bobbyholley)
Attachment #8816958 - Flags: review?(bobbyholley) → review?(gsquelart)
Attachment #8816959 - Flags: review?(bobbyholley) → review?(gsquelart)
Attachment #8816960 - Flags: review?(bobbyholley) → review?(gsquelart)
Hope Gerald has more cycles for 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 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 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+
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 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.
Attachment #8818435 - Flags: review?(gsquelart)
(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 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+
Thanks for the review!
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
Depends on: 1371982
You need to log in before you can comment on or make changes to this bug.