Permalink
Browse files

fix(Player#play): Wait for loadstart in play() when changing sources …

…instead of just ready. (#4743)

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:

```js
player.src('...');
player.ready(() => player.play());
```

In fact, given this change, we should even be able to do:

```js
player.src('...');
player.play();
```

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 `setSource` process.
  • Loading branch information...
misteroneill authored and gkatsev committed Nov 16, 2017
1 parent 6cbe3ed commit 26b0d2cadd3dd5d45d3e87a396ff982b83db4ab0
Showing with 198 additions and 34 deletions.
  1. +5 −9 package-lock.json
  2. +3 −4 src/js/big-play-button.js
  3. +42 −21 src/js/player.js
  4. +28 −0 src/js/utils/promise.js
  5. +100 −0 test/unit/player-play.test.js
  6. +20 −0 test/unit/utils/promise.test.js
View

Some generated files are not rendered by default. Learn more.

Oops, something went wrong.
@@ -3,6 +3,7 @@
*/
import Button from './button.js';
import Component from './component.js';
import {isPromise} from './utils/promise';
/**
* The initial play button that shows before the video has played. The hiding of the
@@ -58,10 +59,8 @@ class BigPlayButton extends Button {
const playFocus = () => playToggle.focus();
if (playPromise && playPromise.then) {
const ignoreRejectedPlayPromise = () => {};
playPromise.then(playFocus, ignoreRejectedPlayPromise);
if (isPromise(playPromise)) {
playPromise.then(playFocus, () => {});
} else {
this.setTimeout(playFocus, 1);
}
View
@@ -24,6 +24,7 @@ import MediaError from './media-error.js';
import safeParseTuple from 'safe-json-parse/tuple';
import {assign} from './utils/obj';
import mergeOptions from './utils/merge-options.js';
import {silencePromise} from './utils/promise';
import textTrackConverter from './tracks/text-track-list-converter.js';
import ModalDialog from './modal-dialog';
import Tech from './tech/tech.js';
@@ -468,6 +469,8 @@ class Player extends Component {
this.on('stageclick', this.handleStageClick_);
this.changingSrc_ = false;
this.playWaitingForReady_ = false;
this.playOnLoadstart_ = null;
}
/**
@@ -1665,37 +1668,55 @@ class Player extends Component {
}
/**
* start media playback
* Attempt to begin playback at the first opportunity.
*
* @return {Promise|undefined}
* Returns a `Promise` if the browser returns one, for most browsers this will
* return undefined.
* Returns a `Promise` only if the browser returns one and the player
* is ready to begin playback. For some browsers and all non-ready
* situations, this will return `undefined`.
*/
play() {
if (this.changingSrc_) {
this.ready(function() {
const retval = this.techGet_('play');
// silence errors (unhandled promise from play)
if (retval !== undefined && typeof retval.then === 'function') {
retval.then(null, (e) => {});
}
// If this is called while we have a play queued up on a loadstart, remove
// that listener to avoid getting in a potentially bad state.
if (this.playOnLoadstart_) {
this.off('loadstart', this.playOnLoadstart_);
}
// If the player/tech is not ready, queue up another call to `play()` for
// when it is. This will loop back into this method for another attempt at
// playback when the tech is ready.
if (!this.isReady_) {
// Bail out if we're already waiting for `ready`!
if (this.playWaitingForReady_) {
return;
}
this.playWaitingForReady_ = true;
this.ready(() => {
this.playWaitingForReady_ = false;
silencePromise(this.play());
});
// Only calls the tech's play if we already have a src loaded
} else if (this.isReady_ && (this.src() || this.currentSrc())) {
// If the player/tech is ready and we have a source, we can attempt playback.
} else if (!this.changingSrc_ && (this.src() || this.currentSrc())) {
return this.techGet_('play');
// If the tech is ready, but we do not have a source, we'll need to wait
// for both the `ready` and a `loadstart` when the source is finally
// resolved by middleware and set on the player.
//
// This can happen if `play()` is called while changing sources or before
// one has been set on the player.
} else {
this.ready(function() {
this.tech_.one('loadstart', function() {
const retval = this.play();
// silence errors (unhandled promise from play)
if (retval !== undefined && typeof retval.then === 'function') {
retval.then(null, (e) => {});
}
});
});
this.playOnLoadstart_ = () => {
this.playOnLoadstart_ = null;
silencePromise(this.play());
};
this.one('loadstart', this.playOnLoadstart_);
}
}
View
@@ -0,0 +1,28 @@
/**
* Returns whether an object is `Promise`-like (i.e. has a `then` method).
*
* @param {Object} value
* An object that may or may not be `Promise`-like.
*
* @return {Boolean}
* Whether or not the object is `Promise`-like.
*/
export function isPromise(value) {
return value !== undefined && typeof value.then === 'function';
}
/**
* Silence a Promise-like object.
*
* This is useful for avoiding non-harmful, but potentially confusing "uncaught
* play promise" rejection error messages.
*
* @param {Object} value
* An object that may or may not be `Promise`-like.
*/
export function silencePromise(value) {
if (isPromise(value)) {
value.then(null, (e) => {});
}
}
@@ -0,0 +1,100 @@
/* eslint-env qunit */
import sinon from 'sinon';
import {silencePromise} from '../../src/js/utils/promise';
import TestHelpers from './test-helpers';
QUnit.module('Player#play', {
beforeEach() {
this.clock = sinon.useFakeTimers();
this.player = TestHelpers.makePlayer({});
this.techPlayCallCount = 0;
this.player.tech_.play = () => {
this.techPlayCallCount++;
};
},
afterEach() {
this.player.dispose();
this.clock.restore();
}
});
QUnit.test('tech not ready + no source = wait for ready, then loadstart', function(assert) {
// Mock the player/tech not being ready.
this.player.isReady_ = false;
// Attempt to play.
this.player.play();
this.clock.tick(100);
assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because the tech was not ready');
// Ready the player.
this.player.triggerReady();
this.clock.tick(100);
assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because there was no source');
// Add a source and trigger loadstart.
this.player.src('xyz.mp4');
this.clock.tick(100);
this.player.trigger('loadstart');
assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called');
});
QUnit.test('tech not ready + has source = wait for ready', function(assert) {
// Mock the player/tech not being ready, but having a source.
this.player.isReady_ = false;
this.player.src('xyz.mp4');
this.clock.tick(100);
// Attempt to play.
this.player.play();
this.clock.tick(100);
assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because the tech was not ready');
// Ready the player.
this.player.triggerReady();
this.clock.tick(100);
assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called');
});
QUnit.test('tech ready + no source = wait for loadstart', function(assert) {
// Attempt to play.
this.player.play();
this.clock.tick(100);
assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because the tech was not ready');
// Add a source and trigger loadstart.
this.player.src('xyz.mp4');
this.clock.tick(100);
this.player.trigger('loadstart');
assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called');
});
QUnit.test('tech ready + has source = play immediately!', function(assert) {
// Mock the player having a source.
this.player.src('xyz.mp4');
this.clock.tick(100);
// Attempt to play, but silence the promise that might be returned.
silencePromise(this.player.play());
assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called');
});
QUnit.test('tech ready + has source + changing source = wait for loadstart', function(assert) {
// Mock the player having a source and in the process of changing its source.
this.player.src('xyz.mp4');
this.clock.tick(100);
this.player.src('abc.mp4');
this.player.play();
this.clock.tick(100);
assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because the source was changing');
this.player.trigger('loadstart');
assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called');
});
@@ -0,0 +1,20 @@
/* eslint-env qunit */
import window from 'global/window';
import * as promise from '../../../src/js/utils/promise';
QUnit.module('utils/promise');
QUnit.test('can correctly identify a native Promise (if supported)', function(assert) {
// If Promises aren't supported, skip this.
if (!window.Promise) {
return assert.expect(0);
}
assert.ok(promise.isPromise(new window.Promise((resolve) => resolve())), 'a native Promise was recognized');
});
QUnit.test('can identify a Promise-like object', function(assert) {
assert.notOk(promise.isPromise({}), 'an object without a `then` method is not Promise-like');
assert.ok(promise.isPromise({then: () => {}}), 'an object with a `then` method is Promise-like');
});

0 comments on commit 26b0d2c

Please sign in to comment.