Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure tech is defined before checking tech.isReady_ fixes #631 #632

Closed
wants to merge 1 commit into from
Closed

Conversation

tdcook
Copy link

@tdcook tdcook commented Jul 11, 2013

No description provided.

@heff
Copy link
Member

heff commented Jul 29, 2013

Thanks!

Re: #631, If Flash is uninstalled, it shouldn't ever even try to load the Flash tech and should fall back to the 'no video supported' message. If Flash is installed but disabled, it would make sense to see that error. Was Flash disabled in your case?

I'm not sure of a way to detect if Flash is installed but disabled, but that would be good to add to the Flash.isSupported check.
https://github.com/videojs/video.js/blob/v4.1.0/src/js/media/flash.js#L301

If I remember right I commented out that original code that checked for tech because it was hiding tech errors and making it harder to figure out why things weren't working. This change makes the next else statement a little off because it assumes tech is defined, but it would at least still throw an error, so that could be good enough.

@tdcook
Copy link
Author

tdcook commented Jul 29, 2013

The bug was uncovered while testing our site in a fresh Windows VM from modern.ie, which doesn't come with Flash. I'm not completely sure what happens if Flash is installed but disabled.

@heff
Copy link
Member

heff commented Jul 30, 2013

Would you be able to check if the following returns true or false on that machine?

vjs.Flash.isSupported()
https://github.com/videojs/video.js/blob/master/src/js/media/flash.js#L301

@heff
Copy link
Member

heff commented Jul 30, 2013

And maybe the version at the same time.
vjs.Flash.version()
https://github.com/videojs/video.js/blob/master/src/js/media/flash.js#L371

@tdcook
Copy link
Author

tdcook commented Jul 31, 2013

vjs.Flash.isSupported() returns false
vjs.Flash.version() returns ['0','0','0']

This is on a Windows 7 VM with IE8, both with no Flash and with Flash installed but disabled.

@heff
Copy link
Member

heff commented Aug 6, 2013

Ah ok. I see the main issue now. No tech is ever loaded in this case.

Looks good to me, thanks!

@heff heff closed this in 07351ad Aug 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants