Better timeout handling

[mmcc]

Right now, each time we create a timeout or interval within an instance of the player, we must also set up dispose listeners to clean up those timers. This PR adds a setTimeout and setInterval method on the player that behaves exactly the same as those native functions except they automatically add the dispose listener.

[gkatsev]

we probably should just capture player() as a variable, it's silly to call this function 4 times in two lines.

[heff]

let's say window.setTimout here to be verbose

[heff]

let's bind the function to this by default. Otherwise it will execute in the window scope, which is less useful. Also event listeners work that way.

[gkatsev]

binding to this will change the semantics of our setTimeout compared to the global window.setTimeout.

[heff]

Depends on how you look at it. With window.setTimeout the scope is window. With player.setTimeout I would expect the scope to be player.

[gkatsev]

Yep, just pointing it out.

[heff]

Pasting the code we fleshed out for adding the this.clearTimeout equivalent, that also removes the dispose listener.

vjs.Player.prototype.setInterval = function(fn, interval) {
  fn = vjs.bind(this, fn);

  var intervalId = setInterval(fn, interval);

  var disposeFn = function() {
    clearInterval(intervalId);
  };

  disposeFn.guid = 'vjs-interval-'+intervalId;

  this.on('dispose', disposeFn);

  return intervalId;
};

vjs.Player.prototype.clearInterval = function(intervalId) {
  window.clearInterval(intervalId);

  var disposeFn = function(){};
  disposeFn.guid = 'vjs-interval-'+intervalId;

  this.off('dispose', disposeFn);

  return intervalId;
};

[mmcc]

Just to note what the current status of this is, the PR is good to go in terms of getting us back to the original functionality, but we're still seeing a bug on dispose when a chapter track is involved.

[gkatsev]

@mmcc I've solved that issue in my PR (#1656) and the commit is: gkatsev@85249f6

[mmcc]

[mmcc]

Then this should be good to go. I was convinced I'd missed a timer somewhere...

[heff]

It doesn't look like interval is used anywhere here.

[heff]

Aside from a few notes this looks great. It cleans a bunch of things up nicely, including some areas where we weren't already cleaning up intervals.

So tests...
You can use the recent event listener tests as a starting model, and then Sinon timers will come in handy for this. We can pair on this if you want.

[mmcc]

I was about to get all indignant about tests because I remembered the time-to-green struggle.

...but that was to fix the weird sinon timeout issue with the current tests. These should be pretty easy, I'll take care of them + the changes in the next few days.

[heff]

In think there's a way you could do this.componentClock here, and use that in the tests, if you wanted to avoid the global.

[mmcc]

Ah cool, I was just blindly following what we did in other tests. I'll give it a try.

[mmcc]

I went ahead and removed a few globals that I remember seeing in other tests in case you're wondering where those changes came from.

[heff]

You probably want a ok(timeoutsFired === 1, 'One timeout should have fired'); here also, just in case the first one fails but a later one succeeds

[heff]

Tests look good to me!

[heff]

Could you merge this with master? There's some noops that are breaking in test/unit/media.js after merging.