Add video analytics values to variable substitution #10820
Conversation
src/service/video-manager-impl.js
Outdated
| * @return {!Promise<!../video-interface.VideoAnalyticsDetailsDef>|!Promise<undefined>} | ||
| */ | ||
| getVideoAnalyticsDetails(videoElement) { | ||
| for (let i = 0; i < this.entries_.length; i++) { |
There was a problem hiding this comment.
you can call getEntryForVideo_ to find the entry ( it also handles the assert for you), but asserts are not silent, they actually break execution. If you want it to be silent, try-catch the call to getEntryForVideo_ call
There was a problem hiding this comment.
getEntryForVideo_ takes the video instance not the element. Maybe we should add a getEntryForElement_ method :)
src/service/video-manager-impl.js
Outdated
| return entry; | ||
| } | ||
| } | ||
| dev().assert(false, 'video is not registered to this video manager'); |
There was a problem hiding this comment.
this would throw before the return. should be error() instead
There was a problem hiding this comment.
actually I wouldn't even log anything. caller can decide
There was a problem hiding this comment.
I agree that dev().error() is more appropriate than dev().assert(). Here I was mirroring the behavior of getEntryForVideo_. Should I change it there too?
There was a problem hiding this comment.
Will do. The logging has helped me while developing so I'll keep it in with the assert changed to an error
Closes #9902