Closed Bug 1321250 Opened 4 years ago Closed 4 years ago

Add MozPromise::ThenPromise() for easier promise chaining

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

We would like to add a ThenPromise() function which is equivalent to Then(target, ...)->CompletionPromise() without the restriction that CompletionPromise() must be called on the |target| thread. So ThenPromise() can be called on any thread as Then().

The syntax is close to JS promise and makes promise chaining easier where you can do: p->ThenPromise()->ThenPromise()->ThenPromise().
Assignee: nobody → jwwang
Priority: -- → P3
Attachment #8815978 - Flags: review?(bobbyholley)
Attachment #8815978 - Flags: review?(bobbyholley) → review?(jyavenard)
Comment on attachment 8815978 [details]
Bug 1321250 - Add MozPromise::ThenPromise() for easier promise chaining.

https://reviewboard.mozilla.org/r/96742/#review96980

::: xpcom/threads/MozPromise.h:353
(Diff revision 1)
>      MozPromise* CompletionPromise() override
>      {
> -      MOZ_DIAGNOSTIC_ASSERT(mResponseTarget->IsCurrentThreadIn());
> +      MOZ_DIAGNOSTIC_ASSERT(mInitCompletionPromise ||
> +                            mResponseTarget->IsCurrentThreadIn());
>        MOZ_DIAGNOSTIC_ASSERT(!Request::mComplete);
> -      if (!mCompletionPromise) {
> +      if (!mInitCompletionPromise && !mCompletionPromise) {

Why allow CompletionPromise() after a ThenPromise at all?

it would do nothing anyway.

I think simply asserting is fine.

and later return the mCompletionPromise member directly.

::: xpcom/threads/MozPromise.h:646
(Diff revision 1)
> +  // without the restriction that CompletionPromise() must be called on the
> +  // |target| thread. So ThenPromise() can be called on any thread as Then().
> +  // The syntax is close to JS promise and makes promise chaining easier
> +  // where you can do: p->ThenPromise()->ThenPromise()->ThenPromise();
> +  //
> +  // Note you would like to call Then() instead when the result needs to be held

"you would have to call"

Doesn't really matter if you like it or not :P

::: xpcom/threads/MozPromise.h:652
(Diff revision 1)
> +  // by a MozPromiseRequestHolder for future disconnection.
> +  //
> +  // TODO: replace Then()->CompletionPromise() with ThenPromise() and
> +  // stop exposing CompletionPromise() to the client code.
> +  template<typename This, typename Resolver, typename Rejector>
> +  MOZ_MUST_USE RefPtr<MozPromise>

Why MOZ_MUST_USE ?

Returning a RefPtr indicates that it's okay to not use the result. It can simply go out of scope and be forgotten.

::: xpcom/threads/MozPromise.h:663
(Diff revision 1)
> +      aRejector, aCallSite, true /* aInitCompletionPromise */);
> +    ThenInternal(aTarget, thenValue, aCallSite);
> +    return thenValue->CompletionPromise();
> +  }
> +
> +  template<typename Resolver, typename Rejector>

Please use the same typename and arguments name as used with the plain Then for consistency. makes it harder to read otherwise.

::: xpcom/threads/MozPromise.h:672
(Diff revision 1)
> +  {
> +    using ThenType = FunctionThenValue<Resolver, Rejector>;
> +    RefPtr<ThenValueBase> thenValue = new ThenType(aTarget, Move(aResolver),
> +      Move(aRejector), aCallSite, true /* aInitCompletionPromise */);
> +    ThenInternal(aTarget, thenValue, aCallSite);
> +    return thenValue->CompletionPromise();

I would use a different way to return the CompletionPromise() that would disallow the use of CompletionPromise alongside ThenPromise.
Attachment #8815978 - Flags: review?(jyavenard) → review+
Comment on attachment 8815978 [details]
Bug 1321250 - Add MozPromise::ThenPromise() for easier promise chaining.

https://reviewboard.mozilla.org/r/96742/#review96986

::: xpcom/threads/MozPromise.h:657
(Diff revision 1)
> +    RefPtr<ThenValueBase> thenValue = new ThenType(aTarget, aThis, aResolver,
> +      aRejector, aCallSite, true /* aInitCompletionPromise */);
> +    ThenInternal(aTarget, thenValue, aCallSite);
> +    return thenValue->CompletionPromise();

Fly-by suggestion: (not an issue)

It seems MethodThenValue(..., true) is only used here.
So instead of using an extra member mInitCompletionPromise just to handle this case, why not have a private CompletionPromiseInternal() that will not check threads, to be used here only?
In this case, it should be safe to do:
  RefPtr<ThenValueBase> thenValue = ...(same as before);
  // Safe because nobody else knows about thenValue yet.
  RefPtr<Promise> p = thenValue->CompletionPromiseInternal();
  ThenInternal(...);
  return p;

(And same suggestion for the functional ThenPromise.)
Comment on attachment 8815978 [details]
Bug 1321250 - Add MozPromise::ThenPromise() for easier promise chaining.

https://reviewboard.mozilla.org/r/96742/#review96980

> Why allow CompletionPromise() after a ThenPromise at all?
> 
> it would do nothing anyway.
> 
> I think simply asserting is fine.
> 
> and later return the mCompletionPromise member directly.

https://hg.mozilla.org/try/rev/ddceb8281e9e541be149ab1b064cd19aea740a55
It will be removed in next bug.

> Why MOZ_MUST_USE ?
> 
> Returning a RefPtr indicates that it's okay to not use the result. It can simply go out of scope and be forgotten.

ThenPromise() means you want more promise chaining so the result should always be used.

> I would use a different way to return the CompletionPromise() that would disallow the use of CompletionPromise alongside ThenPromise.

https://hg.mozilla.org/try/rev/ddceb8281e9e541be149ab1b064cd19aea740a55
Will be fixed in next bug.
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fc28aab9031
Add MozPromise::ThenPromise() for easier promise chaining. r=jya
Blocks: 1321471
https://hg.mozilla.org/mozilla-central/rev/0fc28aab9031
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1323155
You need to log in before you can comment on or make changes to this bug.