Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error handling (builds on #1191) #1197

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .jshintrc
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"start",
"stop",
"strictEqual",
"test"
"test",
"sinon"
]
}
2 changes: 1 addition & 1 deletion Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ module.exports = function(grunt) {
},
tests: {
src: ['build/files/combined.video.js', 'build/compiler/goog.base.js', 'src/js/exports.js', 'test/unit/*.js'],
externs: ['src/js/player.externs.js', 'src/js/media/flash.externs.js', 'test/qunit-externs.js'],
externs: ['src/js/player.externs.js', 'src/js/media/flash.externs.js', 'test/qunit-externs.js', 'test/sinon-externs.js'],
dest: 'build/files/test.minified.video.js'
}
},
Expand Down
2 changes: 2 additions & 0 deletions build/source-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var sourceFiles = [
"src/js/button.js",
"src/js/slider.js",
"src/js/menu.js",
"src/js/media-error.js",
"src/js/player.js",
"src/js/control-bar/control-bar.js",
"src/js/control-bar/live-display.js",
Expand All @@ -37,6 +38,7 @@ var sourceFiles = [
"src/js/poster.js",
"src/js/loading-spinner.js",
"src/js/big-play-button.js",
"src/js/error-display.js",
"src/js/media/media.js",
"src/js/media/html5.js",
"src/js/media/flash.js",
Expand Down
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
},
"devDependencies": {
"grunt-cli": "~0.1.0",
"grunt": "~0.4",
"grunt": "0.4.2",
"grunt-contrib-connect": "~0.7.1",
"grunt-contrib-jshint": "~0.4.3",
"grunt-contrib-watch": "~0.1.4",
Expand Down Expand Up @@ -57,6 +57,7 @@
"grunt-tagrelease": "~0.3.3",
"github": "~0.1.14",
"open": "0.0.4",
"grunt-version": "~0.3.0"
"grunt-version": "~0.3.0",
"sinon": "~1.9.1"
}
}
72 changes: 72 additions & 0 deletions src/css/video-js.less
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,11 @@ The default control bar that is a container for most of the controls.
display: none;
}

/* The control bar shouldn't show after an error */
.vjs-default-skin.vjs-error .vjs-control-bar {
display: none;
}

/* IE8 is flakey with fonts, and you have to change the actual content to force
fonts to show/hide properly.
- "\9" IE8 hack didn't work for this
Expand Down Expand Up @@ -543,6 +548,61 @@ easily in the skin designer. http://designer.videojs.com/
height: 100%;
}

.vjs-error .vjs-big-play-button {
display: none;
}

/* Error Display
--------------------------------------------------------------------------------
*/

.vjs-error-display {
display: none;
}

.vjs-error .vjs-error-display {
display: block;
position: absolute;
left: 0;
top: 0;
width: 100%;
height: 100%;
}

.vjs-error .vjs-error-display:before {
// content: @play-icon;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nit picking I know, but do we want these comments here for content and font?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

// font-family: VideoJS;
content: 'X';
font-family: Arial;
font-size: 4em;
color: #666666;
/* In order to center the play icon vertically we need to set the line height
to the same as the button height */
line-height: 1;
text-shadow: 0.05em 0.05em 0.1em #000;
text-align: center /* Needed for IE8 */;
vertical-align: middle;

position: absolute;
top: 50%;
margin-top: -0.5em;
width: 100%;
}

.vjs-error-display div {
position: absolute;

font-size: 1.4em;
text-align: center;
bottom: 1em;
right: 1em;
left: 1em;
}

.vjs-error-display a, .vjs-error-display a:visited {
color: #F4A460;
}

/* Loading Spinner
--------------------------------------------------------------------------------
*/
Expand All @@ -566,6 +626,18 @@ easily in the skin designer. http://designer.videojs.com/
.animation(spin 1.5s infinite linear);
}

/* Errors are unrecoverable without user interaction,
so hide the spinner in the case of an error */
.video-js.vjs-error .vjs-loading-spinner {
/* using !important flag because currently the loading spinner
uses hide()/show() instead of classes. The !important can be
removed when that's updated */
display: none !important;

/* ensure animation doesn't continue while hidden */
.animation(none);
}

