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: Fix selectVariantsByLabel using src= #5154

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

avelad
Copy link
Collaborator

@avelad avelad commented Apr 17, 2023

No description provided.

@avelad avelad added type: bug Something isn't working correctly browser: Safari Issues affecting Safari or WebKit derivatives priority: P2 Smaller impact or easy workaround labels Apr 17, 2023
@avelad avelad added this to the v4.4 milestone Apr 17, 2023
@github-actions
Copy link
Contributor

Incremental code coverage: 0.00%

Comment on lines +4312 to +4323
const audioTracks = Array.from(this.video_.audioTracks);

let trackMatch = null;

for (const audioTrack of audioTracks) {
if (audioTrack.label == label) {
trackMatch = audioTrack;
}
}
if (trackMatch) {
this.switchHtml5Track_(trackMatch);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious: Is there any reason we use for/of over built in Array methods?

      const trackMatch = Array.from(this.video_.audioTracks)
                           .find(track => track.label == label);

      if (trackMatch) {
        this.switchHtml5Track_(trackMatch);
      }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have done it like this for consistency with the rest of the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joeyparrish what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment really was just a curiosity. I don't see anything wrong with this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it matters or not. I seem to recall that callback-based methods might have a performance penalty, maybe use more memory, I don't know. We used to do a lot of array.forEach() when the codebase was all ES5, but there was some issue that caused us to go to for-of when we upgraded to ES6+. I really can't recall what it was, and I could be misremembering the whole thing. I won't make it a policy to use one style or the other without a very good reason, and I can't remember a reason.

@avelad avelad merged commit e7d94f7 into shaka-project:main Apr 18, 2023
21 checks passed
@avelad avelad deleted the selectVariantsByLabel branch April 18, 2023 17:00
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
browser: Safari Issues affecting Safari or WebKit derivatives priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants