1362910 - Support move-only types for MozPromise

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P1)

Description

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

It would be quite useful to support move-only types when IsExclusive [1] is true for there is at most only one consumer and it is safe to move the ResolveOrRejectValue stored in the promise.

[1] http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/xpcom/threads/MozPromise.h#130

Comment 1

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

Attachment: Bug 1362910. P1 - enable move when IsExclusive is true.

When IsExclusive is true, there is at most only one consumer.
So it is safe to move the ResolveOrRejectValue stored in the promise.

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

Comment 2

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

Attachment: Bug 1362910. P2 - fix callers.

Since RefPtr<T>&& can't be converted to T* implicitly, we need to change
T* to RefPtr<T> in resolve/reject callbacks to make it compile again.

We should review the changes later to look for the opportunity to
optimize away unnecessary AddRef/Release pairs.

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

Comment 3

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

Attachment: Bug 1362910. P3 - add a gtest to test move-only types with MozPromise.

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

Comment 4

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

Comment on attachment 8866665 [details]
Bug 1362910. P1 - enable move when IsExclusive is true.

https://reviewboard.mozilla.org/r/138272/#review141466

Comment 5

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

Comment on attachment 8866666 [details]
Bug 1362910. P2 - fix callers.

https://reviewboard.mozilla.org/r/138274/#review141470

Comment 6

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

Comment on attachment 8866667 [details]
Bug 1362910. P3 - add a gtest to test move-only types with MozPromise.

https://reviewboard.mozilla.org/r/138276/#review141478

::: dom/media/gtest/TestMozPromise.cpp:340
(Diff revision 1)
> +      EXPECT_FALSE(aVal.IsNothing());
> +      EXPECT_TRUE(aVal.IsResolve());
> +      EXPECT_FALSE(aVal.IsReject());
> +      EXPECT_EQ(87, *aVal.ResolveValue());
> +
> +      // Move() shouldn't change the resovle/reject state of aVal.

resovle -> resolve

Comment 7

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

Thanks for the review!

Comment 8

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

Comment on attachment 8866667 [details]
Bug 1362910. P3 - add a gtest to test move-only types with MozPromise.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3e212b01bec
P1 - enable move when IsExclusive is true. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f3590d95b1a6
P2 - fix callers. r=gerald
https://hg.mozilla.org/integration/autoland/rev/a9520acf01ad
P3 - add a gtest to test move-only types with MozPromise. r=gerald

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3e212b01bec
https://hg.mozilla.org/mozilla-central/rev/f3590d95b1a6
https://hg.mozilla.org/mozilla-central/rev/a9520acf01ad