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

feat: Add support for defining custom headers for default license and certificate requests. #76

Merged
merged 7 commits into from
Mar 20, 2019

Conversation

alex-barstow
Copy link
Contributor

No description provided.

@gkatsev
Copy link
Member

gkatsev commented Feb 28, 2019

I think headers is too generic. Since it can be passed in via a source object, someone could see it and maybe expect it to be added for all source requests.

@alex-barstow
Copy link
Contributor Author

@gkatsev Yeah it is definitely too generic. I left it as such until we can establish the following: should we allow headers to be set only on license requests? Or also certificate and key requests?

@misteroneill
Copy link
Member

misteroneill commented Mar 14, 2019

I think we ought to allow custom headers for any of the default requests contrib-eme can make.

@misteroneill misteroneill changed the title feat: add support for headers emeOption (WIP) feat: Add support for defining custom headers for default license and certificate requests. Mar 14, 2019
const defaultGetLicense = (url) => (emeOptions, keyMessage, callback) => {
const defaultGetLicense = (keySystemOptions) => (emeOptions, keyMessage, callback) => {
const headers = videojs.mergeOptions(
{'Content-type': 'application/octet-stream'},
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're now providing options to add headers, any chance you could not include this by default? For whatever reason, our DRM provider doesn't have Content-Type in the Access-Control-Allow-Headers header, so I provide a custom function just so this header isn't there.

Copy link
Member

Choose a reason for hiding this comment

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

do you think there would be issues in removing it? I've got no major objections unless removing it may break someone.

Copy link
Member

Choose a reason for hiding this comment

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

What if we allowed removing headers by setting them to null or something, so you could do:

licenseHeaders: {
  'Content-Type': null
}

I am hesitant to remove defaults because it could easily break someone.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Test updates look good to me!

@misteroneill misteroneill merged commit 7197390 into videojs:master Mar 20, 2019
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