1292091 - Add Promise::MaybeResolveWithUndefined utility for implementing promise-based HTMLMediaElement::play() API

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Tzuhao Kuo [:kaku]]

In bug 1244768, we are going to implement the promise-based HTMLMediaElement::play() API which resolves promises with undefined.

https://html.spec.whatwg.org/multipage/embedded-content.html#resolve-pending-play-promises, this is where the resolving promises with undefined is specified.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1292091 part 1 - add Promise::MaybeResolveWithUndefined() utility method;

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Why is MaybeResolve(JS::UndefinedHandleValue) not good enough?  http://searchfox.org/mozilla-central/search?q=MaybeResolve(JS%3A%3AUndefinedHandleValue suggests it's quite widely used already....

Comment 3

[Tzuhao Kuo [:kaku]]

You are quite right, MaybeResolve(JS::UndefinedHandleValue) is good enough. I followed the mindset of Promise::MaybeRejectWithUndefined() without a search of usage to the code base, sorry about that. Will cancel the review request and close this issue.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well.  It might be worth having a MaybeResolveWithUndefined, but we should just convert _all_ the callers to it.  That would be worth it, I guess.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Did you want to do the "convert all the callers" thing?

Comment 6

[Tzuhao Kuo [:kaku]]

I was already convinced that MaybeResolve(JS::UndefinedHandleValue) is good enough, so I am wondering why do you think the MaybeResolveWithUndefined() is worthy?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, we have a MaybeRejectWithUndefined (which is a _much_ weirder operation, by the way)...  And resolving with undefined is in fact a common spec operation, so it might make sense to have a shortcut for it.

Comment 8

[Tzuhao Kuo [:kaku]]

Then, I could do it. Will update the patches soon.

Comment 9

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1292091 part 2 - replace MaybeResolve(JS::UndefinedHandleValue) with MaybeResolveWithUndefined();

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

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8777786 [details]
Bug 1292091 part 1 - add Promise::MaybeResolveWithUndefined() utility method;

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8777786 [details]
Bug 1292091 part 1 - add Promise::MaybeResolveWithUndefined() utility method;

https://reviewboard.mozilla.org/r/69262/#review67838

r=me

::: dom/promise/Promise.cpp:933
(Diff revision 2)
>  void
> +Promise::MaybeResolveWithUndefined()
> +{
> +  NS_ASSERT_OWNINGTHREAD(Promise);
> +
> +  MaybeSomething(JS::UndefinedHandleValue, &Promise::MaybeResolve);

This could probably just call MaybeResolve(JS::UndefinedHandleValue) instead of the more complicated MaybeSomething invocation.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779269 [details]
Bug 1292091 part 2 - replace MaybeResolve(JS::UndefinedHandleValue) with MaybeResolveWithUndefined();

https://reviewboard.mozilla.org/r/70292/#review67840

r=me.  Thank you!

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8777786 [details]
Bug 1292091 part 1 - add Promise::MaybeResolveWithUndefined() utility method;

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

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8779269 [details]
Bug 1292091 part 2 - replace MaybeResolve(JS::UndefinedHandleValue) with MaybeResolveWithUndefined();

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

Comment 15

[Tzuhao Kuo [:kaku]]

Thanks for the review!
Nothing weird found in the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03375e58a667

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa719d1a5a6a
Part 1 - add Promise::MaybeResolveWithUndefined() utility method; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbaa18a61a87
Part 2 - replace MaybeResolve(JS::UndefinedHandleValue) with MaybeResolveWithUndefined(); r=bz

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/fa719d1a5a6a
https://hg.mozilla.org/mozilla-central/rev/dbaa18a61a87