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

Pass tech options to source handlers #3245

Closed

Conversation

nickygerritsen
Copy link
Contributor

Description

This PR passes the whole tech's options to a source handler in handleSource, as discussed in #2616. Note that that PR also mentions things about sourceHandlerOrder but I think that has nothing to do with passing options, so I did not add that. If desired, I can create a separate PR for this.

As far as my grep skills are accurate, I could not find any documentation that explains how source handlers work. If this is somewhere, let me know so I can add it there as well.

Specific Changes proposed

This just adds the options to handleSource. I also cleaned up some documentation and added a test for this. Furthermore I added the options argument to all handleSource calls done internally, although it is not used anywhere yet.

Requirements Checklist

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

@gkatsev
Copy link
Member

gkatsev commented Apr 8, 2016

We don't really have documentation for source handlers :(

@gkatsev
Copy link
Member

gkatsev commented Apr 8, 2016

Heh, that was pretty simple. LGTM.
Can you review this @imbcmdth and @dmlap?

@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 Apr 8, 2016
@dmlap
Copy link
Member

dmlap commented Apr 13, 2016

@nickygerritsen we don't have any official documentation for them (which I am sad about). heff did a pretty good job of doc'ing them in his original PR though. It would be great to get all that info into our docs at some point.

@@ -714,7 +714,7 @@ Tech.withSourceHandlers = function(_Tech){
this.off('dispose', this.disposeSourceHandler);

this.currentSource_ = source;
this.sourceHandler_ = sh.handleSource(source, this);
this.sourceHandler_ = sh.handleSource(source, this, this.options_);
Copy link
Member

Choose a reason for hiding this comment

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

Brilliant.

@dmlap
Copy link
Member

dmlap commented Apr 13, 2016

LGTM

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Apr 13, 2016
nickygerritsen pushed a commit to StreamOneNL/video.js that referenced this pull request Apr 13, 2016
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