Paul Adenot (:padenot) Reporter
	Description • 8 years ago

Spec discussion: https://github.com/whatwg/html/issues/505

Spec commit: https://github.com/whatwg/html/commit/be8edde20ac46786af968d4de23e90801d4c7213

Spec text: https://html.spec.whatwg.org/#dom-media-play

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 1 • 8 years ago

Mass change P2 -> P3

	Renski
	Comment 2 • 8 years ago

Note that MDN's documenation mentions (already) that the method returns a promise: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/play

Available in Chrome since v50: https://developers.google.com/web/updates/2016/03/play-returns-promise

	Tzuhao Kuo [:kaku] Assignee
	Comment 3 • 8 years ago

Note about an issue following the original spec commit.

Issue: https://github.com/whatwg/html/issues/996

Issue PR: https://github.com/whatwg/html/pull/1038

	JW Wang [:jwwang] [:jw_wang]
	Comment 12 • 8 years ago
mozreview-review

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review67530

::: dom/html/HTMLMediaElement.h:1662
(Diff revision 1)
>    bool mIsAudioTrackAudible;
>  
>    // True if media element is audible for users.
>    bool mAudible;
> +
> +  // A list of pending play promises. The entries are push during the play()

are "pushed" during...

Usually we call "elements" of an array and "entries" of a table.

::: dom/html/HTMLMediaElement.cpp:165
(Diff revision 1)
>  // Threshold above which audio is muted
>  static const double THRESHOLD_HIGH_PLAYBACKRATE_AUDIO = 4.0;
>  // Threshold under which audio is muted
>  static const double THRESHOLD_LOW_PLAYBACKRATE_AUDIO = 0.5;
>  
> +void ResolvePendingPlayPromises(nsTArray<RefPtr<Promise>>&& aPromises)

I can't see the point to pass aPromises as an rvalue since there is no ownership transfer at all.

