From d2a930c9beea0007aaa3a9c2d9807567bab931db Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 24 Nov 2015 14:11:01 -0500 Subject: [PATCH 1/4] Close the error display when clearing errors --- src/js/player.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/js/player.js b/src/js/player.js index 15dc5cee8d..91d28d61f1 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -2107,6 +2107,7 @@ class Player extends Component { if (err === null) { this.error_ = err; this.removeClass('vjs-error'); + this.errorDisplay.close(); return this; } From db26dd02cd4a9d501215a814fe3255e586b9a7c9 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 24 Nov 2015 14:11:09 -0500 Subject: [PATCH 2/4] Trigger error last in error function Triggers are synchronous so, if you are trying to disable the error in an error handler, you end up throwing an error because the log line refers to this.error_ which would then get cleared up when error(null) is called. --- src/js/player.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/js/player.js b/src/js/player.js index 91d28d61f1..ca24b008c3 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -2118,9 +2118,6 @@ class Player extends Component { this.error_ = new MediaError(err); } - // fire an error event on the player - this.trigger('error'); - // add the vjs-error classname to the player this.addClass('vjs-error'); @@ -2128,6 +2125,9 @@ class Player extends Component { // ie8 just logs "[object object]" if you just log the error object log.error(`(CODE:${this.error_.code} ${MediaError.errorTypes[this.error_.code]})`, this.error_.message, this.error_); + // fire an error event on the player + this.trigger('error'); + return this; } From 9e0dd29391449f78740a37f304edeb03fd66ce1f Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 24 Nov 2015 14:12:33 -0500 Subject: [PATCH 3/4] remove dead code --- test/unit/player.test.js | 70 ---------------------------------------- 1 file changed, 70 deletions(-) diff --git a/test/unit/player.test.js b/test/unit/player.test.js index 5298161e1b..ee12f21a1f 100644 --- a/test/unit/player.test.js +++ b/test/unit/player.test.js @@ -17,36 +17,6 @@ q.module('Player', { } }); -// Compiler doesn't like using 'this' in setup/teardown. -// module("Player", { -// /** -// * @this {*} -// */ -// setup: function(){ -// window.player1 = true; // using window works -// }, - -// /** -// * @this {*} -// */ -// teardown: function(){ -// // if (this.player && this.player.el() !== null) { -// // this.player.dispose(); -// // this.player = null; -// // } -// } -// }); - -// Object.size = function(obj) { -// var size = 0, key; -// for (key in obj) { -// console.log('key', key) -// if (obj.hasOwnProperty(key)) size++; -// } -// return size; -// }; - - test('should create player instance that inherits from component and dispose it', function(){ var player = TestHelpers.makePlayer(); @@ -339,24 +309,6 @@ test('should set controls and trigger events', function() { player.dispose(); }); -// Can't figure out how to test fullscreen events with tests -// Browsers aren't triggering the events at least -// asyncTest('should trigger the fullscreenchange event', function() { -// expect(3); - -// var player = TestHelpers.makePlayer(); -// player.on('fullscreenchange', function(){ -// ok(true, 'fullscreenchange event fired'); -// ok(this.isFullscreen() === true, 'isFullscreen is true'); -// ok(this.el().className.indexOf('vjs-fullscreen') !== -1, 'vjs-fullscreen class added'); - -// player.dispose(); -// start(); -// }); - -// player.requestFullscreen(); -// }); - test('should toggle user the user state between active and inactive', function(){ var player = TestHelpers.makePlayer({}); @@ -456,28 +408,6 @@ test('make sure that controls listeners do not get added too many times', functi player.dispose(); }); -// test('should use custom message when encountering an unsupported video type', -// function() { -// videojs.options['notSupportedMessage'] = 'Video no go link'; -// var fixture = document.getElementById('qunit-fixture'); - -// var html = -// ''; - -// fixture.innerHTML += html; - -// var tag = document.getElementById('example_1'); -// var player = new Player(tag, { techOrder: ['techFaker'] }); - -// var incompatibilityMessage = player.el().getElementsByTagName('p')[0]; -// // ie8 capitalizes tag names -// equal(incompatibilityMessage.innerHTML.toLowerCase(), 'video no go link'); - -// player.dispose(); -// }); - test('should register players with generated ids', function(){ var fixture, video, player, id; fixture = document.getElementById('qunit-fixture'); From b2e67cecbd6c70c27112ee61fcb380e750a6a54d Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 24 Nov 2015 14:26:10 -0500 Subject: [PATCH 4/4] test that you can clear error from error event --- test/unit/player.test.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/unit/player.test.js b/test/unit/player.test.js index ee12f21a1f..932a5769eb 100644 --- a/test/unit/player.test.js +++ b/test/unit/player.test.js @@ -804,3 +804,21 @@ test('createModal() options object', function() { strictEqual(modal.options_.label, 'boo', 'modal options are set properly'); modal.close(); }); + +test('you can clear error in the error event', function() { + let player = TestHelpers.makePlayer(); + + sinon.stub(log, 'error'); + + player.error({code: 4}); + ok(player.error(), 'we have an error'); + player.error(null); + + player.one('error', function() { + player.error(null); + }); + player.error({code: 4}); + ok(!player.error(), 'we no longer have an error'); + + log.error.restore(); +});