feat(ads): Add control AdsRenderingSettings #5027
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
rizalibnu
force-pushed
the
ads-rendering-settings
branch
from
04887fd
to
197aba4
Compare
avelad
added
type: enhancement
|
@rizalibnu can you fix the tests? Thanks for your contribution! |
| @@ -26,7 +26,7 @@ shaka.ads.ClientSideAdManager = class { | |||
| * @param {string} locale | |||
| * @param {function(!shaka.util.FakeEvent)} onEvent | |||
There was a problem hiding this comment.
You should add adsRenderingSettings to the JSDoc here.
| * @param {!google.ima.AdsRenderingSettings} adsRenderingSettings | ||
| */ | ||
| updateAdsRenderingSettings(adsRenderingSettings) { | ||
| if (this.imaAdsManager_) { |
There was a problem hiding this comment.
You should also change the value of this.adsRenderingSettings_, so that this method will still work if called before we receive an ADS_MANAGER_LOADED event and create this.imaAdsManager_.
| }; | ||
|
|
||
|
|
||
| /** @const */ | ||
| google.ima.AdsManagerLoadedEvent = class extends Event { | ||
| /** | ||
| * @param {!HTMLElement} video | ||
| * @param {!google.ima.AdsRenderingSettings=} adsRenderingSettings |
There was a problem hiding this comment.
I'm... actually not sure if you need to define the google.ima.AdsRenderingSettings type in the externs?
At no point in the code do we actually access the inner variables of the type, we just take it in as a parameter and then pass it unmodified to the IMA SDK. We also never make an google.ima.AdsRenderingSettings object in-code.
So it might just be enough to say that this is an Object=. We do stuff like for other pass-through config objects in other externs, like in externs/mux.js (for the constructor of muxjs.mp4.Transmuxer).
There was a problem hiding this comment.
@theodab I think that adding the definition is not bad, and it can help people in the integration. I would not take away the work that has already been done.
|
@rizalibnu can you review the latest comments? Thanks! |
avelad
added
the
status: waiting on response
|
Closing due to inactivity. If you need to reopen this PR, just put |
github-actions
bot
removed
the
status: waiting on response
github-actions
bot
added
the
status: archived
Closes #5011
This add the possibility to control the rendering of ads