fix: Wait for loadstart when changing sources, instead of just ready. #4743
Conversation
src/js/player.js
Outdated
|
|
||
| // Only calls the tech's play if we already have a src loaded | ||
| } else if (this.isReady_ && (this.src() || this.currentSrc())) { | ||
| // If the tech is not ready, queue up another call to `play()` for when it |
There was a problem hiding this comment.
The key change in all this is that we kind of have a "checklist" of conditions before we can attempt playback.
First, we need the tech to be ready before anything else. When that's satisfied, play() gets called again.
Second, we need to make sure middleware isn't changing sources. When that's satisfied by a loadstart, we call play() again.
Finally, both conditions should be satisfied, so we can attempt playback if there's a source.
misteroneill
force-pushed
the
ready-play
branch
from
1d5920a
to
aabe9eb
Compare
|
|
||
| this.playWaitingForReady_ = true; | ||
| this.ready(() => { | ||
| this.playWaitingForReady_ = false; |
There was a problem hiding this comment.
can we set this in the constructor to a default value as well?
src/js/player.js
Outdated
|
|
||
| // Finally, if we have a source, we can attempt playback for real. | ||
| } else if (this.src() || this.currentSrc()) { | ||
| return this.techGet_('play'); | ||
| } |
There was a problem hiding this comment.
is it possible that we end up in an else condition?
There was a problem hiding this comment.
I've been trying to think of one that isn't captured by the previous two statements, but I haven't been able to. It probably makes sense to reverse this conditional block so the ready call comes in the else block.
There was a problem hiding this comment.
Actually, yeah, the only condition that we'd have that would be an else would be if the tech is ready, we're not changing sources, and we don't have a source.
|
|
||
| playPromise.then(playFocus, ignoreRejectedPlayPromise); | ||
| if (isPromise(playPromise)) { | ||
| playPromise.then(playFocus, () => {}); |
There was a problem hiding this comment.
why not import silencePromise and use that?
There was a problem hiding this comment.
Copy/paste from my other PR, good call.
There was a problem hiding this comment.
Oh, actually, we can't because it's not silencing it.
There was a problem hiding this comment.
It isn't? What the difference between ignoreRejectedPlayPromise and silencePromise?
Oh, I see. We are silencing the playPromise but we're also adding a playFocus handler.
There was a problem hiding this comment.
we could potentially do something like:
if (isPromise(playPromise)) {
silencePromise(playPromise);
playPromise.then(playFocus);
}There was a problem hiding this comment.
Seems like overkill...
There was a problem hiding this comment.
yeah, either way. using silencePromise is probably extra explicit/clear but either is fine.
test/unit/utils/promise.test.js
Outdated
| }); | ||
|
|
||
| QUnit.test('can identify a Promise-like object', function(assert) { | ||
| assert.notOk(promise.isPromise({})); |
test/unit/player-play.test.js
Outdated
|
|
||
| beforeEach() { | ||
| this.clock = sinon.useFakeTimers(); | ||
| this.player = TestHelpers.makePlayer({techOrder: ['html5']}); |
There was a problem hiding this comment.
if the html5 tech is required for testing, we'd want to wrap this file in a !IS_IE8.
There was a problem hiding this comment.
I don't think it actually is. It can probably use the fake tech.
The core goal here is to make sure the following works in light of some middleware process that makes setting the source more async than next tick:
In fact, given this change, we should even be able to do:
Unlike #4665, which would have clarified/changed the meaning of "ready", it remains a reflection of the tech's state and we make better use of the ability to queue things on that state and on the middleware
setSourceprocess.Requirements Checklist