1368382 - Move ThenValueBase::mCompletionPromise down the class hierarchy

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

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

http://searchfox.org/mozilla-central/rev/596d188f6dbc8cb023e625f0a4310d184875f8fc/xpcom/threads/MozPromise.h#488

To support bug 1367679, we need to store a completion promise of different type. By moving mCompletionPromise down the class hierarchy, the sub-class will be able to decide its type depending on the return value of resolve/reject callbacks.

E.g.

MyPromise1::CreateAndResolve(...)
->Then(...,
       []() { return MyPromise2::CreateAndResolve(...); }
       []() { return MyPromise2::CreateAndReject(...); });

mCompletionPromise will store RefPtr<MyPromise2::Private> instead of RefPtr<MyPromise1::Private>.

Comment 1

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

Attachment: Bug 1368382. P1 - rename and make MethodThenValue/FunctionValue specializations of ThenValue<>.

This allows us to remove 2 overloads of MozPromise::Then() using variadic template.

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

Comment 2

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

Attachment: Bug 1368382. P2 - let ThenCommand reference the sub-type of ThenValueBase.

Since we will store mCompletionPromise in the sub-class of ThenValueBase,
ThenCommand needs to reference the sub-type in order to access mCompletionPromise.

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

Comment 3

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

Attachment: Bug 1368382. P3 - remove 2 overloads of Then() using variadic template.

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

Comment 4

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

Attachment: Bug 1368382. P4 - move mCompletionPromise down the class hierarchy so it can store a different promise type.

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

Comment 5

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

Comment on attachment 8873309 [details]
Bug 1368382. P1 - rename and make MethodThenValue/FunctionValue specializations of ThenValue<>.

https://reviewboard.mozilla.org/r/144760/#review148644

::: xpcom/threads/MozPromise.h:605
(Diff revision 1)
>      RefPtr<ThisType> mThisVal; // Only accessed and refcounted on dispatch thread.
>      ResolveMethodType mResolveMethod;
>      RejectMethodType mRejectMethod;
>    };
>  
>    // Specialization of MethodThenValue (with 3rd template arg being 'void')

"(with 3rd template arg being 'void')" should be removed.

::: xpcom/threads/MozPromise.h:608
(Diff revision 1)
>    };
>  
>    // Specialization of MethodThenValue (with 3rd template arg being 'void')
>    // that only takes one method, to be called with a ResolveOrRejectValue.
>    template<typename ThisType, typename ResolveRejectMethodType>
> -  class MethodThenValue<ThisType, ResolveRejectMethodType, void> : public ThenValueBase
> +  class ThenValue<ThisType*, ResolveRejectMethodType> : public ThenValueBase

... (see next comment)

::: xpcom/threads/MozPromise.h:652
(Diff revision 1)
>      ResolveRejectMethodType mResolveRejectMethod;
>    };
>  
>    // NB: We could use std::function here instead of a template if it were supported. :-(
>    template<typename ResolveFunction, typename RejectFunction>
> -  class FunctionThenValue : public ThenValueBase
> +  class ThenValue<ResolveFunction, RejectFunction> : public ThenValueBase

We have two specializations of Then that take two types, and the only difference is with the first type being a pointer or not...
So this prevents using function pointers as 'ResolveFunction'!

So far we've only used lambdas, so that's probably why it builds.

I'll still r+ this, as I think it's unlikely we will need function pointers, and a workaround would just be to enclose their call in a lambda.
I think a comment explaining this constraint would be nice, to show that we've thought about it.

::: xpcom/threads/MozPromise.h:709
(Diff revision 1)
>    private:
>      Maybe<ResolveFunction> mResolveFunction; // Only accessed and deleted on dispatch thread.
>      Maybe<RejectFunction> mRejectFunction; // Only accessed and deleted on dispatch thread.
>    };
>  
>    // Specialization of FunctionThenValue (with 2nd template arg being 'void')

"(with 2nd template arg being 'void')" should be removed too.

Comment 6

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

Comment on attachment 8873310 [details]
Bug 1368382. P2 - let ThenCommand reference the sub-type of ThenValueBase.

https://reviewboard.mozilla.org/r/144762/#review148650

Comment 7

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

Comment on attachment 8873311 [details]
Bug 1368382. P3 - remove 2 overloads of Then() using variadic template.

https://reviewboard.mozilla.org/r/144764/#review148652

Comment 8

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

Comment on attachment 8873312 [details]
Bug 1368382. P4 - move mCompletionPromise down the class hierarchy so it can store a different promise type.

https://reviewboard.mozilla.org/r/144766/#review148654

Comment 9

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

Comment on attachment 8873309 [details]
Bug 1368382. P1 - rename and make MethodThenValue/FunctionValue specializations of ThenValue<>.

https://reviewboard.mozilla.org/r/144760/#review148644

> We have two specializations of Then that take two types, and the only difference is with the first type being a pointer or not...
> So this prevents using function pointers as 'ResolveFunction'!
> 
> So far we've only used lambdas, so that's probably why it builds.
> 
> I'll still r+ this, as I think it's unlikely we will need function pointers, and a workaround would just be to enclose their call in a lambda.
> I think a comment explaining this constraint would be nice, to show that we've thought about it.

'Concepts' is not available yet to constraint the type accepted by the template. I will open a new bug to add some static assertions with comprehensive messages if someone accidentally passes function pointers to Then().

Comment 10

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

Comment on attachment 8873309 [details]
Bug 1368382. P1 - rename and make MethodThenValue/FunctionValue specializations of ThenValue<>.

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

Comment 11

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

Comment on attachment 8873310 [details]
Bug 1368382. P2 - let ThenCommand reference the sub-type of ThenValueBase.

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

Comment 12

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

Comment on attachment 8873311 [details]
Bug 1368382. P3 - remove 2 overloads of Then() using variadic template.

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

Comment 13

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

Comment on attachment 8873312 [details]
Bug 1368382. P4 - move mCompletionPromise down the class hierarchy so it can store a different promise type.

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

Comment 14

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

Thanks for the review!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f143a22591fa
P1 - rename and make MethodThenValue/FunctionValue specializations of ThenValue<>. r=gerald
https://hg.mozilla.org/integration/autoland/rev/a6df0f5e3000
P2 - let ThenCommand reference the sub-type of ThenValueBase. r=gerald
https://hg.mozilla.org/integration/autoland/rev/aa9719398028
P3 - remove 2 overloads of Then() using variadic template. r=gerald
https://hg.mozilla.org/integration/autoland/rev/d96110d76619
P4 - move mCompletionPromise down the class hierarchy so it can store a different promise type. r=gerald

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f143a22591fa
https://hg.mozilla.org/mozilla-central/rev/a6df0f5e3000
https://hg.mozilla.org/mozilla-central/rev/aa9719398028
https://hg.mozilla.org/mozilla-central/rev/d96110d76619