Closed Bug 1367679 Opened 7 years ago Closed 7 years ago

Add the ability to chain MozPromises of different types

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla55

Tracking Flags:

Tracking

Status

firefox55

---

fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(5 files)

Bug 1367679. P1 - refactor InvokeCallbackMethod() to deal with one concern at a time. 7 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	mozbugz : review+	Details
Bug 1367679. P2 - overload InvokeCallbackMethod() according to whether promise-chaining is supported. 7 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	mozbugz : review+	Details
Bug 1367679. P3 - pass mCompletionPromise to InvokeCallbackMethod(). 7 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	mozbugz : review+	Details
Bug 1367679. P4 - specialize the type of mCompletionPromise according to whether chaining is supported. 7 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	mozbugz : review+	Details
Bug 1367679. P5 - add a gtest to test chaining between different promise types. 7 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	mozbugz : review+	Details

JW Wang [:jwwang] [:jw_wang]

Assignee

Description

•

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

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

7 years ago

Depends on: 1368382

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

7 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)

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

Updated

•

7 years ago

Assignee: nobody → jwwang

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

Comment 6

•

7 years ago

mozreview-review

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+

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

Comment 7

•

7 years ago

mozreview-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+

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

Comment 8

•

7 years ago

mozreview-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+

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

Comment 9

•

7 years ago

mozreview-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+

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

Comment 10

•

7 years ago

mozreview-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 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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

JW Wang [:jwwang] [:jw_wang]

Assignee

Comment 15

•

7 years ago

Thanks!

Pulsebot

Comment 16

•

7 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

Carsten Book [:Tomcat]

Comment 17

•

7 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

7 years ago

Depends on: 1370005

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

7 years ago

No longer depends on: 1370005

Pulsebot

Comment 18

•

7 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

JW Wang [:jwwang] [:jw_wang]

Assignee

Comment 19

•

7 years ago

re-landed per bug 1370005 comment 9.

Ryan VanderMeulen [:RyanVM]

Comment 20

•

7 years ago

bugherder

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

Flags: in-testsuite+

Andrew McCreight [:mccr8]

Updated

•

7 years ago

Depends on: 1371982

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

7 years ago

No longer depends on: 1371982

You need to log in before you can comment on or make changes to this bug.