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

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1244768
(Assignee)

Updated

2 years ago
Assignee: nobody → kaku
(Assignee)

Comment 1

2 years ago
Created attachment 8777786 [details]
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/
Attachment #8777786 - Flags: review?(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....
Flags: needinfo?(kaku)
(Assignee)

Comment 3

2 years ago
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.
Flags: needinfo?(kaku)
(Assignee)

Updated

2 years ago
Attachment #8777786 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Assignee: kaku → nobody
No longer blocks: 1244768
Status: NEW → UNCONFIRMED
Ever confirmed: false
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.
Did you want to do the "convert all the callers" thing?
Flags: needinfo?(kaku)
(Assignee)

Comment 6

2 years ago
I was already convinced that MaybeResolve(JS::UndefinedHandleValue) is good enough, so I am wondering why do you think the MaybeResolveWithUndefined() is worthy?
Flags: needinfo?(kaku) → needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 8

2 years ago
Then, I could do it. Will update the patches soon.
(Assignee)

Updated

2 years ago
Assignee: nobody → kaku
Blocks: 1244768
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8777786 - Flags: review?(bzbarsky) → review+
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!
Attachment #8779269 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
Thanks for the review!
Nothing weird found in the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03375e58a667
Keywords: checkin-needed

Comment 16

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa719d1a5a6a
https://hg.mozilla.org/mozilla-central/rev/dbaa18a61a87
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.