Add the ability to chain MozPromises of different types

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla55
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
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);
    },
    []() {});
(Assignee)

Updated

2 years ago
Depends on: 1368382
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8873672 - Flags: review?(gsquelart)
Attachment #8873673 - Flags: review?(gsquelart)
Attachment #8873674 - Flags: review?(gsquelart)
Attachment #8873675 - Flags: review?(gsquelart)
Attachment #8873676 - Flags: review?(gsquelart)
Assignee: nobody → jwwang
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
Attachment #8873672 - Flags: review?(gsquelart) → review+
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?)
Attachment #8873673 - Flags: review?(gsquelart) → review+
Comment on attachment 8873674 [details]
Bug 1367679. P3 - pass mCompletionPromise to InvokeCallbackMethod().

https://reviewboard.mozilla.org/r/145064/#review149052
Attachment #8873674 - Flags: review?(gsquelart) → review+
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
Attachment #8873675 - Flags: review?(gsquelart) → review+
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
Attachment #8873676 - Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
Thanks!

Comment 16

2 years ago
Pushed by jwwang@mozilla.com:
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
(Assignee)

Updated

2 years ago
Depends on: 1370005
(Assignee)

Updated

2 years ago
No longer depends on: 1370005

Comment 18

2 years ago
Pushed by jwwang@mozilla.com:
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
(Assignee)

Comment 19

2 years ago
re-landed per bug 1370005 comment 9.
Depends on: 1371982
(Assignee)

Updated

2 years ago
No longer depends on: 1371982
You need to log in before you can comment on or make changes to this bug.