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

Error if Flash tech is unusable #50

Merged
merged 13 commits into from
Dec 7, 2016
Merged

Conversation

forbesjo
Copy link
Contributor

@forbesjo forbesjo commented Dec 6, 2016

No description provided.

@@ -61,6 +61,13 @@ const initPlugin = function(player, options) {
const resetMonitor = function() {
window.clearTimeout(monitor);
monitor = window.setTimeout(function() {
// if using Flash then make sure its API is available
if (/swf/i.test(player.tech_.el().data) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get at this without using tech_? That's supposed to be a private property.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to use player.$() instead

let tech = player.$('.vjs-tech');

// don't error if using Flash and its API is available
if (tech && tech.type === 'application/x-shockwave-flash' && tech.vjs_getProperty) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be && !tech.vjs_getProperty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are correct

@forbesjo
Copy link
Contributor Author

forbesjo commented Dec 7, 2016

I think something's still wrong with this, here's the scenario:
Player is triggering timeupdates
Paused is true for some reason
Flash player is broken and its API is unavailable
-> Timeout will trigger in healthcheck, resetMonitor is not called
-> healthcheck triggers again but in check resetMonitor is called
-> this will clear the existing timeout

}

// playback isn't expected if the player is paused, shut
// down monitoring
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is somewhat inaccurate. Technically, you're saying "keep checking, it looks like everything is fine right now".

@forbesjo forbesjo merged commit 85a9b0c into videojs:master Dec 7, 2016
shoremark pushed a commit to SardiusMedia/videojs-errors that referenced this pull request Jan 23, 2017
* Error if Flash tech is unusable

* Return early if there already is an error

* Added clarifying comments
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.

3 participants