Closed Bug 1292091 Opened 3 years ago Closed 3 years ago

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


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

Not set



Tracking Status
firefox51 --- fixed


(Reporter: kaku, Assigned: kaku)




(2 files)

In bug 1244768, we are going to implement the promise-based HTMLMediaElement::play() API which resolves promises with undefined., this is where the resolving promises with undefined is specified.
Blocks: 1244768
Assignee: nobody → kaku
Why is MaybeResolve(JS::UndefinedHandleValue) not good enough? suggests it's quite widely used already....
Flags: needinfo?(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.
Flags: needinfo?(kaku)
Attachment #8777786 - Flags: review?(bzbarsky)
Assignee: kaku → nobody
No longer blocks: 1244768
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)
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)
Then, I could do it. Will update the patches soon.
Assignee: nobody → kaku
Blocks: 1244768
Comment on attachment 8777786 [details]
Bug 1292091 part 1 - add Promise::MaybeResolveWithUndefined() utility method;


::: dom/promise/Promise.cpp:933
(Diff revision 2)
>  void
> +Promise::MaybeResolveWithUndefined()
> +{
> +
> +  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();

r=me.  Thank you!
Attachment #8779269 - Flags: review?(bzbarsky) → review+
Thanks for the review!
Nothing weird found in the try:
Keywords: checkin-needed
Pushed by
Part 1 - add Promise::MaybeResolveWithUndefined() utility method; r=bz
Part 2 - replace MaybeResolve(JS::UndefinedHandleValue) with MaybeResolveWithUndefined(); r=bz
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.