Comscore several fixes tva #16
Conversation
| @@ -91,14 +91,14 @@ | |||
| }; | |||
|
|
|||
| var player = new bitmovin.player.Player(container, playerConf); | |||
|
|
|||
There was a problem hiding this comment.
Please remove empty spaces
| player.load(source).then(function() { | ||
| console.log('Successfully created Bitmovin Player instance'); | ||
| }, function(reason) { | ||
| console.error('Error while creating Bitmovin Player instance'); | ||
| console.error(reason); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Please remove empty spaces
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "@bitmovin/player-integration-comscore", | |||
| "name": "@bitmovin/player-integration-comscore2", | |||
There was a problem hiding this comment.
What was the intention to rename this package but include the old version of this package as a dependency again?
That does not seem right at all.
| @@ -6,14 +6,16 @@ export class ComScoreAnalytics { | |||
| private static started: boolean = false; | |||
| private static configuration: ComScoreConfiguration; | |||
| private static logger: ComScoreLogger; | |||
| public configuration: ComScoreConfiguration; | |||
There was a problem hiding this comment.
Why do we need this instance variable now? Everything I see in this class is static so there will never be an instance.
|
|
||
| /** | ||
| * Starts ComScore app tracking | ||
| * @param configuration - Configuration object for your ComScore specific identifiers | ||
| */ | ||
| public static start(configuration: ComScoreConfiguration) { | ||
| this.logger = new ComScoreLogger(configuration); | ||
|
|
||
| this.configuration = configuration; |
There was a problem hiding this comment.
I guess this can be reverted.
| @@ -127,6 +168,8 @@ export class ComScoreStreamingAnalytics { | |||
|
|
|||
| private playbackFinished = (event: PlayerEventBase) => { | |||
| this.stopComScoreTracking(); | |||
| this.streamingAnalytics = null; | |||
| new ComScoreStreamingAnalytics(this.player, this.metadata, this.configuration); | |||
There was a problem hiding this comment.
Why initializing a new instance of ComScoreStreamingAnalytics without using the instance?
| @@ -137,6 +180,14 @@ export class ComScoreStreamingAnalytics { | |||
| this.transitionToVideo(); | |||
| } | |||
|
|
|||
| private stopComScoreTrackingBackgroundAd(): void { | |||
| console.log(' DEBUG ComScoreStreamingAnalytics stopped active background Ad'); | |||
There was a problem hiding this comment.
Please remove log output or use the logger.
| @@ -137,6 +180,14 @@ export class ComScoreStreamingAnalytics { | |||
| this.transitionToVideo(); | |||
| } | |||
|
|
|||
| private stopComScoreTrackingBackgroundAd(): void { | |||
There was a problem hiding this comment.
Why not using the existing stopComScoreTracking method? Seems to do the same but also stets the state.
| } | ||
|
|
||
| private timeChanged = (event: PlaybackEvent) => { | ||
| if ( ( this.playerTransitioningState === PlayerTransitionState.TransitioningToVideo ) && ( this.comScoreState !== ComScoreState.Video ) ) { |
There was a problem hiding this comment.
Why not calling the existing transitionToAd / transitionToVideo methods and keep the code there?
| c3: this.metadata.uniqueContentId, | ||
| c4: this.metadata.uniqueContentId, | ||
| c6: this.metadata.uniqueContentId, | ||
| ns_st_ft: this.metadata.feedType, | ||
| ns_st_ce: this.metadata.completeEpisode ? '1' : null, | ||
| ns_st_ia: this.metadata.advertisementLoad ? '1' : null, | ||
| ns_st_ce: this.metadata.completeEpisode ? this.metadata.completeEpisode : '*null', |
There was a problem hiding this comment.
What is this *null value and why not using null like before?
If *null should be used please extract this to a const variable and use the variable instead
Fixed the following issues:
When ad is paused on Home/ sleep button press -> Call stop() -> This will fire a measurement with ns_st_ev=pause
Coming to the issue, when ad is resumed from pause, you are calling the playVideoAdvertisement API. This fires two measurements. One measurement with ns_st_ev=end and second measurement with ns_st_ev=play. This is incorrect.
When ad is resumed from pause, no API call should be made, the playVideoAdvertisement API should not be called. No measurements should be fired on ad resume.