Closed Bug 1368382 Opened 7 years ago Closed 7 years ago

Move ThenValueBase::mCompletionPromise down the class hierarchy

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(4 files)

Bug 1368382. P1 - rename and make MethodThenValue/FunctionValue specializations of ThenValue<>.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1368382. P2 - let ThenCommand reference the sub-type of ThenValueBase.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1368382. P3 - remove 2 overloads of Then() using variadic template.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1368382. P4 - move mCompletionPromise down the class hierarchy so it can store a different promise type.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details 
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>.
Assignee: nobody → jwwang
Blocks: 1367679
Attachment #8873309 - Flags: review?(gsquelart)
Attachment #8873310 - Flags: review?(gsquelart)
Attachment #8873311 - Flags: review?(gsquelart)
Attachment #8873312 - Flags: review?(gsquelart)
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.
Attachment #8873309 - Flags: review?(gsquelart) → review+
Comment on attachment 8873310 [details]
Bug 1368382. P2 - let ThenCommand reference the sub-type of ThenValueBase.

https://reviewboard.mozilla.org/r/144762/#review148650
Attachment #8873310 - Flags: review?(gsquelart) → review+
Comment on attachment 8873311 [details]
Bug 1368382. P3 - remove 2 overloads of Then() using variadic template.

https://reviewboard.mozilla.org/r/144764/#review148652
Attachment #8873311 - Flags: review?(gsquelart) → review+
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
Attachment #8873312 - Flags: review?(gsquelart) → review+
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().
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: