chore: update travis to test vjs5/6 and chrome/firefox

[brandonocasey]

No description provided.

[gkatsev]

it's failing because of a breaking change in videojs 6 with regards to https://github.com/videojs/videojs-contrib-ads/blob/master/src/plugin.js#L185-L186

[misteroneill]

Hmm, seems like that's gonna be an issue we'll have to figure out.

[incompl]

What change causes those lines to break?

[gkatsev]

We've changed player.src() to not return the blob url anymore. Since means that there's currently no way to get the actual src set on the video element and the current way of telling if the source has changed doesn't work anymore. A potential fix would be to get ask the tech directly for its src.

[incompl]

Is it really OK for there to be no way to get the actual src?

[gkatsev]

It's not that there isn't really a way. It just requires going through the tech right now (There's no way to get it directly from the player). Thinking about what src and currentSrc() are supposed to represent, especially with the new middleware feature, the changes make sense. We noticed this breakage back then and decided that it's still worth moving forward with the changes.

Basically, src() should be set to whatever url is actually set, and currentSrc() would be set to the middleware source. In the case of putting an HLS url directly, as opposed to having it be translated by middleware, src() and currentSrc() would end up returning the same url. currentSource() would be the object containing currentSrc() and currentType().

[incompl]

My understanding was that accessing the tech directly is an architectural antipattern, is that going to continue to be the case with vjs 6? Is this problem unique to contrib-ads for some reason?

[gkatsev]

Yeah, we've pulled back the curtain on tech a bit. There are times when it's definitely valid, though, should still be discouraged. player.tech() in 6.0 will log a warning, and could be silenced with any variable passed in.
Also, yeah, I think it's more of an issue with contrib-ads since it needs to be able to handle cases where the video element was changed directly. Most users of videojs would want the new behavior as it matches expectations of what should be happening more closely.

[misteroneill]

@brandonocasey If you don't mind, we can probably pretty easily update this PR to replace the src() check with a direct call to the tech. That should be consistent between Video.js 5 and 6.

EDIT: My $0.02 on talking directly with the tech is that it's kind of a grey area. As @gkatsev said there are definitely valid use-cases and this is one of them. I personally don't like that we expose a method, but require passing something to it.

[ldayananda]

shouldn't karma-detect-browsers and karma-qunit be added to config.plugins to have browser detection and running the tests in a browser work locally?

[brandonocasey]

By default karma includes anything in plugins that matches the following: karma-* . So the default works in cases like this where the plugin is named karma-detect-browsers or karma-qunit. So we don't have to explicitly define anything in the plugins array.

[ldayananda]

Cool, good to know!

[incompl]

LGTM overall.

[ldayananda]

LGTM, but should be tested before merging due to the code change.

[brandonocasey]

available as [email protected] and videojs-contrib-ads@vjs6qa on npm

[misteroneill]

What testing does this need? The source code changes are really minimal and would be caught in unit tests...

[incompl]

We decided to test the VJS6 updates to IMA and contrib-ads together, so it's being tested that way.