Skip to content
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

[JW8-10170] Resolve pauseAds issues #3466

Merged
merged 3 commits into from Aug 14, 2019
Merged
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -403,6 +403,16 @@ const InstreamAdapter = function(_controller, _model, _view, _mediaPool) {
* @return {void}
*/
this.skipAd = function(event) {
const autoPauseAds = _model.get('autoPause').pauseAds;
This conversation was marked as resolved by waxidiotic

This comment has been minimized.

Copy link
@vseventer

vseventer Aug 14, 2019

Collaborator

Is there a possibility _model.get('autoPause') doesn't return an object?

This comment has been minimized.

Copy link
@waxidiotic

waxidiotic Aug 14, 2019

Author Member

Nope.

const didNotAutostart = _model.get('playReason') !== 'autostart';
This conversation was marked as resolved by waxidiotic

This comment has been minimized.

Copy link
@vseventer

vseventer Aug 14, 2019

Collaborator

What do you think about naming this didAutostart instead? Is a bit easier to read imo.

This comment has been minimized.

Copy link
@waxidiotic

waxidiotic Aug 14, 2019

Author Member

I did initially but felt like keeping the variables positive in the conditional was better but I'm open to change if you feel like it is necessary.

This comment has been minimized.

Copy link
@pajong

pajong Aug 14, 2019

Member

I am fine either way

This comment has been minimized.

Copy link
@vseventer

vseventer Aug 14, 2019

Collaborator

I think it's best practice to avoid negative variable names - let's imagine in the future we'd have to add a else if (!didNotAutostart); the double negative gets confusing.

This comment has been minimized.

Copy link
@waxidiotic

waxidiotic Aug 14, 2019

Author Member

Good point. I'll change it.

const viewable = _model.get('viewable');
if (autoPauseAds && didNotAutostart && !viewable) {
// If autoPause.pauseAds is enabled and player is not viewable
// when skipAd() is called, do not resume playback unless player
// was autostarted out of view and never came in to view
this.noResume = true;
}

const skipAdType = AD_SKIPPED;
this.trigger(skipAdType, event);
_skipAd.call(this, {
@@ -469,9 +479,7 @@ const InstreamAdapter = function(_controller, _model, _view, _mediaPool) {
// when instream was inited and the player was not destroyed\
_controller.attachMedia();

const autoPauseAds = _model.get('autoPause').pauseAds;
const playerState = _model.get('state');
if (this.noResume || (playerState === STATE_PAUSED) && autoPauseAds) {
if (this.noResume) {
return;
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.