-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Augment userAgent detection #470
Conversation
@@ -314,7 +314,7 @@ vjs.IOS_VERSION = (function(){ | |||
if (match && match[1]) { return match[1]; } | |||
})(); | |||
|
|||
vjs.IS_ANDROID = !!vjs.USER_AGENT.match(/Android.*AppleWebKit/i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only one that worries me, because we currently use the webkit check to make sure we only override the canPlayType function in the default android browser. Changing this would match Firefox too. We just need a solution that takes that into consideration.
https://github.com/videojs/video.js/blob/master/src/js/media/html5.js#L231
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That if statement also checks the version of android. Firefox does not report the version of android; only Chrome for Android and the Native Android browser do. So, we could add a check there to make sure the version isn't null, since null < 3
is true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, why are we check if the version is less than 3 if it is only broken on 2.2 and lower?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming it's still broken in 2.3 and fixed in 3.
On May 6, 2013, at 3:59 PM, Gary Katsevman notifications@github.com wrote:
In src/js/lib.js:
@@ -314,7 +314,7 @@ vjs.IOS_VERSION = (function(){
if (match && match[1]) { return match[1]; }
})();-vjs.IS_ANDROID = !!vjs.USER_AGENT.match(/Android.*AppleWebKit/i);
By the way, why are we check if the version is less than 3 if it is only broken on 2.2 and lower?—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, just the comment and code are in disagreement. I can do a quick check and see anyway.
Do you think that checking for null
there is a good compromise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it does say 2.2. Definitely worth checking then.
Checking for null how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in media/html5.js, do a null check before checking the version, since firefox doesn't report report android version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's interesting. I didn't realize Firefox didn't report Android version. Is that true for other browsers like opera mobile too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to http://www.useragentstring.com/pages/Opera Mobile/, some versions do report the android version and some don't. I'll do a quick check on that as well.
Checking for webkit in the long run, isn't a good idea, since chrome is moving to Blink at some point. We might want to have a separate check for webkit or something else for older androids.
Taking the convo out of line-notes. |
Opera does return the android version in the if (vjs.IS_ANDROID && vjs.IS_WEBKIT && vjs.ANDROID_VERSION < 2.3) {
/* apply canPlayType fix */
} I can't think of any other or better ways at the moment. |
Ok, thanks for doing all that research. What if we wrapped those 3 checks into an 'oldAndroid' check. I'm looking at a presentation I did and I had 3 major bugs listed:
I like what you said earlier about only using user agent checks for older browsers. Having a vis.IS_WEBKIT might open the door for people to use that in places they shouldn't. |
|
Cool, that works for me. On May 8, 2013, at 3:29 PM, Gary Katsevman notifications@github.com wrote:
|
Ok, i'll make that change in a bit. |
Looks like |
Updated with a fix to |
Add a userAgent detection for Chrome.
Rework detection using RegExp#test to make the checks more efficient.
Make the
vjs.IS_ANDROID
just check for android so that it can be used in conjunction with the firefox and chrome checks.Fix
vjs.IS_FIREFOX
being a function that is used as a property by making it be a property like the other predicates.