.vjs-default-skin .vjs-loading-spinner:before {
content: @spinner3-icon;
font-family: VideoJS;
Expand Down
8 changes: 3 additions & 5 deletions src/js/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,12 @@ vjs.options = {
'textTrackDisplay': {},
'loadingSpinner': {},
'bigPlayButton': {},
'controlBar': {}
'controlBar': {},
'errorDisplay': {}
},

// Default message to show when a video cannot be played.
'notSupportedMessage': 'Sorry, no compatible source and playback ' +
'technology were found for this video. Try using another browser ' +
'like <a href="http://bit.ly/ccMUEC">Chrome</a> or download the ' +
'latest <a href="http://adobe.ly/mwfN1">Adobe Flash Player</a>.'
'notSupportedMessage': 'No compatible source was found for this video.'
};

// Set CDN Version of swf
Expand Down
31 changes: 31 additions & 0 deletions src/js/error-display.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Display that an error has occurred making the video unplayable
* @param {vjs.Player|Object} player
* @param {Object=} options
* @constructor
*/
vjs.ErrorDisplay = vjs.Component.extend({
init: function(player, options){
vjs.Component.call(this, player, options);

this.update();
player.on('error', vjs.bind(this, this.update));
}
});

vjs.ErrorDisplay.prototype.createEl = function(){
var el = vjs.Component.prototype.createEl.call(this, 'div', {
className: 'vjs-error-display'
});

this.contentEl_ = vjs.createEl('div');
el.appendChild(this.contentEl_);

return el;
};

vjs.ErrorDisplay.prototype.update = function(){
if (this.player().error()) {
this.contentEl_.innerHTML = this.player().error().message;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our side conversation, curious if this can be overridden the case of a custom errors plugin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it just be a matter of the message property on the object given to player.error?

try {
 // something that throws some error
} catch (e) {
  player.error({
    code: 42,
    message: "Error 42 happened. We still don't know the question, though."
  });
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think Tom and I got this figured out in chat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, good to have this here then for future purposes.

}
};
1 change: 1 addition & 0 deletions src/js/exports.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ goog.exportSymbol('videojs.DurationDisplay', vjs.DurationDisplay);
goog.exportSymbol('videojs.TimeDivider', vjs.TimeDivider);
goog.exportSymbol('videojs.RemainingTimeDisplay', vjs.RemainingTimeDisplay);
goog.exportSymbol('videojs.LiveDisplay', vjs.LiveDisplay);
goog.exportSymbol('videojs.ErrorDisplay', vjs.ErrorDisplay);
goog.exportSymbol('videojs.Slider', vjs.Slider);
goog.exportSymbol('videojs.ProgressControl', vjs.ProgressControl);
goog.exportSymbol('videojs.SeekBar', vjs.SeekBar);
Expand Down
74 changes: 67 additions & 7 deletions src/js/lib.js
Original file line number Diff line number Diff line change
Expand Up @@ -656,14 +656,74 @@ vjs.getAbsoluteURL = function(url){
return url;
};

// usage: log('inside coolFunc',this,arguments);
// http://paulirish.com/2009/log-a-lightweight-wrapper-for-consolelog/
vjs.log = function(){
vjs.log.history = vjs.log.history || []; // store logs to an array for reference
vjs.log.history.push(arguments);
if(window.console){
window.console.log(Array.prototype.slice.call(arguments));
// if there's no console then don't try to output messages
// they will still be stored in vjs.log.history
var _noop = function(){};
var _console = window['console'] || {
'log': _noop,
'warn': _noop,
'error': _noop
};

/**
* Log messags to the console and history based on the type of message
*
* @param {String} type The type of message, or `null` for `log`
* @param {[type]} args The args to be passed to the log
* @private
*/
function _logType(type, args){
// convert args to an array to get array functions
var argsArray = Array.prototype.slice.call(args);

if (type) {
// add the type to the front of the message
argsArray.unshift(type.toUpperCase()+':');
} else {
// default to log with no prefix
type = 'log';
}

// add to history
vjs.log.history.push(argsArray);

// add console prefix after adding to history
argsArray.unshift('VIDEOJS:');

// call appropriate log function
if (_console[type].apply) {
_console[type].apply(_console, argsArray);
} else {
// ie8 doesn't allow error.apply, but it will just join() the array anyway
_console[type](argsArray.join(' '));
}
}

/**
* Log plain debug messages
*/
vjs.log = function(){
_logType(null, arguments);
};

/**
* Keep a history of log messages
* @type {Array}
*/
vjs.log.history = [];

/**
* Log error messages
*/
vjs.log.error = function(){
_logType('error', arguments);
};

/**
* Log warning messages
*/
vjs.log.warn = function(){
_logType('warn', arguments);
};

// Offset Left
Expand Down
1 change: 0 additions & 1 deletion src/js/loading-spinner.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ vjs.LoadingSpinner = vjs.Component.extend({
// 'seeking' event
player.on('seeked', vjs.bind(this, this.hide));

player.on('error', vjs.bind(this, this.show));
player.on('ended', vjs.bind(this, this.hide));

// Not showing spinner on stalled any more. Browsers may stall and then not trigger any events that would remove the spinner.
Expand Down
69 changes: 69 additions & 0 deletions src/js/media-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/**
* Custom MediaError to mimic the HTML5 MediaError
* @param {Number} code The media error code
*/
vjs.MediaError = function(code){
if (typeof code == 'number') {
this.code = code;
} else if (typeof code == 'string') {
// default code is zero, so this is a custom error
this.message = code;
} else if (typeof code == 'object') { // object
vjs.obj.merge(this, code);
}

if (!this.message) {
this.message = vjs.MediaError.defaultMessages[this.code] || '';
}
};

/**
* The error code that refers two one of the defined
* MediaError types
* @type {Number}
*/
vjs.MediaError.prototype.code = 0;

/**
* An optional message to be shown with the error.
* Message is not part of the HTML5 video spec
* but allows for more informative custom errors.
* @type {String}
*/
vjs.MediaError.prototype.message = '';

/**
* An optional status code that can be set by plugins
* to allow even more detail about the error.
* For example the HLS plugin might provide the specific
* HTTP status code that was returned when the error
* occurred, then allowing a custom error overlay
* to display more information.
* @type {[type]}
*/
vjs.MediaError.prototype.status = null;

vjs.MediaError.errorTypes = [
'MEDIA_ERR_CUSTOM', // = 0
'MEDIA_ERR_ABORTED', // = 1
'MEDIA_ERR_NETWORK', // = 2
'MEDIA_ERR_DECODE', // = 3
'MEDIA_ERR_SRC_NOT_SUPPORTED', // = 4
'MEDIA_ERR_ENCRYPTED' // = 5
];

vjs.MediaError.defaultMessages = {
1: 'You aborted the video playback',
2: 'A network error caused the video download to fail part-way.',
3: 'The video playback was aborted due to a corruption problem or because the video used features your browser did not support.',
4: 'The video could not be loaded, either because the server or network failed or because the format is not supported.',
5: 'The video is encrypted and we do not have the keys to decrypt it.'
};

// Add types as properties on MediaError
// e.g. MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED = 4;
for (var errNum = 0; errNum < vjs.MediaError.errorTypes.length; errNum++) {
vjs.MediaError[vjs.MediaError.errorTypes[errNum]] = errNum;
// values should be accessible on both the class and instance
vjs.MediaError.prototype[vjs.MediaError.errorTypes[errNum]] = errNum;
}
11 changes: 9 additions & 2 deletions src/js/media/flash.js
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,15 @@ vjs.Flash['onEvent'] = function(swfID, eventName){
// Log errors from the swf
vjs.Flash['onError'] = function(swfID, err){
var player = vjs.el(swfID)['player'];
player.trigger('error');
vjs.log('Flash Error', err, swfID);
var msg = 'FLASH: '+err;

if (err == 'srcnotfound') {
player.error({ code: 4, message: msg });

// errors we haven't categorized into the media errors
} else {
player.error(msg);
}
};

// Flash Version Check
Expand Down
Loading