1322964 - Add single-function MozPromise::Then

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

MozPromise::Then and ThenPromise currently take two functions: One when the promise is resolved, another one when the promise is rejected.

This is sometimes unnecessary, if both functions should be exactly the same, or one will never actually be called (e.g., because the MozPromise can only be resolved).
More importantly, for function objects (e.g., lambdas), we have two separate captures, which can prevent some use cases, like capturing a UniquePtr that could be used from either functions.

So I propose that we add Then&ThenPromise that only take one function. And this function will take a MozPromise::ResolveOrRejectValue, which contains both the type of call (resolution or rejection) and the associated value.

With help from bug 1322962, a useful example can be implemented in GeckoMediaPluginServiceParent::GetContentParent, which currently passes a raw pointer to be later used in the "winning" function. (With the risk of leak if the Then was disconnected early.)

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1322964 - MozPromise clean-up -

Removed 'virtual' from overrides, as per coding guidelines.
Fixed some incorrect indentations.

Review commit: https://reviewboard.mozilla.org/r/98246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98246/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1322964 - MozPromise.Then() taking only one resolve+reject function -

Then and ThenPromise can now be given only one function object, which takes a
`const MozPromise::ResolveOrRejectValue&`.

Review commit: https://reviewboard.mozilla.org/r/98248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98248/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1322964 - MozPromise.Then() taking only one resolve+reject method -

Then and ThenPromise can now be given only one member function, which takes a
`const MozPromise::ResolveOrRejectValue&`.

Review commit: https://reviewboard.mozilla.org/r/98250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98250/

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1322964 - Use Then with single lambda in GeckoMediaPluginServiceParent::GetContentParent -

Instead of capturing a raw pointer in two lambdas, we can now capture a safer
UniquePtr (inside a UniquePtrForCpp11Lambda for now) inside the one lambda we
chain to the initialization promise.

Review commit: https://reviewboard.mozilla.org/r/98252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98252/

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8818136 [details]
Bug 1322964 - Use Then with single lambda in GeckoMediaPluginServiceParent::GetContentParent -

https://reviewboard.mozilla.org/r/98252/#review98450

Thanks, I think this is an improvement.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8818133 [details]
Bug 1322964 - MozPromise clean-up -

https://reviewboard.mozilla.org/r/98246/#review98528

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I'll wait for bug 1323155 and bug 1321744, as they will probably impact my MozPromises changes.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818133 [details]
Bug 1322964 - MozPromise clean-up -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98246/diff/1-2/

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818134 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject function -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98248/diff/1-2/

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818135 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject method -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98250/diff/1-2/

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818136 [details]
Bug 1322964 - Use Then with single lambda in GeckoMediaPluginServiceParent::GetContentParent -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98252/diff/1-2/

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818133 [details]
Bug 1322964 - MozPromise clean-up -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98246/diff/2-3/

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818134 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject function -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98248/diff/2-3/

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818135 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject method -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98250/diff/2-3/

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818136 [details]
Bug 1322964 - Use Then with single lambda in GeckoMediaPluginServiceParent::GetContentParent -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98252/diff/2-3/

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8818134 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject function -

https://reviewboard.mozilla.org/r/98248/#review102386

::: dom/media/gtest/TestMozPromise.cpp:119
(Diff revision 3)
>  
> +TEST(MozPromise, BasicResolveOrRejectResolved)
> +{
> +  AutoTaskQueue atq;
> +  RefPtr<TaskQueue> queue = atq.Queue();
> +  RunOnTaskQueue(queue, [queue] () -> void {

You can omit the trailing return type.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8818135 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject method -

https://reviewboard.mozilla.org/r/98250/#review102388

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818134 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject function -

https://reviewboard.mozilla.org/r/98248/#review102386

> You can omit the trailing return type.

Agreed, normally I would have done without it.
But I'm being consistent with the existing style in this file, where all lambdas specify their return type.

Maybe the next person to modify TestMozPromise.cpp could remove all of them in a separate "clean-up" patch. ;-)

Comment 19

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818133 [details]
Bug 1322964 - MozPromise clean-up -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98246/diff/3-4/

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818134 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject function -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98248/diff/3-4/

Comment 21

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8818135 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject method -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98250/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84c08dfe219e
MozPromise clean-up - r=jwwang
https://hg.mozilla.org/integration/autoland/rev/11c458a99a6d
MozPromise.Then() taking only one resolve+reject function - r=jwwang
https://hg.mozilla.org/integration/autoland/rev/3111403f3ac2
MozPromise.Then() taking only one resolve+reject method - r=jwwang

Comment 23

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Removed the last part (use of 1-arg Then() in GMPServiceParent), as it depended on bug 1322962 which won't happen.
Also it was not stricly related to this bug. I'll open a separate bug to implement that once C++14 is available...

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84c08dfe219e
https://hg.mozilla.org/mozilla-central/rev/11c458a99a6d
https://hg.mozilla.org/mozilla-central/rev/3111403f3ac2