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

Match by lowercase extensions in "_canPlay" #1091

Merged
merged 1 commit into from Jul 22, 2016
Merged

Conversation

sjlu
Copy link
Contributor

@sjlu sjlu commented Jul 22, 2016

In case the URL contains a uppercase character, it'll help match the mediaType list (e.g. "MP4" vs "mp4)

@@ -408,6 +408,8 @@ export default class HTML5Video extends Playback {

HTML5Video._canPlay = function(type, mimeTypesByExtension, resourceUrl, mimeType) {
var extension = (resourceUrl.split('?')[0].match(/.*\.(.*)$/) || [])[1]
extension = (extension || "").toLowerCase()

var mimeTypes = mimeType || mimeTypesByExtension[extension] || []
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @sjlu

Well spotted thanks for the PR.

I think changing this line to

var mimeTypes = mimeType || (extension && mimeTypesByExtension[extension.toLowerCase()]) || []

would be better, because we shouldn't be trying to access mimeTypesByExtension[undefined]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, will change.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 71.316% when pulling a5ad548 on sjlu:master into a4ca853 on clappr:master.

@sjlu
Copy link
Contributor Author

sjlu commented Jul 22, 2016

@tjenkinson change made

@tjenkinson
Copy link
Contributor

Great thanks! Will wait for someone else to check this.

A similar change could also be made here, here and here if you're feeling adventurous :P

@sjlu
Copy link
Contributor Author

sjlu commented Jul 22, 2016

@tjenkinson #1094 👍

@towerz towerz merged commit a4c38f2 into clappr:master Jul 22, 2016
@towerz
Copy link
Member

towerz commented Jul 22, 2016

Thanks @sjlu! 🍻

@sjlu
Copy link
Contributor Author

sjlu commented Jul 22, 2016

Glad to be of help @towerz

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.

None yet

4 participants