-
Notifications
You must be signed in to change notification settings - Fork 10
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
Headers, credentials and base64 certificate #16
base: master
Are you sure you want to change the base?
Headers, credentials and base64 certificate #16
Conversation
'certificate' as a base64string 'licenseRequestHeaders' as a function to call to add any headers that might be needed 'withCredentials' so we can send cookies with the license request
licenseRequestHeaders as an object makes more sense.
Just switched to an object for the request headers, am looking at passing back some information on the response too :) |
Any update on this pull request? Being able to override headers is an extremely handy feature to have |
@chris-heathwood-piksel i seriously hope you forked this; idk why i sporadically get emails from github. i am no longer working on video stuff, but i can do a cursory review this week and merge a year later :X |
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.
Overall comment, it would be easier to merge and review if the changes were separated and there was a brief description to the reasons behind the changes.
@@ -12,6 +12,10 @@ class Html5Fairplay { | |||
logToBrowserConsole = value; | |||
} | |||
|
|||
ab2str(buf) { |
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 would move this to util
@@ -36,6 +40,11 @@ class Html5Fairplay { | |||
|
|||
tech.isReady_ = false; | |||
|
|||
if (this.protection_.certificate) { | |||
this.log('Using base64 encoded certificate set on the source protection.'); |
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.
is there value to log to indicate a parameter has been set?
@@ -36,6 +40,11 @@ class Html5Fairplay { | |||
|
|||
tech.isReady_ = false; | |||
|
|||
if (this.protection_.certificate) { | |||
this.log('Using base64 encoded certificate set on the source protection.'); | |||
certificate = base64DecodeUint8Array(this.protection_.certificate); |
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.
is it worth it to refactor?
// from
let certificate;
// to
if (this.protection._certificate) {
this.setCertificate(this.protection._certificate);
}
setCertificate(certificate) {
// can normalize input later and or just leave it simple—would also need to update references
this.certificate = certificate;
}
} | ||
|
||
return all; | ||
}, |
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.
}, {});
style preference.
if (status !== 200) { | ||
this.onRequestError(event.target, ERROR_TYPE.FETCH_LICENCE); | ||
|
||
// Return response content | ||
if (licenseResponseErrorContent && typeof licenseResponseErrorContent === 'function') { |
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.
licenseResponseErrorContent
=> licenseLoadError
is there a better name or something more applicable?
is an inverse needed? licenseLoadSuccess
? licenseLoadHeaders
?
return; | ||
} | ||
|
||
// Return response headers here | ||
if (licenseResponseHeaders && typeof licenseResponseHeaders === 'function') { | ||
const headers = event.target.getAllResponseHeaders().split('\r\n').reduce((all, part) => { |
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 would chuck this in a method somewhere to facilitate legibility and if its needed in another method.
createHeadersFromResponseHeaders
or something.
// Optional settings | ||
|
||
// Instead of a certificateUrl send the cert in a string | ||
certificate: 'base64string', |
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 would stick this next to certificateUrl
and indicate either or.
certificate: 'base64string', | ||
|
||
// Object to add any extra license request headers that might be needed | ||
licenseRequestHeaders: { |
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.
can this just be headers
? is there multiple headers that need to be set to indicate and prefix a type (licenseRequest
)?
}, | ||
|
||
// Function to call back with license response content if it errors | ||
licenseResponseErrorContent: function (content) { |
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.
see some of the comments below, ideally I would like to consolidate naming to one specific style.
shooting from the hip;
onLicenseLoad
(can be changed if needed)onLicenseError({ error })
onLicenseSuccess({ headers })
signatures would be able to be grouped and are conventionally named, they can be enhanced with data as needed.
@@ -24,6 +24,31 @@ videojs('player_id').ready(function () { | |||
|
|||
certificateUrl: '/path/to/certificate', | |||
licenseUrl: '/path/to/license', | |||
|
|||
// Optional settings |
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.
Ideally, options would be separated and described in an options section.
See https://github.com/chemoish/videojs-bif#options or whatever similar.
Feel free to email me directly if you still need these changes, I will prioritize whatever you need. Again, apologies for not seeing this sooner. |
Hi,
Hope it is ok created the PR with some extra optional settings.
It would be great if this could make it in to npm :)
Thanks
Chris H