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

Fix MediaError #3560

Closed
wants to merge 3 commits into from
Closed

Conversation

misteroneill
Copy link
Member

We discovered that attempting to construct a video.js MediaError object would fail to properly copy a native MediaError object due to the code property being non-enumerable. Additionally, there were no tests for this module.

Specific Changes proposed

  • Fix construction of video.js MediaError objects from native MediaError objects by manually assigning the code property.
  • Allow the video.js MediaError object to be redundantly constructed, such that if it is called with a video.js MediaError object, it simply returns the same object. This removes the need for two code forks found elsewhere in the code.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@misteroneill misteroneill added the patch This PR can be added to a patch release. label Aug 22, 2016

// We assign the `code` property manually because native MediaError objects
// do not expose it as an own/enumerable property of the object.
if (typeof value.code === 'number') {
Copy link
Member

Choose a reason for hiding this comment

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

should we copy this before the assign?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Not sure there's a case where there would be a difference in behavior.

@gkatsev
Copy link
Member

gkatsev commented Aug 22, 2016

Can you rebase/change this PR against the stable branch?

@gkatsev
Copy link
Member

gkatsev commented Aug 22, 2016

Two things, LGTM otherwise.

@misteroneill
Copy link
Member Author

Yeah, will rebase tomorrow morning.

@misteroneill misteroneill mentioned this pull request Aug 23, 2016
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants