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

Jump to bottom

Make sure that XHR uses the XDomainRequest on old browsers. #2633

Closed
wants to merge 9 commits into from
Closed

Make sure that XHR uses the XDomainRequest on old browsers. #2633

wants to merge 9 commits into from

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Sep 23, 2015

No description provided.

@heff
Copy link
Member

heff commented Sep 23, 2015

How will this affect same-domain tracks?

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

Hm... Good question. XHR uses regular XmlHttpRequests in modern browsers, so, for those, we won't be affected. Looking into how XDR works to figure out whether it should be an issue as well as trying some out.

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

I'm going to open an issue against XHR as well.

@pam
Copy link

pam commented Sep 23, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: c8a8ed5290ea3f5ac882ef85dc37fe044fd22922
Build details: https://travis-ci.org/pam/video.js/builds/81824626

(Please note that this is a fully automated comment.)

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

Also, sounds like XDR might work with same domain as long as the CORS headers are set up correctly.

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

It works, still need to confirm whether it'll require CORS headers or not.

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

Yep, XDR requires the CORS headers even for same-domain requests.

@heff
Copy link
Member

heff commented Sep 23, 2015

If this doesn't introduce same-domain issues, then looks good to me.

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

It does mean we can't request tracks from the same domain if it doesn't also include CORS.

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

Btw, here's the issue against XHR: naugtur/xhr#94

@gkatsev gkatsev added this to the v5.0.0 milestone Sep 23, 2015
@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

I'm going to add https://github.com/videojs/video.js/blob/stable/src/js/xhr.js#L80-L84 in here for now so it won't be a blocker and then we can discuss naugtur/xhr#94 there and whether a PR for this will be submitted.

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

Looks like that doesn't specific snippet doesn't take into account protocol relative urls. But I know how to handle those too.

@gkatsev
Copy link
Member Author

gkatsev commented Sep 23, 2015

@heff a more complete implementation now.

@gkatsev gkatsev mentioned this pull request Sep 23, 2015
Open
@pam
Copy link

pam commented Sep 23, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: b5718b9
Build details: https://travis-ci.org/pam/video.js/builds/81849412

(Please note that this is a fully automated comment.)

@pam
Copy link

pam commented Sep 23, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 25f26ac
Build details: https://travis-ci.org/pam/video.js/builds/81850165

(Please note that this is a fully automated comment.)

gkatsev
gkatsev reviewed Sep 23, 2015
View reviewed changes
src/js/tracks/text-track.js
let srcProtocol = urlInfo.protocol === ':' ? winLoc.protocol : urlInfo.protocol;
// Check if url is for another domain/origin
// IE8 doesn't know location.origin, so we won't rely on it here
let crossOrigin = (srcProtocol + urlInfo.host) !== (winLoc.protocol + winLoc.host);
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe I should move that into our url module.

@heff
Copy link
Member

heff commented Sep 23, 2015

lgtm

Conversation from chat.

@gkatsev: so it essentially works the same as our custom xhr now, yeah?

heff [5:16 PM]
I guess at least for tracks

gkatsev [5:16 PM] 
yep

gkatsev [5:16 PM]
tracks is the only place where we use XHR internally right now

gkatsev [5:16 PM]
and discussing how and what to do about the xhr module here: https://github.com/Raynos/xhr/issues/94

GitHub
Doesn't auto choose XDR when making a cross-domain request · Issue #94 · Raynos/xhr · GitHub
As a user of XHR, I would expect it to realize when the request will require CORS and choose to use XDR. This is mostly an issue on browsers like IE8 and IE9 where a CORS request requires the usage...

heff [5:17 PM] 
Is everything like HLS going to have to reimplement what you have in tracks?

gkatsev [5:17 PM] 
HLS can probably just always set cors to true

gkatsev [5:17 PM]
since they don't care about IE8

gkatsev [5:18 PM]
only code that needs to work about IE8 would need to check whether it's actually cross origin or not

gkatsev [5:18 PM]
which is why I think we should move that into our url module and expose it as a utility

@gkatsev
Move isCrossOrigin to url.js.
972e167 
Write some tests using proxyquireify to mock out 'global/window' in url.js.
@gkatsev
Expose isCrossOrigin on videojs object.
3dd226f
@heff
Copy link
Member

heff commented Sep 24, 2015

lgtm!

gkatsev
gkatsev reviewed Sep 24, 2015
View reviewed changes
test/unit/utils/url.test.js
@@ -1,6 +1,8 @@
import document from 'global/document';
import window from 'global/window';
import * as Url from '../../../src/js/utils/url.js';
import proxyquireify from 'proxyquireify';
const proxyquire = proxyquireify(require);
Copy link
Member Author

Choose a reason for hiding this comment

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

Proxyquireify uses a module called merge-descriptors that is currently throwing some warnings in chrome. I'll open an issue against it so we can get that closed out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened an issue: sindresorhus/merge-descriptors#5

@gkatsev gkatsev mentioned this pull request Sep 24, 2015
Closed
@gkatsev
Copy link
Member Author

gkatsev commented Sep 24, 2015

Somehow completely missed the jshint issue.

gkatsev
gkatsev reviewed Sep 24, 2015
View reviewed changes
src/js/utils/url.js
// IE8 protocol relative urls will return ':' for protocol
let srcProtocol = urlInfo.protocol === ':' ? winLoc.protocol : urlInfo.protocol;

console.log(srcProtocol, winLoc.protocol, urlInfo.host, winLoc.host);
Copy link
Member Author

Choose a reason for hiding this comment

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

this is for travis testing :/. Will be removed ASAP.

@gkatsev gkatsev closed this in 0b7cf58 Sep 24, 2015
@gkatsev gkatsev deleted the cors-xhr branch September 24, 2015 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gkatsev @heff @pam