1367679 - Add the ability to chain MozPromises of different types

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

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

It would be quite handy to be able to do:

  using Promise1 = MozPromise<int, bool, true>;
  using Promise2 = MozPromise<UniquePtr<int>, bool, true>;

  Promise1::CreateAndResolve(87, __func__)
  ->Then(queue, __func__,
    [](int aVal) {
      EXPECT_EQ(87, aVal);
      return Promise2::CreateAndResolve(MakeUnique<int>(94), __func__);
    },
    []() {
      return Promise2::CreateAndResolve(MakeUnique<int>(97), __func__);
    })
  ->Then(queue, __func__,
    [queue](UniquePtr<int> aVal) {
      EXPECT_EQ(94, *aVal);
    },
    []() {});

Comment 1

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

Attachment: Bug 1367679. P1 - refactor InvokeCallbackMethod() to deal with one concern at a time.

InvokeMethod() handles optional arguments.
InvokeCallbackMethod() handles optional return value.

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

Comment 2

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

Attachment: Bug 1367679. P2 - overload InvokeCallbackMethod() according to whether promise-chaining is supported.

This patch fix InvokeCallbackMethod() which should return null
if promise-chaining is not supported.

Before this patch, it could return non-null if one of the resolve/reject callbacks
returns a MozPromise while the other not.

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

Comment 3

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

Attachment: Bug 1367679. P3 - pass mCompletionPromise to InvokeCallbackMethod().

This reduces duplicate code by handling promise chaining in one place.

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

Comment 4

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

Attachment: Bug 1367679. P4 - specialize the type of mCompletionPromise according to whether chaining is supported.

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

Comment 5

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

Attachment: Bug 1367679. P5 - add a gtest to test chaining between different promise types.

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

Comment 6

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

Comment on attachment 8873672 [details]
Bug 1367679. P1 - refactor InvokeCallbackMethod() to deal with one concern at a time.

https://reviewboard.mozilla.org/r/145060/#review149046

Comment 7

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

Comment on attachment 8873673 [details]
Bug 1367679. P2 - overload InvokeCallbackMethod() according to whether promise-chaining is supported.

https://reviewboard.mozilla.org/r/145062/#review149048

r+ with fix, and suggestion if feasible (or happy to leave it for another bug).

::: commit-message-cbe88:3
(Diff revision 1)
> +Bug 1367679. P2 - overload InvokeCallbackMethod() according to whether promise-chaining is supported.
> +
> +This patch fix InvokeCallbackMethod() which should return null

'fix' -> 'fixes'

::: xpcom/threads/MozPromise.h:596
(Diff revision 1)
>  
>      void DoResolveOrRejectInternal(ResolveOrRejectValue& aValue) override
>      {
>        RefPtr<MozPromise> result;
>        if (aValue.IsResolve()) {
> -        result = InvokeCallbackMethod(
> +        result = InvokeCallbackMethod(SupportChaining(),

Since SupportChaining is a compile-time type (and not a runtime value), couldn't you make it a 'bool' template parameter in first position in InvokeCallbackMethod?
So the call would become `InvokeCallbackMethod<SupportChaining::value>(...)`.

This would avoid constructing a SupportChaining object (which may take a register/stack space) just to select which function to use
(Though it's quite possible it's all optimized away at compile time?)

Comment 8

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

Comment on attachment 8873674 [details]
Bug 1367679. P3 - pass mCompletionPromise to InvokeCallbackMethod().

https://reviewboard.mozilla.org/r/145064/#review149052

Comment 9

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

Comment on attachment 8873675 [details]
Bug 1367679. P4 - specialize the type of mCompletionPromise according to whether chaining is supported.

https://reviewboard.mozilla.org/r/145066/#review149060

Comment 10

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

Comment on attachment 8873676 [details]
Bug 1367679. P5 - add a gtest to test chaining between different promise types.

https://reviewboard.mozilla.org/r/145068/#review149062

Comment 11

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

Comment on attachment 8873673 [details]
Bug 1367679. P2 - overload InvokeCallbackMethod() according to whether promise-chaining is supported.

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

Comment 12

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

Comment on attachment 8873674 [details]
Bug 1367679. P3 - pass mCompletionPromise to InvokeCallbackMethod().

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

Comment 13

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

Comment on attachment 8873675 [details]
Bug 1367679. P4 - specialize the type of mCompletionPromise according to whether chaining is supported.

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

Comment 14

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

Comment on attachment 8873676 [details]
Bug 1367679. P5 - add a gtest to test chaining between different promise types.

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

Comment 15

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

Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/324ef6c18030
P1 - refactor InvokeCallbackMethod() to deal with one concern at a time. r=gerald
https://hg.mozilla.org/integration/autoland/rev/fcc064923628
P2 - overload InvokeCallbackMethod() according to whether promise-chaining is supported. r=gerald
https://hg.mozilla.org/integration/autoland/rev/3297397ead64
P3 - pass mCompletionPromise to InvokeCallbackMethod(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/499cb7e486c0
P4 - specialize the type of mCompletionPromise according to whether chaining is supported. r=gerald
https://hg.mozilla.org/integration/autoland/rev/b5cb93142cb0
P5 - add a gtest to test chaining between different promise types. r=gerald

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/324ef6c18030
https://hg.mozilla.org/mozilla-central/rev/fcc064923628
https://hg.mozilla.org/mozilla-central/rev/3297397ead64
https://hg.mozilla.org/mozilla-central/rev/499cb7e486c0
https://hg.mozilla.org/mozilla-central/rev/b5cb93142cb0

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/715ef330d2d4
P1 - refactor InvokeCallbackMethod() to deal with one concern at a time. r=gerald
https://hg.mozilla.org/integration/autoland/rev/e9ec33eda908
P2 - overload InvokeCallbackMethod() according to whether promise-chaining is supported. r=gerald
https://hg.mozilla.org/integration/autoland/rev/76c8629e6fa1
P3 - pass mCompletionPromise to InvokeCallbackMethod(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/470e0ad16158
P4 - specialize the type of mCompletionPromise according to whether chaining is supported. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ac71a080692d
P5 - add a gtest to test chaining between different promise types. r=gerald

Comment 19

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

re-landed per bug 1370005 comment 9.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/715ef330d2d4
https://hg.mozilla.org/mozilla-central/rev/e9ec33eda908
https://hg.mozilla.org/mozilla-central/rev/76c8629e6fa1
https://hg.mozilla.org/mozilla-central/rev/470e0ad16158
https://hg.mozilla.org/mozilla-central/rev/ac71a080692d