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

Remove dead code XDomainRequest reference #1326

Merged
merged 1 commit into from Sep 14, 2017

Conversation

benjamingr
Copy link
Contributor

There is an XDomainRequest reference here from 7f772ef#diff-5b5ca1c2ac7acf4ca9be5b53329cc1a2 but this only works in IE9 and IE10 which don't have MSE anyway and aren't supported by hls.js

Remove this tidbit of dead code.

There is an XDomainRequest reference here from video-dev@7f772ef#diff-5b5ca1c2ac7acf4ca9be5b53329cc1a2 but this only works in IE9 and IE10 which don't have MSE anyway and aren't supported by hls.js

Remove this tidbit of dead code.
@benjamingr benjamingr changed the title Remove XDomainRequest reference Remove dead code XDomainRequest reference Sep 3, 2017
@mangui
Copy link
Member

mangui commented Sep 6, 2017

thanks @benjamingr
the issue is that these lines have been added on purpose
see #185 (comment)
I am bit reluctant to remove them
ping @mimse

@benjamingr
Copy link
Contributor Author

Hi @mangui - thanks for the review.

XDomainRequest is not defined in IE11 to begin (not on Windows 7 and not on Windows 8.1).

I just checked with BrowserStack:

screen shot 2017-09-06 at 16 14 41

screen shot 2017-09-06 at 16 15 20

I'm afraid no code has ever entered this path in hls.js :) This is a good thing, since XDomainRequest has completely different semantics for CORS - for example you can't use it to read an array buffer (IIRC it's only text data, and no additional headers or something silly like that).

@mangui
Copy link
Member

mangui commented Sep 10, 2017

thanks @benjamingr fair enough, i am going to merge next week unless @mimse gives rationale until then

@mangui mangui merged commit 8c706b9 into video-dev:master Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants