-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[player-1545] Adding in variable to enable skippable ads for iOS10+ #215
Conversation
@pilievOoyala will be fixing and adding unit tests |
…ck if param sets correct setting in IMA sdk.
…oad and loadMetadata called, so we have the page level overrides and avoid a possible race condition. Adjusting the unit tests to work too.
@@ -12,6 +12,7 @@ google = | |||
google.ima.linearAds = true; | |||
google.ima.delayAdRequest = false; | |||
google.ima.adsRenderingSettingsInstance = null; | |||
google.ima.disableCustomPlaybackForIOS10Plus = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you defined this up above as well? This way it does not start out as undefined.
js/google_ima.js
Outdated
@@ -630,7 +637,10 @@ require("../html5-common/js/utils/utils.js"); | |||
{ | |||
//resumeAd will only be called if we have exited fullscreen | |||
//so this is safe to call | |||
this.sharedVideoElement.webkitEnterFullscreen(); | |||
if (OO.isIosMajorVersion < 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a separate function to calculate if we support inline playback on iOS? That way we can use this new function here and down below when we exit full screen.
js/google_ima.js
Outdated
//in the case where skippable ads are enabled we want to exit fullscreen | ||
//because custom playback is disabled and ads can't be rendered in fullscreen. | ||
if (OO.iosMajorVersion >= 10 && this.enableIosSkippableAds === true) { | ||
this.sharedVideoElement.webkitExitFullscreen(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment for webkitEnterFullscreen about using a separate function.
LGTM |
Single review merging to get it in this build |
Need to verify that iPad is still okay and that all events for exitingFullscreen get propogated to the rest of the player. There also seems to be a race condition for trying to exit fullscreen but we are going to live with it for the moment as it's only for midroll, so we are going to advice customers only use prerolls if they want skippable ads.