Add canPlayType method to player #2709
Conversation
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 991b6e801687ea29adf469923a3f5d1a188af1a1 (Please note that this is a fully automated comment.) |
|
This one seems to have the same Vista error btw :( |
|
If you rebase against master, I think the test failures will go away. |
|
|
||
| // Check if the browser supports this technology | ||
| if (tech.isSupported()) { | ||
| can = tech.canPlayType(type); |
There was a problem hiding this comment.
should the tech's canPlayType check whether it is supported itself?
I guess the downside is that you won't know whether it can't because it isn't supported or because it can't play that type if you ask the tech directly.
There was a problem hiding this comment.
Also kinda copied this from selectSource, to keep them "in sync".
There was a problem hiding this comment.
Keeping them in sync might not be a bad idea. Not certain which I like more.
Do you have a strong preference, @nickygerritsen?
There was a problem hiding this comment.
No not really, although maybe when we want to add support for #2705 that it would be better to add the isSupported call inside the tech. Otherwise we need to duplicate that again there?
There was a problem hiding this comment.
Actually, thinking about it some more, I think I like keeping the isSupported out of canPlayType. Makes canPlayType super simple and do exactly one thing.
There was a problem hiding this comment.
OK then an update is incoming in 3...
|
Thanks @nickygerritsen for the PR. I made a few comments but it generally looks good! |
|
LGTM. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 9ddaeca4a51ec9c820e34a3fa4f645f29c5b75d5 (Please note that this is a fully automated comment.) |
|
Vista IE9 still seems to be broken :( |
|
Travis passed but the savage tests are broken right now (fixed in master). |
|
Hmmm but if I'm not mistaken I did rebase onto master. But maybe my git skills failed |
|
I can have pam retry the tests. |
|
@pam retry |
|
@nickygerritsen looks like it didn't get picked up since you're missing some commits. Make sure you have the latest master. There were a bunch of stuff committed yesterday :) |
|
Ah now I should have the correct master :) |
| can = handlers[i].canPlayType(type); | ||
|
|
||
| if (can) { | ||
| return can; |
|
There's a couple indent issues in player.js besides the ones I pointed out. Otherwise, LGTM |
|
Not sure what went wrong there but fixed anyways |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 3d85440 (Please note that this is a fully automated comment.) |
|
Hurray all green! (technically) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 9ddaeca4a51ec9c820e34a3fa4f645f29c5b75d5 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: c88eddf (Please note that this is a fully automated comment.) |
|
Ignore those last two. They're old builds. |
|
Thanks! |
|
\o/ |
If someone calls player.canPlayType() with HLS registered as a source handler, it would fail at runtime. Related to videojs/video.js#2709.
This should be the code for #2701 including some tests.
I'm not sure this is what is wanted exactly, but I'm of course open to comments. For example, does it make more sense to let
player.canPlayTypereturn a boolean? I had that at first but then decided that returning the maybe / probably thingy isn't any more work and maybe more helpful?For testing, there is only one small test for
player.canPlayType, but I couldn't find any tests for things linkselectSourceso I did not really have anything to base my tests upon.