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

Retain details from tech error. #3313

Closed
wants to merge 1 commit into from

Conversation

MattiasBuelens
Copy link
Contributor

Description

Extra details on media errors from a Tech component are lost when propagated to the Player instance. For example, custom error messages are lost and always use the default message based on the error's code.

Demonstration

This example on JSBin registers a dummy MyTech tech. In its play() function, it immediately calls error() with a custom error object:

play() {
    this.trigger('play');
    this.error({
        code: MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED,
        message: 'Custom error message'
    });
}

Expected behavior

The player shows an error with the message: "Custom error message"

Actual behavior

The player shows an error with the default message for this media error code: "The media could not be loaded, either because the server or network failed or because the format is not supported."

Specific Changes proposed

Player.handleTechError_ is changed to pass the whole error object to Player.error, instead of only passing error.code. This ensures that additional details (such as error messages) are retained.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@gkatsev
Copy link
Member

gkatsev commented May 11, 2016

Makes sense to me. LGTM.

@gkatsev gkatsev added minor This PR can be added to a minor release. It should not be added to a patch release. needs: LGTM Needs one or more additional approvals labels May 11, 2016
@nickygerritsen
Copy link
Contributor

LGTM as well

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels May 11, 2016
@MattiasBuelens MattiasBuelens deleted the TechError branch May 20, 2016 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants