move timeout back into parseCues

[gkatsev]

Description

Make sure that vttjs has finished loading before proceeding with parsing cues.

Specific Changes proposed

Specifically, move the code that does a setTimeout from loadTrack back into parseCues. Make parseCues into an asynchronous loop waiting till vttjs has loaded.

Requirements Checklist

• Feature implemented / Bug fixed
• Reviewed by one Core Contributors

[gkatsev]

Going to see if I can add a test for this so it won't happen again :)

[gkatsev]

@chemoish can you review this one? You probably know the most about how tracks load next to me.

[chemoish]

Does video.js have pub sub or anything we can leverage to listen to events?

I would prefer…

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 = function () {
    console.log('load vtt.js');
  };
  this.el().parentNode.appendChild(script);
  window['WebVTT'] = true;
}

Instead of having a setTimeout.

---

Also, can't remember the influence of…

// NOTE: this is only used for the alt/video.novtt.js build

---

Also, should it be pointing to vtt.min.js?

[chemoish]

If window.WebVTT never loads, this is infinite loop?

script.src = this.options_['vtt.js'] || '../node_modules/videojs-vtt.js/dist/vtt.js'; still seems fragile (hence the question).

[gkatsev]

Yes, it would. It should have a retry count, I guess.
Maybe changing it to send an event we can listen to is a better way of doing it, I'll see if I can do it quickly, I'd like to get this change in as quickly as possible.

[chemoish]

@gkatsev since this is a patch I am ok with this going in and fixing it later to support retry or pubsub.

I didn't step through the tests, but LGTM.

[imbcmdth]

Is 400ms enough time to wait? Is there any guarantee that will be enough time on slow connections or heavily loaded pages?

[gkatsev]

I'm not sure, I guess on really slow pages or really slow connections, it will die.
Maybe we want up to 10 retries?

[chemoish]

Add // TODO: listen to onload event or similar (:

[gkatsev]

I could also have it do incremental back-off, 100 for first, 200 for second, 400 for 3rd, 800 for second. Total would be 1.5s.
Assuming we don't just do the onload.

[gkatsev]

@chemoish looks like it's not as urgent as other things, so, I'll spend time making it wait for vttjs to load instead of retrying :)

[gkatsev]

Updated the code to wait for the load event of the script and have loadTrack delay parseCues until such an event. What's left is to have those event handlers be removed if the script fails to load and also test updates.

[dmlap]

This message is outdated

[dmlap]

Other than the update to the test message, LGTM