::: dom/html/HTMLMediaElement.cpp:170
(Diff revision 1)
> +void ResolvePendingPlayPromises(nsTArray<RefPtr<Promise>>&& aPromises)
> +{
> +  for (auto& promise : aPromises) {
> +    promise->MaybeResolve(JS::UndefinedHandleValue);
> +  }
> +

blank line.

::: dom/html/HTMLMediaElement.cpp:179
(Diff revision 1)
> +                               nsresult aError)
> +{
> +  for (auto& promise : aPromises) {
> +    promise->MaybeReject(aError);
> +  }
> +

blank line.

::: dom/html/HTMLMediaElement.cpp:185
(Diff revision 1)
> +}
> +
> +void AsyncResolvePendingPlayPromises(nsTArray<RefPtr<Promise>>&& aPromises)
> +{
> +  NS_DispatchToMainThread(NS_NewRunnableFunction(
> +    [aPromises]() mutable -> void {

This won't work as intended since the capture [aPromises] is always copy constructed.

::: dom/html/HTMLMediaElement.cpp:305
(Diff revision 1)
> +  {
> +  }
> +
> +  NS_IMETHOD Run()
> +  {
> +    if (IsCancelled()) {

Should reject the promises when IsCancelled() is true?

::: dom/html/HTMLMediaElement.cpp:4768
(Diff revision 1)
>      return NS_OK;
>    }
>  
> -  nsCOMPtr<nsIRunnable> event = new nsAsyncEventRunner(aName, this);
> +  nsCOMPtr<nsIRunnable> event;
> +
> +  if (aName.EqualsLiteral("playing")) {

Can you show me the spec which says to resolve the play promises when firing the 'playing' event?

::: dom/html/HTMLMediaElement.cpp:6235
(Diff revision 1)
> +  mPendingPlayPromises.Clear();
> +  return promises;
> +}
> +
> +void
> +HTMLMediaElement::NotifyAboutPlaying()

Who is the caller of this function?

	JW Wang [:jwwang] [:jw_wang]
	Comment 13 • 8 years ago
mozreview-review

Comment on attachment 8779211 [details]
Bug 1244768 part 4 - call NotifyAboutPlaying() while ready state is changed;

https://reviewboard.mozilla.org/r/70252/#review67566

	JW Wang [:jwwang] [:jw_wang]
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8779212 [details]
Bug 1244768 part 6 - modify the internal pause steps;

https://reviewboard.mozilla.org/r/70254/#review67568

	JW Wang [:jwwang] [:jw_wang]
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8779213 [details]
Bug 1244768 part 7 - refactor the Play() and PlayInternal() methods;

https://reviewboard.mozilla.org/r/70256/#review67570

	JW Wang [:jwwang] [:jw_wang]
	Comment 16 • 8 years ago
mozreview-review

Comment on attachment 8779214 [details]
Bug 1244768 part 8 - extract the HTMLMediaElement::CreateDOMPromise() utility;

https://reviewboard.mozilla.org/r/70258/#review67572

	JW Wang [:jwwang] [:jw_wang]
	Comment 17 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review67574

::: dom/html/HTMLMediaElement.cpp:2705
(Diff revision 1)
> +
> +  // 1. If the media element is not allowed to play, return a promise rejected
> +  //    with a "NotAllowedError" DOMException and abort these steps.
>    if (!IsAllowedToPlay()) {
> -    return;
> +    aRv.Throw(NS_ERROR_DOM_MEDIA_NOT_ALLOWED_ERR);
> +    return nullptr;

|return nullptr| means to return a rejected promise?

::: dom/html/HTMLMediaElement.cpp:2787
(Diff revision 1)
>    // seek to the effective start.
>    if (mPaused) {
> +    // 6.1. Change the value of paused to false.
> +    mPaused = false;
> +
> +    //6.3. If the show poster flag is true, set the element's show poster flag

Put 6.3 below 6.2.

::: dom/html/HTMLMediaElement.cpp:2811
(Diff revision 1)
>        DispatchAsyncEvent(NS_LITERAL_STRING("waiting"));
>        break;
>      case nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA:
>      case nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA:
>        FireTimeUpdate(false);
>        DispatchAsyncEvent(NS_LITERAL_STRING("playing"));

We should run notify-about-playing here, right?

::: dom/html/HTMLMediaElement.cpp:2816
(Diff revision 1)
>        DispatchAsyncEvent(NS_LITERAL_STRING("playing"));
>        break;
>      }
>    }
>  
> -  mPaused = false;
> +  // 7. Otherwise, if the media element's readyState attribute has the value

Step 7 should run only when paused is originally false.

	Tzuhao Kuo [:kaku] Assignee
	Comment 18 • 8 years ago
mozreview-review-reply

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review67530

> I can't see the point to pass aPromises as an rvalue since there is no ownership transfer at all.

Semantically, the ResolvePendingPlayPromises() and RejectPendingPlayPromises() take the ownership of the passed promises; the spec always asks user agents to "take pending play promises" before resolve/reject them and "take pending play promises" is specified to transfer the ownership of the pending play promises out from the HTMLMediaElement. So, I have a method called "HTMLMediaElement::TakePendingPlayPromises()" which returns a temporary object and it's easy to write codes like | ResolvePendingPlayPromises(TakePendingPlayPromises()); | if we make the ResolvePendingPlayPromises() and RejectPendingPlayPromises() take r-value reference parameters as what I have done.


Surely, we could make the ResolvePendingPlayPromises() and RejectPendingPlayPromises() take l-value reference parameter and write codes like:

| auto promises = TakePendingPlayPromises(); |
| ResolvePendingPlayPromises(promises);      |


Not sure which one is better, I personally like the original one, WDYT?

> This won't work as intended since the capture [aPromises] is always copy constructed.

Will then have a hand-written runnable class with a construtor to move the promises.

> Should reject the promises when IsCancelled() is true?

I am afraid not. If the load algorithm (or the resource selection algorithm) is canceled, the already dispatched resolve-/reject-promises tasks should be performed immediately at the time canceling the load algorithm. This is specified here: https://html.spec.whatwg.org/multipage/embedded-content.html#media-element-load-algorithm . I will implement this mechanism at the next version of part 2 patch.

> Can you show me the spec which says to resolve the play promises when firing the 'playing' event?

Here: https://html.spec.whatwg.org/multipage/embedded-content.html#notify-about-playing

> Who is the caller of this function?

Plenty. Please click on the "notify about playing" https://html.spec.whatwg.org/multipage/embedded-content.html#notify-about-playing and you will see the callers.

	Tzuhao Kuo [:kaku] Assignee
	Comment 19 • 8 years ago
mozreview-review-reply

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review67574

> |return nullptr| means to return a rejected promise?

Yes, the promise will be rejected since we have thrown a error.

> We should run notify-about-playing here, right?

Yes, you are right.

> Step 7 should run only when paused is originally false.

Ok.

	JW Wang [:jwwang] [:jw_wang]
	Comment 29 • 8 years ago
mozreview-review-reply

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review67530

> Semantically, the ResolvePendingPlayPromises() and RejectPendingPlayPromises() take the ownership of the passed promises; the spec always asks user agents to "take pending play promises" before resolve/reject them and "take pending play promises" is specified to transfer the ownership of the pending play promises out from the HTMLMediaElement. So, I have a method called "HTMLMediaElement::TakePendingPlayPromises()" which returns a temporary object and it's easy to write codes like | ResolvePendingPlayPromises(TakePendingPlayPromises()); | if we make the ResolvePendingPlayPromises() and RejectPendingPlayPromises() take r-value reference parameters as what I have done.
> 
> 
> Surely, we could make the ResolvePendingPlayPromises() and RejectPendingPlayPromises() take l-value reference parameter and write codes like:
> 
> | auto promises = TakePendingPlayPromises(); |
> | ResolvePendingPlayPromises(promises);      |
> 
> 
> Not sure which one is better, I personally like the original one, WDYT?

I have no idea. Maybe just leave it as is.

	JW Wang [:jwwang] [:jw_wang]
	Comment 30 • 8 years ago
mozreview-review

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review71272

	JW Wang [:jwwang] [:jw_wang]
	Comment 31 • 8 years ago
mozreview-review

Comment on attachment 8779209 [details]
Bug 1244768 part 2 - modify media element load algorith;

https://reviewboard.mozilla.org/r/70248/#review71274

	JW Wang [:jwwang] [:jw_wang]
	Comment 32 • 8 years ago
mozreview-review

Comment on attachment 8779210 [details]
Bug 1244768 part 3 - modify the resource selection algorithm;

https://reviewboard.mozilla.org/r/70250/#review71276

	JW Wang [:jwwang] [:jw_wang]
	Comment 33 • 8 years ago
mozreview-review

Comment on attachment 8782758 [details]
Bug 1244768 part 5 - reject pending play promises while the playback reaching the end;

https://reviewboard.mozilla.org/r/72794/#review71278

	JW Wang [:jwwang] [:jw_wang]
	Comment 34 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review71282

::: dom/html/HTMLMediaElement.cpp:2841
(Diff revision 2)
>        FireTimeUpdate(false);
> -      DispatchAsyncEvent(NS_LITERAL_STRING("playing"));
> +      NotifyAboutPlaying();
>        break;
>      }
>    }
> +  else if (mReadyState >= nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA) {

The style is:
if () {
} else if () {
}

"else" on the same line as the ending bracket.

	Tzuhao Kuo [:kaku] Assignee
	Comment 35 • 8 years ago

Thanks for the review, will then work on mochitests.

	Boris Zbarsky [:bzbarsky]
	Comment 46 • 8 years ago
mozreview-review

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review72824

::: dom/html/HTMLMediaElement.h:1739
(Diff revision 3)
>    // True if media element is audible for users.
>    bool mAudible;
> +
> +  // A list of pending play promises. The elements are pushed during the play()
> +  // method call and are resolved/rejected during further playback steps.
> +  nsTArray<RefPtr<Promise>> mPendingPlayPromises;

Why does this not need to be cycle collected?  I expect it does!

::: dom/html/HTMLMediaElement.cpp:307
(Diff revision 3)
> +  }
> +
> +  NS_IMETHOD Run() override
> +  {
> +    if (IsCancelled()) {
> +      return NS_OK;

Does it not need to be removed from mPendingPlayPromisesRunners?  I don't see anything else in this patch that does that removal....

	Boris Zbarsky [:bzbarsky]
	Comment 47 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review72816

I'd like to understand what ensures that things don't stick around in mPendingPlayPromises on some of the return codepaths in this code.  See below.

::: dom/html/HTMLMediaElement.cpp:3130
(Diff revision 3)
> +  // 2. If the media element's error attribute is not null and its code
> +  //    attribute has the value MEDIA_ERR_SRC_NOT_SUPPORTED, return a promise
> +  //    rejected with a "NotSupportedError" DOMException and abort these steps.
> +  if (mError) {
> +    uint16_t errorCode;
> +    mError->GetCode(&errorCode);

Why are you not using mError->Code()?

::: dom/html/HTMLMediaElement.cpp:3142
(Diff revision 3)
> +  // 3. Let promise be a new promise and append promise to the list of pending
> +  //    play promises.
> +  RefPtr<Promise> promise = CreateDOMPromise(aRv);
> +
> +  if (aRv.Failed()) {
> +    return promise.forget();

return nullptr, please.

::: dom/html/HTMLMediaElement.cpp:3170
(Diff revision 3)
>        !aCallerIsChrome &&
>        OwnerDoc()->Hidden()) {
>      LOG(LogLevel::Debug, ("%p Blocked playback because owner hidden.", this));
>      mPlayBlockedBecauseHidden = true;
> -    return;
> +    aRv.Throw(NS_ERROR_DOM_MEDIA_NOT_ALLOWED_ERR);
> +    return promise.forget();

Should this be rejecting the promise?

::: dom/html/HTMLMediaElement.cpp:3186
(Diff revision 3)
>      }
>      if (!mPausedForInactiveDocumentOrChannel) {
>        nsresult rv = mDecoder->Play();
>        if (NS_FAILED(rv)) {
>          aRv.Throw(rv);
> -        return;
> +        return promise.forget();

Should probably return null instead; the return value will get ignored anyway, and the error result on aRv will be used to create a rejected promise.

That said, what is the intended disposition of "promise" after this point?  What removes it from mPendingPlayPromises?  Same for the other early returns in this function.

::: dom/html/HTMLMediaElement.cpp:3260
(Diff revision 3)
>  }
>  
>  NS_IMETHODIMP HTMLMediaElement::Play()
>  {
>    ErrorResult rv;
> -  PlayInternal(/* aCallerIsChrome = */ true, rv);
> +  RefPtr<Promise> tobeIgnored = PlayInternal(/* aCallerIsChrome = */ true, rv);

So this assumes that the callee will never propagate out error state via the promise directly...

I guess that's ok.  We should look into removing this method altogether, right?

::: dom/html/HTMLMediaElement.cpp:6121
(Diff revision 3)
>    MOZ_ASSERT(mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE ||
>               mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE_DISPOSABLE);
>  
>    SetAudioChannelSuspended(nsISuspendedTypes::NONE_SUSPENDED);
>    ErrorResult rv;
> -  PlayInternal(nsContentUtils::IsCallerChrome(), rv);
> +  RefPtr<Promise> tobeIgnored = PlayInternal(nsContentUtils::IsCallerChrome(), rv);

Similar concerns here: this caller is assuming play state is known sync, whereas that's no longer the case, right?

	JW Wang [:jwwang] [:jw_wang]
	Comment 48 • 8 years ago
mozreview-review

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review72996

::: dom/media/test/test_play_promise.html:387
(Diff revision 1)
> +function initTest(test, token) {
> +  manager.started(token);
> +
> +  playBeforeCanPlay(test, token)
> +  .then(() => { return playWhenCanPlay(test, token); })
> +  .then(() => { return playAfterPlaybackStarted(test, token); })

Both playWhenCanPlay and playAfterPlaybackStarted are independent tests which can run in parallel. Please split them into small tests which are more readable and maintainable.

	Tzuhao Kuo [:kaku] Assignee
	Comment 49 • 8 years ago
mozreview-review-reply

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review72824

> Why does this not need to be cycle collected?  I expect it does!

Yes, it should, thanks.

> Does it not need to be removed from mPendingPlayPromisesRunners?  I don't see anything else in this patch that does that removal....

Yes, it should and also apply to the nsNotifyAboutPlayingRunner bellow.

	Tzuhao Kuo [:kaku] Assignee
	Comment 50 • 8 years ago
mozreview-review-reply

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review72816

> Should this be rejecting the promise?

No, we should return a promise and make it pending here. Thanks!

> Should probably return null instead; the return value will get ignored anyway, and the error result on aRv will be used to create a rejected promise.
> 
> That said, what is the intended disposition of "promise" after this point?  What removes it from mPendingPlayPromises?  Same for the other early returns in this function.

Agree, I should return null with an exception here and remove the promise out from the mPendingPlayPromises right here. Thanks.

> So this assumes that the callee will never propagate out error state via the promise directly...
> 
> I guess that's ok.  We should look into removing this method altogether, right?

Yes, we can just remove this method, JW also agrees on it. I prefer to do it on a separate bug and make this bug relay on it.

> Similar concerns here: this caller is assuming play state is known sync, whereas that's no longer the case, right?

Not sure about what do you mean "sync"?

The play() operation has always been a asynchronous one. It used events before but now it use promise instead. Do I misunderstand anything?

	Boris Zbarsky [:bzbarsky]
	Comment 51 • 8 years ago

> Yes, it should and also apply to the nsNotifyAboutPlayingRunner bellow.

OK.  In that case...  I did _not_ carefully review the ownership/logic model for the runnable stuff and what not.  I was mostly asking questions to make sure someone had.  Someone needs to do that, clearly...  It could also use some documentation describing how it's supposed to work.

> I prefer to do it on a separate bug

Yes, of course.

> Not sure about what do you mean "sync"?

"Synchronously".

> The play() operation has always been a asynchronous one.

Yes.  But what I'm talking about is this.  It used to be that PlayInternal would throw on the ErrorResult in some cases (would it?) and then ResumeFromAudioChannelPaused would skip calling DispatchAsyncEvent("mozinterruptend") in those cases.

Now that PlayInternal can return a rejected promise (does it?) instead of throwing on ErrorResult, wouldn't you need to wait on the promise before deciding whether to fire the event?

	Tzuhao Kuo [:kaku] Assignee
	Comment 52 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #51)
> > Yes, it should and also apply to the nsNotifyAboutPlayingRunner bellow.
> 
> OK.  In that case...  I did _not_ carefully review the ownership/logic model
> for the runnable stuff and what not.  I was mostly asking questions to make
> sure someone had.  Someone needs to do that, clearly...  It could also use
> some documentation describing how it's supposed to work.
I have comments on top of the nsResolveOrRejectPendingPlayPromisesRunner class which indicates that a nsResolveOrRejectPendingPlayPromisesRunner instance will be appended into the mPendingPlayPromisesRunners and will be removed once it is ran. However, what I missed is what if the runner is canceled, I should also remove it out form the mPendingPlayPromisesRunners. Thanks!

	Tzuhao Kuo [:kaku] Assignee
	Comment 53 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #51)
> Yes.  But what I'm talking about is this.  It used to be that PlayInternal
> would throw on the ErrorResult in some cases (would it?) and then
> ResumeFromAudioChannelPaused would skip calling
> DispatchAsyncEvent("mozinterruptend") in those cases.
> 
> Now that PlayInternal can return a rejected promise (does it?) instead of
> throwing on ErrorResult, wouldn't you need to wait on the promise before
> deciding whether to fire the event?
So what you are concerning now is about a pending promise which might be rejected later, right? For those promises that is resolved/rejected immediately, we have handled them via the passed-in ErrorResult variable, _rv_; if a promise is rejected immediately, the |NS_FAILED(rv.StealNSResult())| should be true and then we do not fire the event.

However, I am not sure if we need to handle the pending promises.
Previously, if the PlayInternal() does not throw immediately but the play operation is canceled later silently (by any means), we have no way to capture it. 
@JW, should we wait on the returned promise's before deciding whether to fire the event or not?

	JW Wang [:jwwang] [:jw_wang]
	Comment 54 • 8 years ago

https://hg.mozilla.org/mozilla-central/file/tip/dom/html/HTMLMediaElement.cpp#l5917
Is this event you are talking about?

I don't think we need to wait for the promise before firing the event.

PlayInternal() used to return an error code and "mozinterruptend" won't be fired if it is not NS_OK.

We can now change PlayInternal() to return both an error code and a promise, and "mozinterruptend" won't be fired if the error code is not NS_OK to preserve the same behaviour.

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 8 years ago

> However, what I missed is what if the runner is canceled, I should also remove
> it out form the mPendingPlayPromisesRunners

OK.  Is someone on the hook for carefully re-reviewing the processing model here?  Do I need to do it?

> So what you are concerning now is about a pending promise which might be rejected later, right?

No, I'm worried about a promise which is already rejected being returned from the callee.

> we have handled them via the passed-in ErrorResult variable

For now, yes.  But nothing guarantees this will continue to be the case; someone can change the callee without realizing that the caller depends on this weird behavior.  I say weird, because for a Promise-returning webidl method, returning an error ErrorResult and returning a rejected promise directly are supposed to be exactly equivalent.

> Is this event you are talking about?

Yes.

> PlayInternal() used to return an error code and "mozinterruptend" won't be fired if it is not NS_OK.

Indeed.  And I'm worried about cases in which PlayInternal starts returning an already-rejected promise and an ok ErrorResult and how ResumeFromAudioChannelPaused will hand those.  There's is no indication anywhere in PlayInternal code that it should _not_ return such promises.

	Tzuhao Kuo [:kaku] Assignee
	Comment 56 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #55)
> > However, what I missed is what if the runner is canceled, I should also remove
> > it out form the mPendingPlayPromisesRunners
> 
> OK.  Is someone on the hook for carefully re-reviewing the processing model
> here?  Do I need to do it?
I am not sure what does "processing mode" mean exactly here...
I do think :jw has reviewed this patch carefully, but it's good for me to have a DOM peer to do the review again.
May I ask for your review for all the patch set after addressing your comments?

> > we have handled them via the passed-in ErrorResult variable
> 
> For now, yes.  But nothing guarantees this will continue to be the case;
> someone can change the callee without realizing that the caller depends on
> this weird behavior.  I say weird, because for a Promise-returning webidl
> method, returning an error ErrorResult and returning a rejected promise
> directly are supposed to be exactly equivalent.
> 
> > PlayInternal() used to return an error code and "mozinterruptend" won't be fired if it is not NS_OK.
> 
> Indeed.  And I'm worried about cases in which PlayInternal starts returning
> an already-rejected promise and an ok ErrorResult and how
> ResumeFromAudioChannelPaused will hand those.  There's is no indication
> anywhere in PlayInternal code that it should _not_ return such promises.
So, passively, we could add a comment on the PlayInternal() method, or we could do it better by adding a method to the Promise class for accessing its states (pending, resolved, rejected) and have an assertion on the promise's state and the ErrorResult. WDYT?

	Boris Zbarsky [:bzbarsky]
	Comment 57 • 8 years ago

> I am not sure what does "processing mode" mean exactly here...

The details of what gets added to which lists when and when it gets removed.

> I do think :jw has reviewed this patch carefully

I think so too.  But now you're changing parts of the "when things get removed" stuff, so those bits need careful re-review to make sure there's nothing else we're still missing, right?

> May I ask for your review for all the patch set after addressing your comments?

I can try, sure, but it would be better if someone more familiar with this code did that...

> we could add a comment on the PlayInternal() method

That would be better than nothing.

> adding a method to the Promise class for accessing its states (pending, resolved, rejected)

Indeed.  Should be pretty straightforward to implement, and is probably a good idea here.

	Boris Zbarsky [:bzbarsky]
	Comment 68 • 8 years ago
mozreview-review

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review75100

r-, commit message doesn't explain what is actually going on.

	Boris Zbarsky [:bzbarsky]
	Comment 69 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review75102

Not clear what I should be reviewing here...  Mozreview doesn't even think you asked me for review, but Bugzilla does...

	Alastor Wu [:alwu]
	Comment 70 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review75328

::: dom/html/HTMLMediaElement.cpp:3133
(Diff revision 4)
> +
> +  // 1. If the media element is not allowed to play, return a promise rejected
> +  //    with a "NotAllowedError" DOMException and abort these steps.
>    if (!IsAllowedToPlay()) {
> -    return;
> +    aRv.Throw(NS_ERROR_DOM_MEDIA_NOT_ALLOWED_ERR);
> +    return nullptr;

As our offline disscussion, we should create a pending promise if the media element is blocked.

::: dom/html/HTMLMediaElement.cpp:3139
(Diff revision 4)
> +  }
> +
> +  // 2. If the media element's error attribute is not null and its code
> +  //    attribute has the value MEDIA_ERR_SRC_NOT_SUPPORTED, return a promise
> +  //    rejected with a "NotSupportedError" DOMException and abort these steps.
> +  if (mError) {

nit: if (mError && mError->Code() == nsIDOMMediaError::MEDIA_ERR_SRC_NOT_SUPPORTED)

::: dom/html/HTMLMediaElement.cpp:3142
(Diff revision 4)
> +  //    attribute has the value MEDIA_ERR_SRC_NOT_SUPPORTED, return a promise
> +  //    rejected with a "NotSupportedError" DOMException and abort these steps.
> +  if (mError) {
> +    if (mError->Code() == nsIDOMMediaError::MEDIA_ERR_SRC_NOT_SUPPORTED) {
> +      aRv.Throw(NS_ERROR_DOM_MEDIA_NOT_SUPPORTED_ERR);
> +      return nullptr;

If we do earily return, we can't use the external app to open the non-supported media type.

You need to call OpenUnsupportedMediaWithExtenalAppIfNeeded() here.
But the tricky point is we check the |mPause| in that function, so this function would be called after changing |mPause| in present codebase. 

Therefore, you should also modify that function to make it work well.

::: dom/html/HTMLMediaElement.cpp:3150
(Diff revision 4)
> +
> +  // 3. Let promise be a new promise and append promise to the list of pending
> +  //    play promises.
> +  RefPtr<Promise> promise = CreateDOMPromise(aRv);
> +
> +  if (aRv.Failed()) {

nit : if (NS_WARN_IF(aRv.Failed()))

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros

::: dom/html/HTMLMediaElement.cpp:3266
(Diff revision 4)
>  }
>  
>  NS_IMETHODIMP HTMLMediaElement::Play()
>  {
>    ErrorResult rv;
> -  PlayInternal(rv);
> +  RefPtr<Promise> tobeIgnored = PlayInternal(rv);

nit : s/tobe/toBe, only using lower case for the first word

::: dom/html/HTMLMediaElement.cpp:6088
(Diff revision 4)
>    MOZ_ASSERT(mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE ||
>               mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE_DISPOSABLE);
>  
>    SetAudioChannelSuspended(nsISuspendedTypes::NONE_SUSPENDED);
>    ErrorResult rv;
> -  PlayInternal(rv);
> +  RefPtr<Promise> tobeIgnored = PlayInternal(rv);

ditto

::: dom/html/HTMLMediaElement.cpp:6089
(Diff revision 4)
>               mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE_DISPOSABLE);
>  
>    SetAudioChannelSuspended(nsISuspendedTypes::NONE_SUSPENDED);
>    ErrorResult rv;
> -  PlayInternal(rv);
> +  RefPtr<Promise> tobeIgnored = PlayInternal(rv);
> +  MOZ_ASSERT_IF(rv.Failed(), tobeIgnored->State() == PromiseState::rejected);

The return promise is nullptr if the rv failed, so the |tobeIgnored->State()| would be invalid operation.

	JW Wang [:jwwang] [:jw_wang]
	Comment 71 • 8 years ago
mozreview-review

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review75372

Please split test cases into different patches so it is easier to address review comments for each test case.

::: dom/media/test/play_promise.js:9
(Diff revision 2)
> +function invokePlayShouldBeResolved(testCase, element, token) {
> +  return new Promise((resolve, reject) => {
> +    element.play().then(
> +      (result) => {
> +        if (result == undefined) {
> +          ok(true, `${testCase}(${token}) is resolved with ${result}.`);

Helper functions should not contain policy code. We should let the test case decide whether the test succeeds or not.

::: dom/media/test/play_promise.js:11
(Diff revision 2)
> +    element.play().then(
> +      (result) => {
> +        if (result == undefined) {
> +          ok(true, `${testCase}(${token}) is resolved with ${result}.`);
> +          resolve();
> +        }else {

no space between '}' and 'else'.

::: dom/media/test/test_play_promise_1.html:16
(Diff revision 2)
> +<pre id="test">
> +
> +<script>
> +// Case: invoke play() on an element that doesn't have enough data
> +// Expected result: resolve the promise
> +function playBeforeCanPlay(test, token) {

I don't see the benefit of this function which is used only once. I think it is more readable to inline the code.

::: dom/media/test/test_play_promise_12.html:15
(Diff revision 2)
> +<body>
> +<pre id="test">
> +
> +<script>
> +// Case:: invoke pause() and then play() on an element that is already playing.
> +// Expected result: result the promise.

Do you mean 'resolve' the promise?

::: dom/media/test/test_play_promise_12.html:22
(Diff revision 2)
> +  return new Promise((resolve, reject) => {
> +    let element = document.createElement(getMajorMimeType(test.type));
> +    element.preload = "auto";
> +    element.src = test.name;
> +    once(element, "canplay").then(() => {
> +      element.play();

Why don't you just call play() at the beginning?

::: dom/media/test/test_play_promise_12.html:25
(Diff revision 2)
> +    element.src = test.name;
> +    once(element, "canplay").then(() => {
> +      element.play();
> +    });
> +    once(element, "playing").then(() => {
> +      ok(element.readyState >= HTMLMediaElement.HAVE_FUTURE_DATA);

This check is not related to the promise test. We should remove checks that are not relevant.

::: dom/media/test/test_play_promise_13.html:22
(Diff revision 2)
> +  return new Promise((resolve, reject) => {
> +    let element = document.createElement(getMajorMimeType(test.type));
> +    element.preload = "auto";
> +    element.src = test.name;
> +    once(element, "canplay").then(() => {
> +      // The play() promise will be queued to be resolved.

Do these tests need to run in the 'canplay' handler? Can they run immediately after setting .src?

::: dom/media/test/test_play_promise_13.html:25
(Diff revision 2)
> +    element.src = test.name;
> +    once(element, "canplay").then(() => {
> +      // The play() promise will be queued to be resolved.
> +      invokePlayShouldBeResolved("loadAlgorithmDoesNotCancelTasks", element, token).then(resolve);
> +      element.src = test.name; // Re-invoke the load algorithm again.
> +      ok(element.paused);

Remove this irrelevant check.

::: dom/media/test/test_play_promise_14.html:14
(Diff revision 2)
> +</head>
> +<body>
> +<pre id="test">
> +
> +<script>
> +// Case: step1: create an element with its paused member to be fause and networkState to be NETWORK_EMPTY.

The comment is confusing.

.paused is initialized to be false when an media element is created. It becomes true after calling play().

::: dom/media/test/test_play_promise_14.html:25
(Diff revision 2)
> +    // Invoke play() -> (1) the promise will be left pending.
> +    //                  (2) invoke load() -> (1) set the networkState to be NETWORK_NO_SOURCE.
> +    //                                       (2) queue a task to run resouce selection algorithm.
> +    invokePlayShouldBeResolved("loadAlgorithmKeepPromisesPendingWhenNotPausing", element, token).then(resolve);
> +    once(element, "play").then(() => {
> +      // The resouce selection algorithm has been done -> set the networkState to be NETWORK_EMPTY.

This comment looks wrong to me. Can you point me at the spec text?

::: dom/media/test/test_play_promise_15.html:14
(Diff revision 2)
> +</head>
> +<body>
> +<pre id="test">
> +
> +<script>
> +// Case: step1: create an element with its paused member to be fause and networkState to be NETWORK_EMPTY.

This test case is similar to test_play_promise_14.html. What is the difference?

	Tzuhao Kuo [:kaku] Assignee
	Comment 72 • 8 years ago
mozreview-review-reply

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review75372

Okay, will then open a separated bug.

Except for the explicitly replied, I am ok or agree to other comments.

> The comment is confusing.
> 
> .paused is initialized to be false when an media element is created. It becomes true after calling play().

I think it is on the opposite way. 
http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#2880
mPaused is initialized to be false and becomes true after playing.

> This comment looks wrong to me. Can you point me at the spec text?

Not really form the spec, it's from our implementation.
http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1189

> This test case is similar to test_play_promise_14.html. What is the difference?

The preload policy, however, I think it is identical for an invalid source.

	JW Wang [:jwwang] [:jw_wang]
	Comment 73 • 8 years ago
mozreview-review-reply

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review75372

> I think it is on the opposite way. 
> http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#2880
> mPaused is initialized to be false and becomes true after playing.

Sorry  I get confused.
https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-paused
It is initialized to true.

	JW Wang [:jwwang] [:jw_wang]
	Comment 74 • 8 years ago
mozreview-review-reply

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review75372

> Not really form the spec, it's from our implementation.
> http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1189

https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-algorithm
The resource selection algorithm 6.
Otherwise the media element has no assigned media provider object and has neither a src attribute nor a source element child: set the networkState to NETWORK_EMPTY, and abort these steps; the synchronous section ends.

The network state becomes NETWORK_EMPTY because there is no valid source to load.

> The preload policy, however, I think it is identical for an invalid source.

Please explain how the preload attribute makes a difference for these 2 tests.

	JW Wang [:jwwang] [:jw_wang]
	Comment 75 • 8 years ago
mozreview-review-reply

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review75372

> https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-algorithm
> The resource selection algorithm 6.
> Otherwise the media element has no assigned media provider object and has neither a src attribute nor a source element child: set the networkState to NETWORK_EMPTY, and abort these steps; the synchronous section ends.
> 
> The network state becomes NETWORK_EMPTY because there is no valid source to load.

Btw, please use stable links so you won't get the wrong context when searchfox re-index.

	Tzuhao Kuo [:kaku] Assignee
	Comment 76 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #68)
> Comment on attachment 8779208 [details]
> Bug 1244768 part 1 - implement utilities;
> 
> https://reviewboard.mozilla.org/r/70246/#review75100
> 
> r-, commit message doesn't explain what is actually going on.
Sorry about that, I will then update the comment message and ask for reviewing again. Briefly, this patch mainly implements the utilities, such as the pending play promise array, resolving and rejecting promises helper functions. 

(In reply to Boris Zbarsky [:bz] from comment #69)
> Comment on attachment 8779215 [details]
> Bug 1244768 part 9 - modify the play() method;
> 
> https://reviewboard.mozilla.org/r/70260/#review75102
> 
> Not clear what I should be reviewing here...  Mozreview doesn't even think
> you asked me for review, but Bugzilla does...
I asked for reviewing the modification for comment 47, however, since Alastor has pointed out some problems, I will then update the patch accordingly and then ask for review again.

	Boris Zbarsky [:bzbarsky]
	Comment 77 • 8 years ago

OK.  When you request review, please make it clear whether I need to review the entire patch in detail or just some particular parts...

	Tzuhao Kuo [:kaku] Assignee
	Comment 89 • 8 years ago

Hi bz, 

Please help to review patch-1 and patch-11.

Patch-1 is modified to addressing comment 46; (1) make the mPendingPlayPromises be cycle collected and (2) remove runnables out from mPendingPlayPromisesRunners while the runnable is canceled.

Patch-11 is modified to addressing comment 47.

	Tzuhao Kuo [:kaku] Assignee
	Comment 90 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review75926

::: dom/html/HTMLMediaElement.cpp:3201
(Diff revision 5)
>  
>    // Check if the element is now blocked.
>    if (IsPlayOperationBlocked()) {
> -    return;
> +    return promise.forget();
>    }
> -
> +  

Will remove this space.

	Alastor Wu [:alwu]
	Comment 91 • 8 years ago
mozreview-review

Comment on attachment 8789397 [details]
Bug 1244768 part 10 - OpenUnsupportedMediaWithExtenalAppIfNeeded() should not check whether the element is paused or not;

https://reviewboard.mozilla.org/r/77642/#review76084

	Alastor Wu [:alwu]
	Comment 92 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review76070

::: dom/html/HTMLMediaElement.cpp:6095
(Diff revision 5)
>  
>    SetAudioChannelSuspended(nsISuspendedTypes::NONE_SUSPENDED);
>    ErrorResult rv;
> -  PlayInternal(rv);
> +  RefPtr<Promise> toBeIgnored = PlayInternal(rv);
> +  MOZ_ASSERT_IF(rv.Failed() && toBeIgnored,
> +                toBeIgnored->State() == PromiseState::rejected);

I think you can remove this assert because the returning promise is always nullptr when the rv failed.

	Alastor Wu [:alwu]
	Comment 93 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review76086

	Alastor Wu [:alwu]
	Comment 94 • 8 years ago
mozreview-review

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review76072

::: dom/html/HTMLMediaElement.h:1266
(Diff revision 3)
> +   * won't resume it later.
> +   *
> +   * If IsPlayOperationBlocked() returns true, we make the play operation
> +   * pending for now and will resume it later.
> +   */
> +  bool IsPlayOperationAllowed() const;

Nit : The naming of these functions are similar but they have totally different return meaning, and I think it might be little confusion.

::: dom/html/HTMLMediaElement.cpp:3160
(Diff revision 3)
>  
>    if (mCurrentPlayRangeStart == -1.0) {
>      mCurrentPlayRangeStart = CurrentTime();
>    }
>  
> +  // Check if the element is now blocked.

I am not sure whether we can move the checking here (after doing lines [3128-3158])

Because the original idea is we don't want to change any internal state if the media element was blocked.

	JW Wang [:jwwang] [:jw_wang]
	Comment 95 • 8 years ago
mozreview-review

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review76202

::: dom/html/HTMLMediaElement.cpp:3160
(Diff revision 3)
>  
>    if (mCurrentPlayRangeStart == -1.0) {
>      mCurrentPlayRangeStart = CurrentTime();
>    }
>  
> +  // Check if the element is now blocked.

Can you explain the purpose of this patch? And Why do we return here when it is blocked?

	JW Wang [:jwwang] [:jw_wang]
	Comment 96 • 8 years ago
mozreview-review

Comment on attachment 8789397 [details]
Bug 1244768 part 10 - OpenUnsupportedMediaWithExtenalAppIfNeeded() should not check whether the element is paused or not;

https://reviewboard.mozilla.org/r/77642/#review76206

::: dom/html/HTMLMediaElement.cpp
(Diff revision 1)
>  
>    if (!HaveFailedWithSourceNotSupportedError()) {
>      return;
>    }
>  
> -  // If media doesn't start playing, we don't need to open it.

I can't see how removing this if statement matches the commit message "the element should be unable to play it and so should remain paused.".

We don't change the paused status no matter we return from here or not.

	Tzuhao Kuo [:kaku] Assignee
	Comment 97 • 8 years ago
mozreview-review-reply

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

https://reviewboard.mozilla.org/r/74938/#review76202

> Can you explain the purpose of this patch? And Why do we return here when it is blocked?

"An element is blocked" is different from "an element is not allowed to play". A blocked element suspends play operations and resumes them automatically later, however, if an element is not allowed to play, it discards play operations.

So, at patch 11, if an element is not allowed to play, I will return a rejected promise; if an element is blocked, I will return a pending promise.

I return here because this is where the promise has been created and before changing lots of internal states. Actually, we can move this check-and-return earlier to where just behind creating the promise.

	Tzuhao Kuo [:kaku] Assignee
	Comment 98 • 8 years ago
mozreview-review-reply

Comment on attachment 8789397 [details]
Bug 1244768 part 10 - OpenUnsupportedMediaWithExtenalAppIfNeeded() should not check whether the element is paused or not;

https://reviewboard.mozilla.org/r/77642/#review76206

> I can't see how removing this if statement matches the commit message "the element should be unable to play it and so should remain paused.".
> 
> We don't change the paused status no matter we return from here or not.

Oh, sorry, my fault.
> If a element is fed with unsupported content, the element should be unable to play it and so should remain paused.

This statement is talking about PlayInternal(). In PlayInternal(), if we find out that the mError has been set to be MEDIA_ERR_SRC_NOT_SUPPORTED, then it will call OpenUnsupportedMediaWithExtenalAppIfNeeded() and then return without setting the mPaused to be false.

So, in that case, the mPaused remains what it was, which could be true or false. That's why I remove the check here.

	Tzuhao Kuo [:kaku] Assignee
	Comment 99 • 8 years ago
mozreview-review-reply

Comment on attachment 8789397 [details]
Bug 1244768 part 10 - OpenUnsupportedMediaWithExtenalAppIfNeeded() should not check whether the element is paused or not;

https://reviewboard.mozilla.org/r/77642/#review76206

> Oh, sorry, my fault.
> > If a element is fed with unsupported content, the element should be unable to play it and so should remain paused.
> 
> This statement is talking about PlayInternal(). In PlayInternal(), if we find out that the mError has been set to be MEDIA_ERR_SRC_NOT_SUPPORTED, then it will call OpenUnsupportedMediaWithExtenalAppIfNeeded() and then return without setting the mPaused to be false.
> 
> So, in that case, the mPaused remains what it was, which could be true or false. That's why I remove the check here.

> In PlayInternal(), if we find out that the mError has been set to be MEDIA_ERR_SRC_NOT_SUPPORTED, then it will call OpenUnsupportedMediaWithExtenalAppIfNeeded() and then return without setting the mPaused to be false.

This is what is going to be at patch-11.

	JW Wang [:jwwang] [:jw_wang]
	Comment 100 • 8 years ago
mozreview-review

Comment on attachment 8789397 [details]
Bug 1244768 part 10 - OpenUnsupportedMediaWithExtenalAppIfNeeded() should not check whether the element is paused or not;

https://reviewboard.mozilla.org/r/77642/#review76224

::: dom/html/HTMLMediaElement.cpp
(Diff revision 1)
>  
>    if (!HaveFailedWithSourceNotSupportedError()) {
>      return;
>    }
>  
> -  // If media doesn't start playing, we don't need to open it.

It will then open an external app to play the video no matter this media element is paused or not. Doesn't that break the original intention where we only want to open it after starting playing?

	Boris Zbarsky [:bzbarsky]
	Comment 101 • 8 years ago
mozreview-review

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review76408

I wish I could comment directly on commit messages...

> implement utilities which are the fundation of

"foundation"

> (5) A method to asynchronously resolve all pending promises of a HTMLMediaElement.
> (6) A method to asynchronously reject all pending promises of a HTMLMediaElement.

These two don't seem to have an obvious spec counterpart, fwiw.  That's not a problem per se, of course.

> (7) A method to notify about playing event.

How about "A method to notify that we are not playing" or "A method to dispatch a 'playing' event", depending on whether it resolves the pending play promises.

> nsResolveOrRejectPendingPlayPromisesRunners

"nsResolveOrRejectPendingPlayPromisesRunner"?

::: dom/html/HTMLMediaElement.h:1736
(Diff revision 5)
> +  // A list of pending play promises. The elements are pushed during the play()
> +  // method call and are resolved/rejected during further playback steps.
> +  nsTArray<RefPtr<Promise>> mPendingPlayPromises;
> +
> +  // A list of already-dispatched nsResolveOrRejectPendingPlayPromisesRunners.
> +  // We keep tracing these tasks because the load algorithm resolves/rejects all

There is no tracing involved anywhere here.  Do you mean "tracking"?

It seems like this should be something like:

    // A list of already-dispatched but not yet run nsResolveOrRejectPendingPlayPromisesRunners.  Runners whose Run() method is called remove themselves from this list.  We keep track of these because the load algorithm ... [etc].
    
Probably also document why it's ok to hold weak refs (e.g. you assume that dispatch to main thread never fails and that once dispatched all things will be run).  Those assumptions may well not hold into the future....

::: dom/html/HTMLMediaElement.cpp:166
(Diff revision 5)
>  // Threshold above which audio is muted
>  static const double THRESHOLD_HIGH_PLAYBACKRATE_AUDIO = 4.0;
>  // Threshold under which audio is muted
>  static const double THRESHOLD_LOW_PLAYBACKRATE_AUDIO = 0.5;
>  
> +void ResolvePromisesWithUndefined(nsTArray<RefPtr<Promise>>&& aPromises)

Why does this take an rvalue ref as opposed to just a const lvalue ref, if it doesn't plan to mutate the object?

Also, this should be static or in an anonymous namespace...

::: dom/html/HTMLMediaElement.cpp:173
(Diff revision 5)
> +  for (auto& promise : aPromises) {
> +    promise->MaybeResolveWithUndefined();
> +  }
> +}
> +
> +void RejectPromises(nsTArray<RefPtr<Promise>>&& aPromises, nsresult aError)

Again, why rvalue ref?  And should be static.

::: dom/html/HTMLMediaElement.cpp:272
(Diff revision 5)
>    }
>  };
>  
> +/*
> + * If no error is passed while constructing an instance, the instance will
> + * resolve the passed promises with Undefined; otherwise, the instance will

"Undefined" is not a thing.  The JS value is called "undefined".

::: dom/html/HTMLMediaElement.cpp:276
(Diff revision 5)
> + * If no error is passed while constructing an instance, the instance will
> + * resolve the passed promises with Undefined; otherwise, the instance will
> + * reject the passed promises with the passed error.
> + *
> + * The constructor appends the constructed instance into the passed media
> + * element's mPendingPlayPromisesRunners member and once the the runner is ran

s/is ran/has run/

::: dom/html/HTMLMediaElement.cpp:277
(Diff revision 5)
> + * resolve the passed promises with Undefined; otherwise, the instance will
> + * reject the passed promises with the passed error.
> + *
> + * The constructor appends the constructed instance into the passed media
> + * element's mPendingPlayPromisesRunners member and once the the runner is ran
> + * (whether fulfilled or canceled), it removes itself out from the

s/out from the/from/

::: dom/html/HTMLMediaElement.cpp:299
(Diff revision 5)
> +  }
> +
> +  void ResolveOrReject()
> +  {
> +    if (NS_SUCCEEDED(mError)) {
> +      ResolvePromisesWithUndefined(Move(mPromises));

The Move() here does absolutely nothing.  Why is it being done?

::: dom/html/HTMLMediaElement.cpp:305
(Diff revision 5)
> +    } else {
> +      RejectPromises(Move(mPromises), mError);
> +    }
> +  }
> +
> +  NS_IMETHOD Run() override

The logic here seems overcomplicated.  Just do:

    if (!IsCanceled()) {
      ResolveOrReject();
    }
    mElement->mPendingPlayPromisesRunners.RemoveElement(this);

::: dom/html/HTMLMediaElement.cpp:335
(Diff revision 5)
> +    if (IsCancelled()) {
> +      mElement->mPendingPlayPromisesRunners.RemoveElement(this);
> +      return NS_OK;
> +    }
> +
> +    nsResolveOrRejectPendingPlayPromisesRunner::Run();

This looks wrong.  The event is supposed to fire before the promises resolve, no?  This can be detected in terms of the ordering of callbacks on promises resolved while the event fires with callbacks on the promises we have here...  Please add a test.

::: dom/html/HTMLMediaElement.cpp:806
(Diff revision 5)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mVideoTrackList)
>  #ifdef MOZ_EME
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMediaKeys)
>  #endif
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSelectedVideoStreamTrack)
> +  for (uint32_t i = 0; i < tmp->mPendingPlayPromises.Length(); ++i) {

Just:

    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingPlayPromises);
    
we have a traversal specialization for nsTArray.

::: dom/html/HTMLMediaElement.cpp:836
(Diff revision 5)
>    NS_IMPL_CYCLE_COLLECTION_UNLINK(mVideoTrackList)
>  #ifdef MOZ_EME
>    NS_IMPL_CYCLE_COLLECTION_UNLINK(mMediaKeys)
>  #endif
>    NS_IMPL_CYCLE_COLLECTION_UNLINK(mSelectedVideoStreamTrack)
> +  for (uint32_t i = 0; i < tmp->mPendingPlayPromises.Length(); ++i) {

Again, you can just:

    NS_IMPL_CYCLE_COLLECTION_UNLINK(mPendingPlayPromise);
    
which will just clear the array.

::: dom/html/HTMLMediaElement.cpp:6711
(Diff revision 5)
>  
> +nsTArray<RefPtr<Promise>>
> +HTMLMediaElement::TakePendingPlayPromises()
> +{
> +  nsTArray<RefPtr<Promise>> promises(Move(mPendingPlayPromises));
> +  mPendingPlayPromises.Clear();

Shouldn't it already be cleared by now?  I'd think you could just assert this.

In general, it seems like this function should just be:

    return Move(mPendingPlayPromises);

::: dom/html/HTMLMediaElement.cpp:6720
(Diff revision 5)
> +void
> +HTMLMediaElement::NotifyAboutPlaying()
> +{
> +  // Stick to the DispatchAsyncEvent() call path for now bacause there are some
> +  // side effects in the DispatchAsyncEvent() method.
> +  DispatchAsyncEvent(NS_LITERAL_STRING("playing"));

So this doesn't actually do the promise resolution bits, right?  Given that, it should probably be named somewhat better...

::: dom/html/HTMLMediaElement.cpp:6724
(Diff revision 5)
> +  // side effects in the DispatchAsyncEvent() method.
> +  DispatchAsyncEvent(NS_LITERAL_STRING("playing"));
> +}
> +
> +void
> +HTMLMediaElement::AsyncResolvePendingPlayPromises()

Header should document that it snapshots the promises to resolve when this method is called.

::: dom/html/HTMLMediaElement.cpp:6734
(Diff revision 5)
> +
> +  NS_DispatchToMainThread(event);
> +}
> +
> +void
> +HTMLMediaElement::AsyncRejectPendingPlayPromises(nsresult aError)

Header should document snapshotting behavior.

	Boris Zbarsky [:bzbarsky]
	Comment 102 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review76464

::: dom/html/HTMLMediaElement.cpp:3143
(Diff revision 5)
> +  //    rejected with a "NotSupportedError" DOMException and abort these steps.
> +  if (mError && mError->Code() == nsIDOMMediaError::MEDIA_ERR_SRC_NOT_SUPPORTED) {
> +    aRv.Throw(NS_ERROR_DOM_MEDIA_NOT_SUPPORTED_ERR);
> +
> +    // The check here is to handle the case that the media element starts playing
> +    // after it loaded fail. eg. preload the data before playing.

I'm not sure what "after it loaded fail" means....

::: dom/html/HTMLMediaElement.cpp:3199
(Diff revision 5)
>      mCurrentPlayRangeStart = CurrentTime();
>    }
>  
>    // Check if the element is now blocked.
>    if (IsPlayOperationBlocked()) {
> -    return;
> +    return promise.forget();

Will the promise get resolved or rejected when we get unblocked, then?

::: dom/html/HTMLMediaElement.cpp:3225
(Diff revision 5)
> +  // 6. If the media element's paused attribute is true, run the following
> +  //    substeps:
>    if (oldPaused) {
> +    // 6.1. Change the value of paused to false. (Already done.)
> +    // This step is uplifted because the "block-media-playback" feature needs
> +    // the mPaused to be flase before UpdateAudioChannelPlayingState() being

s/flase/false/ ?

::: dom/html/HTMLMediaElement.cpp:6094
(Diff revision 5)
>               mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE_DISPOSABLE);
>  
>    SetAudioChannelSuspended(nsISuspendedTypes::NONE_SUSPENDED);
>    ErrorResult rv;
> -  PlayInternal(rv);
> +  RefPtr<Promise> toBeIgnored = PlayInternal(rv);
> +  MOZ_ASSERT_IF(rv.Failed() && toBeIgnored,

You want the opposite assert: that if toBeIgnored && its state is rejected then rv.Failed().

r=me with the above fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 103 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review76466

	Tzuhao Kuo [:kaku] Assignee
	Comment 104 • 8 years ago

Comment on attachment 8785916 [details]
Bug 1244768 part 9 - distinguish the "element is not allowd to play" and "element is blocked" cases;

Cancel this review request which depends on bug 1302350.

	Tzuhao Kuo [:kaku] Assignee
	Comment 105 • 8 years ago

Comment on attachment 8789397 [details]
Bug 1244768 part 10 - OpenUnsupportedMediaWithExtenalAppIfNeeded() should not check whether the element is paused or not;

Cancel this review request which now depends on bug 1302362.

	Tzuhao Kuo [:kaku] Assignee
	Comment 106 • 8 years ago

@bz, thanks for your reviews! 
JW, Alastor and I decide to handle bug 1302350 and bug 1302362 first which will simplify patch 9 and patch 10. After that, I will modify patch 11 according to your comments!

	jason.ormand
	Comment 107 • 8 years ago

@kaku Thank you for your excellent work on this issue so far, I really appreciate it.

We (Vox Media) are in the process of swapping GIFs for Videos. Some browsers do not allow videos to autoplay inline (old Safari on iOS for example) and since you can not programmatically ask the video element if autoplay is supported ... the strategy we are using is "if video.play() returns promise, try to play, replace with GIF on error".

Other strategies tend to involve base64 encoded video files (format support dependent) which tends to blow up on IE/Edge https://github.com/Modernizr/Modernizr/issues/1619 and there are various incantations of that "test" all of which are bloated and hacky.

So it's really important to us to have .play() return a promise as the spec and docs claim. This would mean huge bandwidth reductions, faster loading pages, smoother playback experience and less jank. Everyone wins when we replace GIF's with looping videos. Really look forward to Firefox support for the .play() promise.

	Tzuhao Kuo [:kaku] Assignee
	Comment 108 • 8 years ago

@Jason,

Thanks so mush for letting us know your situation, we really want to know what the Web application developers need and we appreciate for your inputs, much more are definitely welcome!

This bug was blocked by other refactoring works, so I switched to other features for a while, however, since the blocker has already been resolved, I should be able to continue working on this soon.

	jason.ormand
	Comment 109 • 8 years ago

@kaku Excellent! Thank you for the quick and supportive response. I do try to log bugs / give suggestions to browser vendors when something comes up. I'm glad it's well received.

I think this particular issue, if resolved, would allow others to do the same thing we are doing with GIFv ... with relative confidence of only using fallback GIFs when they are truly necessary and not just the result of a browser bug / missing feature. I believe Edge may have the same issue, if it does I'll file with them as well.

I plan on doing a write up post and maybe a podcast on GIFs as videos which I hope more sites will do. Just imagine the global impact of just a few major sites doing this. Easily petabyte scale byte savings over just a few sites in relatively short time. Giphy / Twitter and some others already do this but most folks still use plain old browser crushing GIFs. 

We use Thumbor, and open source image processing application to do our GIF to Video transformations and it's quite painless. So I am really hoping to set a good example with this feature and let people know how they can do it too.

Thanks again! I've very excited to see this fix get into stable.

	Tzuhao Kuo [:kaku] Assignee
	Comment 110 • 8 years ago
mozreview-review-reply

Comment on attachment 8779208 [details]
Bug 1244768 part 1 - implement utilities which are the foundation of promise-based play operation;

https://reviewboard.mozilla.org/r/70246/#review76408

> So this doesn't actually do the promise resolution bits, right?  Given that, it should probably be named somewhat better...

It does resolve the pending promises.   `DispatchAsyncEvent(NS_LITERAL_STRING("playing"))` creates nsNotifyAboutPlayingRunner which resolves all pending promises.

	JW Wang [:jwwang] [:jw_wang]
	Comment 121 • 8 years ago
mozreview-review

Comment on attachment 8814028 [details]
Bug 1244768 part 10 - enable web-platform test;

https://reviewboard.mozilla.org/r/95338/#review95492

	Tzuhao Kuo [:kaku] Assignee
	Comment 122 • 8 years ago

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

@JW, since the block-media-element feature is refactored in Bug 1302350, could you please review this patch again? In specific, please review the already_AddRefed<Promise> HTMLMediaElement::Play(ErrorResult& aRv); method, thanks.

	Tzuhao Kuo [:kaku] Assignee
	Comment 123 • 8 years ago

Try: bug 1301426 comment 57

	JW Wang [:jwwang] [:jw_wang]
	Comment 124 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review96390

::: dom/html/HTMLMediaElement.cpp:3136
(Diff revision 6)
> +  // run the following steps.
> +
> +  // 4.8.12.8 - Step 1:
> +  // If the media element is not allowed to play, return a promise rejected
> +  // with a "NotAllowedError" DOMException and abort these steps.
>    if (!IsAllowedToPlay()) {

It looks like our IsAllowedToPlay() doesn't match the spec.

When spec says "not allowed to play", we should return a rejected promise immediately.

However, when IsAllowedToPlay() returns false, we might still be able to play later depending on the value of mAudioChannelSuspended.

Please discuss with Alastor how to separate "not allowed to play" (spec. code) from a blocked media element (policy code).

We want to move spec. code into PlayInternal() and leave policy code here.

::: dom/html/HTMLMediaElement.cpp:3157
(Diff revision 6)
> +
> +    aRv.Throw(NS_ERROR_DOM_MEDIA_NOT_ALLOWED_ERR);
> +    return nullptr;
> +  }
> +
> +  // 4.8.12.8 - Step 2:

We want to separate policy code from spec.code. Please move spec-related code into PlayInternal().

::: dom/html/HTMLMediaElement.cpp:3211
(Diff revision 6)
>      }
>      if (!mPausedForInactiveDocumentOrChannel) {
>        nsresult rv = mDecoder->Play();
>        if (NS_FAILED(rv)) {
>          aRv.Throw(rv);
> -        return;
> +        mPendingPlayPromises.RemoveElement(promise);

It is awkward to remove the element immediately after the insertion when errored.

We can move |mPendingPlayPromises.AppendElement(promise)| below the |if (mDecoder)| block to avoid that.

	Tzuhao Kuo [:kaku] Assignee
	Comment 126 • 8 years ago
mozreview-review-reply

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review96390

> It is awkward to remove the element immediately after the insertion when errored.
> 
> We can move |mPendingPlayPromises.AppendElement(promise)| below the |if (mDecoder)| block to avoid that.

Agree with that the code is awkward, however since the |MaybeDoLoad();| function call, between the promise creation and |if (mDecoder)|, might operate on the mPendingPlayPromises so I prefere keep the code as it is now. WDYT?

	JW Wang [:jwwang] [:jw_wang]
	Comment 127 • 8 years ago
mozreview-review-reply

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review96390

> Agree with that the code is awkward, however since the |MaybeDoLoad();| function call, between the promise creation and |if (mDecoder)|, might operate on the mPendingPlayPromises so I prefere keep the code as it is now. WDYT?

Who will touch mPendingPlayPromises in between?

	Tzuhao Kuo [:kaku] Assignee
	Comment 128 • 8 years ago
mozreview-review-reply

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review96390

> Who will touch mPendingPlayPromises in between?

MaybeDoLoad(); -> DoLoad(); -> AbortExistingLoads(); (https://hg.mozilla.org/try/rev/d7ae3aef956d#l1.15)

	JW Wang [:jwwang] [:jw_wang]
	Comment 129 • 8 years ago
mozreview-review-reply

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review96390

> MaybeDoLoad(); -> DoLoad(); -> AbortExistingLoads(); (https://hg.mozilla.org/try/rev/d7ae3aef956d#l1.15)

Then we shouldn't add the promise to mPendingPlayPromises because it might be accidentally resolved or rejected in-between.

	Tzuhao Kuo [:kaku] Assignee
	Comment 140 • 8 years ago

Try: bug 1301426 comment 78

	JW Wang [:jwwang] [:jw_wang]
	Comment 141 • 8 years ago
mozreview-review

Comment on attachment 8779215 [details]
Bug 1244768 part 9 - modify the play() method;

https://reviewboard.mozilla.org/r/70260/#review97358

	Tzuhao Kuo [:kaku] Assignee
	Comment 152 • 8 years ago

Rebasing onto bug 1321497.

Try: bug 1301426 comment 98

	Tzuhao Kuo [:kaku] Assignee
	Comment 153 • 8 years ago

Try fail: https://treeherder.mozilla.org/logviewer.html#?job_id=32399027&repo=try#L10368

The HTMLMediaElement::AsyncRejectPendingPlayPromises() might dispatch events while shutting the XPCOM.

	Tzuhao Kuo [:kaku] Assignee
	Comment 164 • 8 years ago

Try:  bug 1301426 comment 118

	Tzuhao Kuo [:kaku] Assignee
	Comment 165 • 8 years ago

Try looks good! Thanks for reviews!

	Pulsebot
	Comment 166 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b47c134e5e2
part 1 - implement utilities which are the foundation of promise-based play operation; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/2a13aa8e9a76
part 2 - modify media element load algorith; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/b8c156c5cff8
part 3 - modify the resource selection algorithm; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fafd16dfeac9
part 4 - call NotifyAboutPlaying() while ready state is changed; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/391d56cd85f7
part 5 - reject pending play promises while the playback reaching the end; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ed3195c64abc
part 6 - modify the internal pause steps; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/23f644a03212
part 7 - refactor the Play() and PlayInternal() methods; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/d0869fbb0870
part 8 - extract the HTMLMediaElement::CreateDOMPromise() utility; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/9591466c9f6a
part 9 - modify the play() method; r=alwu,bz,jwwang
https://hg.mozilla.org/integration/autoland/rev/9d916df904a3
part 10 - enable web-platform test; r=jwwang

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 167 • 8 years ago

How did this land without any tests? Last I checked, we've had a policy against that for a decade now...

	Tzuhao Kuo [:kaku] Assignee
	Comment 168 • 8 years ago

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #167)
> How did this land without any tests? Last I checked, we've had a policy
> against that for a decade now...

Although it's somewhat weird, I separated the test cases into bug 1301426 for not messing up the review request of this bug.

	Phil Ringnalda (:philor)
	Comment 169 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1b47c134e5e2
https://hg.mozilla.org/mozilla-central/rev/2a13aa8e9a76
https://hg.mozilla.org/mozilla-central/rev/b8c156c5cff8
https://hg.mozilla.org/mozilla-central/rev/fafd16dfeac9
https://hg.mozilla.org/mozilla-central/rev/391d56cd85f7
https://hg.mozilla.org/mozilla-central/rev/ed3195c64abc
https://hg.mozilla.org/mozilla-central/rev/23f644a03212
https://hg.mozilla.org/mozilla-central/rev/d0869fbb0870
https://hg.mozilla.org/mozilla-central/rev/9591466c9f6a
https://hg.mozilla.org/mozilla-central/rev/9d916df904a3

	jason.ormand
	Comment 170 • 8 years ago

Thanks!! I look forward to v53/Q2 : )

	Tzuhao Kuo [:kaku] Assignee
	Comment 171 • 8 years ago

(In reply to jason.ormand from comment #170)
> Thanks!! I look forward to v53/Q2 : )
Ha, there is still chance to be backed out..., whatever, I guess that you have already know that you could start testing this feature via the Firefox Nightly channel now. If you encountered any problem/bug, please let us know, I will be very happy to handle it. 

Nightly: https://www.mozilla.org/en-US/firefox/channel/desktop/

	jason.ormand
	Comment 172 • 8 years ago

Just ran some functional tests against our responsive GIF to Video (aka GIFv) implementation on FF Nightly and *it worked perfectly*. Which gives you a one up on Chrome which has had play()/pause() race condition issues since they launched promises. Anyhow, looks good from my side. So whenever this makes it into stable we will ship videos to Firefox instead of GIFs.

Thanks again : )

	Eric Shepherd [:sheppy]
	Comment 174 • 7 years ago

The doc already covered this, but had some style issues and was missing an example. Those issues are resolved, so marking dev-doc-complete.