From bf786ebdd1edecd60118dd94d84867e14e6495e8 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 15 Mar 2016 14:15:10 -0400 Subject: [PATCH 01/14] move timeout back into parseCues --- src/js/tracks/text-track.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index 3f28a02fbf..aa4a691f97 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -20,6 +20,14 @@ import XHR from 'xhr'; * @param {Track} track track to addcues to */ const parseCues = function(srcContent, track) { + // Make sure that vttjs has loaded, otherwise, wait till it finished loading + // NOTE: this is only used for the alt/video.novtt.js build + if (typeof window.WebVTT !== 'function') { + window.setTimeout(function() { + parseCues(responseBody, track); + }, 100); + } + let parser = new window.WebVTT.Parser(window, window.vttjs, window.WebVTT.StringDecoder()); @@ -67,14 +75,7 @@ const loadTrack = function(src, track) { track.loaded_ = true; - // NOTE: this is only used for the alt/video.novtt.js build - if (typeof window.WebVTT !== 'function') { - window.setTimeout(function() { - parseCues(responseBody, track); - }, 100); - } else { - parseCues(responseBody, track); - } + parseCues(responseBody, track); })); }; From dd9dde955f9ca966e0b6718ed45e63cb96499732 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 15 Mar 2016 14:18:33 -0400 Subject: [PATCH 02/14] fix variable name --- src/js/tracks/text-track.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index aa4a691f97..0f8516a92e 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -24,7 +24,7 @@ const parseCues = function(srcContent, track) { // NOTE: this is only used for the alt/video.novtt.js build if (typeof window.WebVTT !== 'function') { window.setTimeout(function() { - parseCues(responseBody, track); + parseCues(srcContent, track); }, 100); } From 2e5ce3924fcfde8e64450f28fb8ea548bb130e86 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 15 Mar 2016 16:03:23 -0400 Subject: [PATCH 03/14] test to make sure that the parser is called if vttjs is loaded --- test/unit/tracks/text-track.test.js | 35 +++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/unit/tracks/text-track.test.js b/test/unit/tracks/text-track.test.js index 215619468e..a5e7f87287 100644 --- a/test/unit/tracks/text-track.test.js +++ b/test/unit/tracks/text-track.test.js @@ -1,3 +1,4 @@ +import window from 'global/window'; import TextTrack from '../../../src/js/tracks/text-track.js'; import TestHelpers from '../test-helpers.js'; @@ -254,3 +255,37 @@ test('fires cuechange when cues become active and inactive', function() { player.dispose(); }); + +test('tracks are parsed if vttjs is loaded', function() { + const clock = sinon.useFakeTimers(); + const oldVTT = window.WebVTT; + let parserCreated = false; + + window.WebVTT = () => {}; + window.WebVTT.StringDecoder = () => {} + window.WebVTT.Parser = () => { + parserCreated = true; + return { + oncue() {}, + onparsingerror() {}, + onflush() {}, + parse() {}, + flush() {} + }; + }; + + let xhr; + window.xhr.onCreate = (newXhr) => xhr = newXhr; + + let tt = new TextTrack({ + tech: defaultTech, + src: 'http://example.com' + }); + + xhr.respond(200, {}, 'WebVTT\n'); + + ok(parserCreated, 'WebVTT is loaded, so we can just parse'); + + clock.restore(); + window.WebVTT = oldVTT; +}); From 1c3c218e3690075cfc84650d58296bf614586a67 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 15 Mar 2016 16:08:17 -0400 Subject: [PATCH 04/14] add broken test that tests that parsing is done when webvtt loads --- test/unit/tracks/text-track.test.js | 42 +++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/unit/tracks/text-track.test.js b/test/unit/tracks/text-track.test.js index a5e7f87287..78056d7ac6 100644 --- a/test/unit/tracks/text-track.test.js +++ b/test/unit/tracks/text-track.test.js @@ -289,3 +289,45 @@ test('tracks are parsed if vttjs is loaded', function() { clock.restore(); window.WebVTT = oldVTT; }); + +test('tracks are parsed once vttjs is loaded', function() { + const clock = sinon.useFakeTimers(); + const oldVTT = window.WebVTT; + let parserCreated = false; + + window.WebVTT = true; + + let xhr; + window.xhr.onCreate = (newXhr) => xhr = newXhr; + + let tt = new TextTrack({ + tech: defaultTech, + src: 'http://example.com' + }); + + xhr.respond(200, {}, 'WebVTT\n'); + + ok(!parserCreated, 'WebVTT is not loaded, do not try to parse yet'); + + clock.tick(100); + ok(!parserCreated, 'WebVTT still not loaded, do not try to parse yet'); + + window.WebVTT = () => {}; + window.WebVTT.StringDecoder = () => {} + window.WebVTT.Parser = () => { + parserCreated = true; + return { + oncue() {}, + onparsingerror() {}, + onflush() {}, + parse() {}, + flush() {} + }; + }; + + clock.tick(100); + ok(parserCreated, 'WebVTT is loaded, so we can parse now'); + + clock.restore(); + window.WebVTT = oldVTT; +}); From fb350697f147a50b15e10e10ec2705d315b4d77c Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 15 Mar 2016 16:08:39 -0400 Subject: [PATCH 05/14] Make sure we don't try to parse if webvtt hasn't loaded. Thereby fixing the previous test --- src/js/tracks/text-track.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index 0f8516a92e..d2c3aeaffc 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -23,7 +23,7 @@ const parseCues = function(srcContent, track) { // Make sure that vttjs has loaded, otherwise, wait till it finished loading // NOTE: this is only used for the alt/video.novtt.js build if (typeof window.WebVTT !== 'function') { - window.setTimeout(function() { + return window.setTimeout(function() { parseCues(srcContent, track); }, 100); } From 86a9bf3f05b084564fc3fc326991455b2ef4872c Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 15 Mar 2016 16:13:57 -0400 Subject: [PATCH 06/14] add semicolons --- test/unit/tracks/text-track.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/tracks/text-track.test.js b/test/unit/tracks/text-track.test.js index 78056d7ac6..61d64a661b 100644 --- a/test/unit/tracks/text-track.test.js +++ b/test/unit/tracks/text-track.test.js @@ -262,7 +262,7 @@ test('tracks are parsed if vttjs is loaded', function() { let parserCreated = false; window.WebVTT = () => {}; - window.WebVTT.StringDecoder = () => {} + window.WebVTT.StringDecoder = () => {}; window.WebVTT.Parser = () => { parserCreated = true; return { @@ -313,7 +313,7 @@ test('tracks are parsed once vttjs is loaded', function() { ok(!parserCreated, 'WebVTT still not loaded, do not try to parse yet'); window.WebVTT = () => {}; - window.WebVTT.StringDecoder = () => {} + window.WebVTT.StringDecoder = () => {}; window.WebVTT.Parser = () => { parserCreated = true; return { From b20151fbf5c838821f7a74c4c1b176b6afc1a57f Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Wed, 16 Mar 2016 12:04:45 -0400 Subject: [PATCH 07/14] Add retries to processing tracks when vttjs hasn't loaded --- src/js/tracks/text-track.js | 14 +++++++++-- test/unit/tracks/text-track.test.js | 37 +++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index d2c3aeaffc..234374e0f0 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -19,12 +19,22 @@ import XHR from 'xhr'; * @param {String} srcContent webVTT file contents * @param {Track} track track to addcues to */ -const parseCues = function(srcContent, track) { +const parseCues = function(srcContent, track, retryNum) { // Make sure that vttjs has loaded, otherwise, wait till it finished loading // NOTE: this is only used for the alt/video.novtt.js build if (typeof window.WebVTT !== 'function') { + let retry; + + if (retryNum === 0) { + return log.error(`vttjs did not load, stopping trying to process ${track.src}`); + } else if (typeof retryNum !== 'number') { + retry = 3; + } else { + retry = retryNum - 1; + } + return window.setTimeout(function() { - parseCues(srcContent, track); + parseCues(srcContent, track, retry); }, 100); } diff --git a/test/unit/tracks/text-track.test.js b/test/unit/tracks/text-track.test.js index 61d64a661b..dee0e8824d 100644 --- a/test/unit/tracks/text-track.test.js +++ b/test/unit/tracks/text-track.test.js @@ -1,6 +1,7 @@ import window from 'global/window'; import TextTrack from '../../../src/js/tracks/text-track.js'; import TestHelpers from '../test-helpers.js'; +import log from '../../../src/js/utils/log.js'; const defaultTech = { textTracks() {}, @@ -331,3 +332,39 @@ test('tracks are parsed once vttjs is loaded', function() { clock.restore(); window.WebVTT = oldVTT; }); + +test('retries processing a track 3 times if vttjs has not loaded', function() { + const clock = sinon.useFakeTimers(); + sinon.stub(log, 'error'); + const oldVTT = window.WebVTT; + let parserCreated = false; + + window.WebVTT = true; + + let xhr; + window.xhr.onCreate = (newXhr) => xhr = newXhr; + + let tt = new TextTrack({ + tech: defaultTech, + src: 'http://example.com' + }); + + xhr.respond(200, {}, 'WebVTT\n'); + + ok(!parserCreated, 'WebVTT is not loaded, do not try to parse yet'); + + clock.tick(100); + clock.tick(100); + clock.tick(100); + clock.tick(100); + + let spyCall = log.error.getCall(0); + + ok(spyCall.calledWithMatch('vttjs did not load, stopping trying to process'), + 'we logged the correct error after 3 timeouts'); + ok(!parserCreated, 'WebVTT is not loaded, do not try to parse yet'); + + clock.restore(); + log.error.restore(); + window.WebVTT = oldVTT; +}); From 71a1695126627a5b31a139320f12b522dac7623a Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Thu, 17 Mar 2016 12:34:06 -0400 Subject: [PATCH 08/14] wait for vttjs to load before parsing cues --- src/js/tech/tech.js | 6 ++++++ src/js/tracks/text-track.js | 31 +++++++++++-------------------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/js/tech/tech.js b/src/js/tech/tech.js index 5af128535d..9d6e07752e 100644 --- a/src/js/tech/tech.js +++ b/src/js/tech/tech.js @@ -327,6 +327,12 @@ class Tech extends Component { if (!window['WebVTT'] && this.el().parentNode != null) { let script = document.createElement('script'); script.src = this.options_['vtt.js'] || '../node_modules/videojs-vtt.js/dist/vtt.js'; + script.onload = () => { + this.trigger('vttjsloaded'); + }; + script.onerror = () => { + this.trigger('vttjsfailed'); + } this.el().parentNode.appendChild(script); window['WebVTT'] = true; } diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index 234374e0f0..13f300de8c 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -19,25 +19,7 @@ import XHR from 'xhr'; * @param {String} srcContent webVTT file contents * @param {Track} track track to addcues to */ -const parseCues = function(srcContent, track, retryNum) { - // Make sure that vttjs has loaded, otherwise, wait till it finished loading - // NOTE: this is only used for the alt/video.novtt.js build - if (typeof window.WebVTT !== 'function') { - let retry; - - if (retryNum === 0) { - return log.error(`vttjs did not load, stopping trying to process ${track.src}`); - } else if (typeof retryNum !== 'number') { - retry = 3; - } else { - retry = retryNum - 1; - } - - return window.setTimeout(function() { - parseCues(srcContent, track, retry); - }, 100); - } - +const parseCues = function(srcContent, track) { let parser = new window.WebVTT.Parser(window, window.vttjs, window.WebVTT.StringDecoder()); @@ -85,7 +67,16 @@ const loadTrack = function(src, track) { track.loaded_ = true; - parseCues(responseBody, track); + // Make sure that vttjs has loaded, otherwise, wait till it finished loading + // NOTE: this is only used for the alt/video.novtt.js build + if (typeof window.WebVTT !== 'function') { + if (track.tech_) { + track.tech_.on('vttjsloaded', () => parseCues(responseBody, track)); + } + } else { + parseCues(responseBody, track); + } + })); }; From ef9324eccd499932a6c5c01de7d7cd95c613093f Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Thu, 17 Mar 2016 12:34:23 -0400 Subject: [PATCH 09/14] add a watch task for the novtt file --- build/grunt.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/build/grunt.js b/build/grunt.js index 11679a4e64..0f5d791ee0 100644 --- a/build/grunt.js +++ b/build/grunt.js @@ -149,6 +149,10 @@ module.exports = function(grunt) { }, dist: {}, watch: { + novtt: { + files: ['build/temp/video.js'], + tasks: ['concat:novtt'] + }, minify: { files: ['build/temp/video.js'], tasks: ['uglify'] From c417ab03d073a2b9431953413cb15c48f8887238 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Thu, 17 Mar 2016 14:26:06 -0400 Subject: [PATCH 10/14] fix test to test that cues are parsed when vttjs is loaded --- test/unit/tracks/text-track.test.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/unit/tracks/text-track.test.js b/test/unit/tracks/text-track.test.js index dee0e8824d..d74df2b8f3 100644 --- a/test/unit/tracks/text-track.test.js +++ b/test/unit/tracks/text-track.test.js @@ -1,4 +1,5 @@ import window from 'global/window'; +import EventTarget from '../../../src/js/event-target.js'; import TextTrack from '../../../src/js/tracks/text-track.js'; import TestHelpers from '../test-helpers.js'; import log from '../../../src/js/utils/log.js'; @@ -301,8 +302,13 @@ test('tracks are parsed once vttjs is loaded', function() { let xhr; window.xhr.onCreate = (newXhr) => xhr = newXhr; + let testTech = new EventTarget(); + testTech.textTracks = () => {}; + testTech.currentTime = () => {}; + + let tt = new TextTrack({ - tech: defaultTech, + tech: testTech, src: 'http://example.com' }); @@ -326,7 +332,7 @@ test('tracks are parsed once vttjs is loaded', function() { }; }; - clock.tick(100); + testTech.trigger('vttjsloaded'); ok(parserCreated, 'WebVTT is loaded, so we can parse now'); clock.restore(); From 74970fcc6a65aea4cb30c74a79ae175e9e289e30 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Thu, 17 Mar 2016 14:29:04 -0400 Subject: [PATCH 11/14] stop waiting for vttjs if it errored out --- src/js/tracks/text-track.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index 13f300de8c..74bdc28719 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -71,7 +71,13 @@ const loadTrack = function(src, track) { // NOTE: this is only used for the alt/video.novtt.js build if (typeof window.WebVTT !== 'function') { if (track.tech_) { - track.tech_.on('vttjsloaded', () => parseCues(responseBody, track)); + let loadHandler = () => parseCues(responseBody, track); + track.tech_.on('vttjsloaded', loadHandler); + track.tech_.on('vttjserror', () => { + log.error(`vttjs failed to load, stopping processing ${track.src}`); + track.tech_.off('vttjsloaded', loadHandler); + }); + } } else { parseCues(responseBody, track); From 5d3d68bc52a74e4566bc981e787442e44c3795f1 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Thu, 17 Mar 2016 14:39:16 -0400 Subject: [PATCH 12/14] test that vttjserror removed the loaded event handler --- src/js/tracks/text-track.js | 2 +- test/unit/tracks/text-track.test.js | 24 ++++++++++++++---------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index 74bdc28719..c08fe90747 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -74,7 +74,7 @@ const loadTrack = function(src, track) { let loadHandler = () => parseCues(responseBody, track); track.tech_.on('vttjsloaded', loadHandler); track.tech_.on('vttjserror', () => { - log.error(`vttjs failed to load, stopping processing ${track.src}`); + log.error(`vttjs failed to load, stopping trying to process ${track.src}`); track.tech_.off('vttjsloaded', loadHandler); }); diff --git a/test/unit/tracks/text-track.test.js b/test/unit/tracks/text-track.test.js index d74df2b8f3..394fc6cf4c 100644 --- a/test/unit/tracks/text-track.test.js +++ b/test/unit/tracks/text-track.test.js @@ -306,7 +306,6 @@ test('tracks are parsed once vttjs is loaded', function() { testTech.textTracks = () => {}; testTech.currentTime = () => {}; - let tt = new TextTrack({ tech: testTech, src: 'http://example.com' @@ -339,7 +338,7 @@ test('tracks are parsed once vttjs is loaded', function() { window.WebVTT = oldVTT; }); -test('retries processing a track 3 times if vttjs has not loaded', function() { +test('stops processing if vttjs loading errored out', function() { const clock = sinon.useFakeTimers(); sinon.stub(log, 'error'); const oldVTT = window.WebVTT; @@ -350,8 +349,15 @@ test('retries processing a track 3 times if vttjs has not loaded', function() { let xhr; window.xhr.onCreate = (newXhr) => xhr = newXhr; + let testTech = new EventTarget(); + testTech.textTracks = () => {}; + testTech.currentTime = () => {}; + + sinon.stub(testTech, 'off'); + testTech.off.withArgs('vttjsloaded'); + let tt = new TextTrack({ - tech: defaultTech, + tech: testTech, src: 'http://example.com' }); @@ -359,16 +365,14 @@ test('retries processing a track 3 times if vttjs has not loaded', function() { ok(!parserCreated, 'WebVTT is not loaded, do not try to parse yet'); - clock.tick(100); - clock.tick(100); - clock.tick(100); - clock.tick(100); - - let spyCall = log.error.getCall(0); + testTech.trigger('vttjserror'); + let errorSpyCall = log.error.getCall(0); + let offSpyCall = testTech.off.getCall(0); - ok(spyCall.calledWithMatch('vttjs did not load, stopping trying to process'), + ok(errorSpyCall.calledWithMatch('vttjs failed to load, stopping trying to process'), 'we logged the correct error after 3 timeouts'); ok(!parserCreated, 'WebVTT is not loaded, do not try to parse yet'); + ok(offSpyCall, 'tech.off was called'); clock.restore(); log.error.restore(); From 614fe07752fbc9145845b7269d26d280b99924f2 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Thu, 17 Mar 2016 14:40:59 -0400 Subject: [PATCH 13/14] add semicolon --- src/js/tech/tech.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/js/tech/tech.js b/src/js/tech/tech.js index 9d6e07752e..fa3873ffbd 100644 --- a/src/js/tech/tech.js +++ b/src/js/tech/tech.js @@ -331,8 +331,8 @@ class Tech extends Component { this.trigger('vttjsloaded'); }; script.onerror = () => { - this.trigger('vttjsfailed'); - } + this.trigger('vttjserror'); + }; this.el().parentNode.appendChild(script); window['WebVTT'] = true; } From aa9c5d604bec5a44790812e09f09b2cf8164263e Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Thu, 17 Mar 2016 14:46:58 -0400 Subject: [PATCH 14/14] update test wording --- test/unit/tracks/text-track.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/tracks/text-track.test.js b/test/unit/tracks/text-track.test.js index 394fc6cf4c..1875e64eb7 100644 --- a/test/unit/tracks/text-track.test.js +++ b/test/unit/tracks/text-track.test.js @@ -370,7 +370,7 @@ test('stops processing if vttjs loading errored out', function() { let offSpyCall = testTech.off.getCall(0); ok(errorSpyCall.calledWithMatch('vttjs failed to load, stopping trying to process'), - 'we logged the correct error after 3 timeouts'); + 'vttjs failed to load, so, we logged an error'); ok(!parserCreated, 'WebVTT is not loaded, do not try to parse yet'); ok(offSpyCall, 'tech.off was called');