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

Remove unnecessary references to manifest #1848

Merged

Conversation

nicosang
Copy link
Contributor

The main idea behind this Pull Request is to centralize the access to manifestModel object. When a manifest is downloaded, the callback 'onManifestUpdated' is called. I suggest to update Periods array in DashAdapter class. The manifest is added to each period object. So, we don't need, in each file, to set manifest as parameter. There is an exception : it's for getTracksForTypeFromManifest and getStreamsFromManifest functions of MediaPlayer. Those functions could be called for an unplayed stream.

Copy link
Contributor

@dsparacio dsparacio left a comment

Choose a reason for hiding this comment

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

I think if we do any refactoring around ES5 code it should be converted into ES6 syntax. Remove Var and use Let and Const at the very minimum. I will send out and email today with some details around this ask.


for (var i = 0; i < ln; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are going to refactor this kind of stuff we would want to move to es6. This is a style that we decided on.

@nicosang nicosang force-pushed the CleanUpRequestNext branch 3 times, most recently from f814b5d to d3d285a Compare May 22, 2017 13:01
@nicosang nicosang force-pushed the CleanUpRequestNext branch 3 times, most recently from 4b2c80e to 91c56d8 Compare June 9, 2017 08:34
@dsparacio dsparacio merged commit e80130d into Dash-Industry-Forum:development Jun 13, 2017
@nicosang nicosang deleted the CleanUpRequestNext branch June 14, 2017 07:44
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

2 participants