Closed Bug 1322964 Opened 5 years ago Closed 5 years ago

Add single-function MozPromise::Then

Categories

(Core :: XPCOM, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

Details

Attachments

(3 files, 1 obsolete file)

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 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.
Attachment #8818136 - Flags: review?(cpearce) → review+
Comment on attachment 8818133 [details]
Bug 1322964 - MozPromise clean-up -

https://reviewboard.mozilla.org/r/98246/#review98528
Attachment #8818133 - Flags: review?(jwwang) → review+
Attachment #8818134 - Flags: review?(jwwang)
Attachment #8818135 - Flags: review?(jwwang)
I'll wait for bug 1323155 and bug 1321744, as they will probably impact my MozPromises changes.
Depends on: 1323155, 1321744
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.
Attachment #8818134 - Flags: review?(jwwang) → review+
Comment on attachment 8818135 [details]
Bug 1322964 - MozPromise.Then() taking only one resolve+reject method -

https://reviewboard.mozilla.org/r/98250/#review102388
Attachment #8818135 - Flags: review?(jwwang) → review+
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. ;-)
Attachment #8818136 - Attachment is obsolete: true
Pushed by gsquelart@mozilla.com:
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
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...
No longer depends on: 1322962
Blocks: 1329385
https://hg.mozilla.org/mozilla-central/rev/84c08dfe219e
https://hg.mozilla.org/mozilla-central/rev/11c458a99a6d
https://hg.mozilla.org/mozilla-central/rev/3111403f3ac2
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.