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.

Sign up for GitHub

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

Fix: content can play too early after contentupdate #318

Closed
wants to merge 7 commits into from
Closed

Fix: content can play too early after contentupdate #318

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
90f5dba
call pause() on contentupdate if not already paused, add contentupdat…
alex-barstow Jan 19, 2018
099f217
added 2 tests
alex-barstow Jan 25, 2018
1ee370b
added one more test, modified existing test
alex-barstow Jan 25, 2018
45cc8f2
fix confusing phrasing
alex-barstow Jan 25, 2018
3923e3e
make tests clearer, recommended changes
alex-barstow Jan 25, 2018
17ff5b1
code review changes
alex-barstow Jan 31, 2018
839755a
fix test
alex-barstow Jan 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ const contribAdsPlugin = function(options) {
// Restart the cancelContentPlay process.
player.on('playing', () => {
player.ads._cancelledPlay = false;
player.ads._pausedOnContentupdate = false;
});

player.one('loadstart', () => {
Expand Down Expand Up @@ -435,6 +436,9 @@ const contribAdsPlugin = function(options) {
},
nopreroll() {
this.state = 'content-playback';
},
contentupdate() {
this.state = 'content-set';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a loadstart/contentupdate for the initial source. Is there any guarantee that this isn't that event? Specifically, if the play / adsready came in before loadstart.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'm not sure there is any such guarantee. We could add a safeguard, something like:

contentupdate() {
  // only change states if this is not the first loadstart/contentupdate
  if (player.ads._hasThereBeenALoadStartDuringPlayerLife) {
    this.state = 'content-set';
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to test it to make sure _hasThereBeenALoadStartDuringPlayerLife is set, it's also set on loadstart so the order of event handlers matters here.

Copy link
Contributor Author

@alex-barstow alex-barstow Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hindsight, this check won't have any value for subsequent videos in a playlist since _hasThereBeenALoadStartDuringPlayerLife will be true for the life of the player after the first loadstart, as the name suggests :P. Will need to find another way

}
}
},
Expand Down Expand Up @@ -638,10 +642,10 @@ const contribAdsPlugin = function(options) {
triggerevent: player.ads.triggerevent
});

// Play the content if cancelContentPlay happened and we haven't played yet.
// This happens if there was no preroll or if it errored, timed out, etc.
// Otherwise snapshot restore would play.
if (player.ads._cancelledPlay) {
// Play the content if cancelContentPlay happened or we paused on 'contentupdate'
// and we haven't played yet. This happens if there was no preroll or if it
// errored, timed out, etc. Otherwise snapshot restore would play.
if (player.ads._cancelledPlay || player.ads._pausedOnContentupdate) {
if (player.paused()) {
player.play();
}
Expand All @@ -666,6 +670,10 @@ const contribAdsPlugin = function(options) {
if (player.paused()) {
this.state = 'content-set';
} else {
// If we get here we missed the 'play' event, so pause the player so
// that content doesn't start playing immediately after the source change
player.pause();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may want to pause the player if it's not paused yet and then go to the same state content-set.

If I understand correctly, the reason this else block will go to ads-ready? is that there is an assumption that the play event has been missed. So once the player is paused, we should be in the same state that causes the if clause to change state to content-set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an offline conversation we came to the conclusion that the state transition to ads-ready? is needed still.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if contentupdate for the initial source happens during content-playback? This can happen with autoplay.

player.ads._pausedOnContentupdate = true;
this.state = 'ads-ready?';
}
},
Expand Down
39 changes: 39 additions & 0 deletions test/test.ads.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,45 @@ QUnit.test('adsready in content-playback triggers readyforpreroll', function(ass
assert.strictEqual(spy.getCall(0).args[0].type, 'readyforpreroll', 'readyforpreroll should have been triggered.');
});

QUnit.test('contentupdate in preroll? transitions to content-set', function(assert) {
assert.expect(2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend avoiding using expect unless your code has assertions in a loop. For simple procedural tests, the expect doesn't add value and requires maintenance if the test is updated.


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe all these tests should trigger loadstart before play as well, to make sure the player is setup correctly

this.player.trigger('play');
this.player.trigger('adsready');
assert.strictEqual(this.player.ads.state, 'preroll?');
this.player.trigger('contentupdate');
assert.strictEqual(this.player.ads.state, 'content-set');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should avoid tests that assert the state. Firstly, they will break very shortly. Secondly, it is preferable to test things that indicate that the player is functioning correctly. For example, making sure it is paused if it should be paused.

Copy link
Contributor Author

@alex-barstow alex-barstow Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, I'll remove these first two tests in their entirety since they are just testing that certain state transitions occur under the proper conditions. Is that what you would recommend in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, it does seem like the third test covers the functionality you've implemented here.

});

QUnit.test('contentupdate in content-playback transitions to content-set if the player is paused', function(assert) {
assert.expect(2);

this.player.trigger('play');
this.player.trigger('adtimeout');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just do this.players.ads.skipLinearAdMode() here, or explicitly do startLinearAdMode() and endLinearAdMode() right after.

assert.strictEqual(this.player.ads.state, 'content-playback');
this.player.paused = function() {
return true;
};
this.player.trigger('contentupdate');
assert.strictEqual(this.player.ads.state, 'content-set');
});

QUnit.test('contentupdate in content-playback transitions to ads-ready? and pauses player if not already paused', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: remove state reference from test name

assert.expect(4);

this.player.trigger('play');
this.player.trigger('adtimeout');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same feedback for this line as the above test

assert.strictEqual(this.player.ads.state, 'content-playback');
this.player.paused = function() {
return false;
};
sinon.spy(this.player, 'pause');
this.player.trigger('contentupdate');
assert.strictEqual(this.player.ads.state, 'ads-ready?');
assert.ok(this.player.pause.calledOnce, 'player was paused');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertions on pause.calledOnce and _pausedOnContentUpdate are good, you can remove the assertions on the state. The test remains just as valuable and would survive the refactor.

assert.ok(this.player.ads._pausedOnContentupdate, '_pausedOnContentupdate is true');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also test that the play happens?

});

// ----------------------------------
// Event prefixing during ad playback
// ----------------------------------
Expand Down