NR-320035- Updating Video-Shaka-Js #10
Conversation
|
|
| <script> | ||
| function initPlayer() { |
There was a problem hiding this comment.
move the functions to separate js file
|
|
||
| this.tag.addEventListener('loadstart', this.onDownload.bind(this)) |
There was a problem hiding this comment.
Look for a better way to bind these events, we can prevent the repetition.
Example: we can move the event and function mapping to a separate (js/json) file and in JS file just run a loop to bind, unbind the events, that way maintain the mapping as well.
There was a problem hiding this comment.
@bhansa-nr This code is pretty clear as it is now, one single line per event added. I could understand a factorization in case of multiple lines of code repeated for each event handler, but it's not the case. In my opinion a different approach, like the ones you mentioned, will just make the code unnecessary cumbersome, with no clear gain in readability, compactness or maintainability.
There was a problem hiding this comment.
I slightly agree, but still I think we can find a better way to keep the events lists separate somewhere. It will increase the readability of the code.
| } | ||
|
|
||
| unregisterListeners () { | ||
| this.tag.removeEventListener('loadstart', this.onDownload) |
There was a problem hiding this comment.
Same thing here, try to use the same mapping defined for events.
|
|
||
| export default class ShakaTracker extends nrvideo.VideoTracker { | ||
| setPlayer (player, tag) { | ||
| if (!tag && player.getMediaElement) tag = player.getMediaElement() |
There was a problem hiding this comment.
My comment here is the same I did for the dash.js tracker: newrelic/video-dash-js#1 (comment) I know this is not something you created, but something the tracker had already. But maybe we should change it (check my other comment here).
There was a problem hiding this comment.
For shaka player, Not all events are directly coming from shaka Api. Once we load the Shaka player, then we can use
method getMediaElement(), by which all html5 events can be tracked. What We are trying to do is tracking some event which is coming from Shaka from player object and rest from tag object.
| this.tag.addEventListener("playing", this.onPlaying.bind(this)); | ||
| this.tag.addEventListener("seeking", this.onSeeking.bind(this)); | ||
| this.tag.addEventListener("seeked", this.onSeeked.bind(this)); | ||
| this.tag.addEventListener("error", this.onError.bind(this)); | ||
|
|
There was a problem hiding this comment.
I haven't checked the documentation of Shaka, but is it possible to register all elements in the player directly or do we really need to use the media element (stored in the tag)? If we can rely only in the player, I think we should avoid storing and using the media element.
There was a problem hiding this comment.
This can be achieved but by doing this.player.getMediaElement().addEventlistner(). Because all events are not coming directly from player object.
|
@rajeevkumar-nr before merging, could you please bump the version number (to 0.3.0) and update the changelog? |
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ## [0.2.0] - 2024/10/07 | |||
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ## [0.2.0] - 2024/10/07 | |||
| ### Lib | |||
There was a problem hiding this comment.
For some reason the previous entry in the changelog used the Lib label, which is wrong. We should use the standard semantic commit labels: "add", "update", etc.
There was a problem hiding this comment.
If in doubt, check the changelog of video-core: https://github.com/newrelic/video-core-js/blob/master/CHANGELOG.md
Jira Link - https://new-relic.atlassian.net/browse/NR-320035
This pull request updates the Shaka Tracker, Babel, and Webpack versions. It also upgrades the Shaka Player version from 2.2.1 to 4.11.2 and adds support for the MSS format, which is supported by Shaka Player.