Cmcdv2/cmcd parameters#156
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…nto cmcdv2/cmcd-parameters
…v2/cmcd-parameters
…v2/cmcd-parameters
Summary of ChangesHello @cotid-qualabs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CMCD implementation by enabling the definition and processing of CMCD version 2 reporting targets directly from the DASH manifest. This change provides greater flexibility and dynamic control over how CMCD data is collected and transmitted, allowing content providers to specify reporting endpoints and parameters within the media manifest itself. The update includes new data structures, parsing logic, and configuration prioritization to seamlessly integrate manifest-driven CMCD reporting into the player's workflow. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for CMCD v2 parameters, specifically focusing on ReportingTargets and EventTarget objects as defined in the CMCD specification. The changes include adding new constants, updating the DASH parser to recognize EventTarget nodes, and implementing a new EventTarget class. The CMCDParameters class has been enhanced to correctly parse ReportingTargets from the manifest, handling both single and multiple EventTarget entries. The CmcdPropertyMap has been updated to prioritize manifest-defined CMCD v2 parameters over player settings, ensuring adherence to the specification's precedence rules. Additionally, new unit tests have been added to thoroughly validate the new functionality across various scenarios, including different reporting modes, event filtering, and manifest-settings priority. Overall, the implementation appears robust and well-tested, significantly extending the player's CMCD capabilities.
| this.mode = data.mode ?? 'query'; | ||
| this.keys = data.keys ? data.keys.split(' ') : null; | ||
| this.events = data.events ? data.events.split(' ') : null; | ||
| this.enabled = data.enabled; |
There was a problem hiding this comment.
The enabled property is initialized to true in the constructor, but then directly assigned data.enabled in the init method. If data.enabled is undefined or null (meaning it's not explicitly provided in the manifest or settings), it will overwrite the default true with undefined or null. It's generally safer to provide a fallback for data.enabled if it's not explicitly set, similar to how mode is handled, to ensure the property always has a boolean value.
| this.enabled = data.enabled; | |
| this.enabled = data.enabled ?? true; |
No description provided.