Ad state refactor #315

incompl · 2018-01-02T22:55:59Z

No description provided.

ldayananda

This is looking pretty exciting. I have to come back to review the test changes again.

ldayananda · 2018-02-01T18:46:58Z

README.md

@@ -66,7 +66,7 @@ And here are the interaction points you use to send information to the ads plugi
 * `adplaying` (EVENT) - Trigger this event when an ads starts playing. If your integration triggers `playing` event when an ad begins, it will automatically be redispatched as `adplaying`.
 * `adscanceled` (EVENT) — Trigger this event after starting up the player or setting a new video to skip ads entirely. This event is optional; if you always plan on displaying ads, you don't need to worry about triggering it.
 * `adserror` (EVENT) - Trigger this event to indicate that an error in the ad integration has ocurred and any ad states should abort so that content can resume.
-* `nopreroll` (EVENT) - Trigger this event to indicate that there will be no preroll ad. Otherwise, the player will wait until a timeout occurs before playing content. This event is optional, but can improve user experience.
+* `nopreroll` (EVENT) - Trigger this event to indicate that there will be no preroll ads to skip the preroll process completely. This event is optional, but can improve user experience. It is ignored if it is seen after `readyforpreroll`.


not sure I understand the first part of this sentence

Looks like something got pasted wrong. Fixing

ldayananda · 2018-02-01T18:52:22Z

README.md

@@ -437,9 +437,9 @@ player.src('movie-high.mp4');
 ### disableNextSnapshotRestore
 Prevents videojs-contrib-ads from restoring the previous video source

-If you need to change the video source during ad playback, you can use _disableNextSnapshotRestore_ to prevent videojs-contrib-ads to restore to the previous video source.
+If you need to change the video source during an ad break, you can use _disableNextSnapshotRestore_ to prevent videojs-contrib-ads to restore to the previous video source.


~~to prevent videojs-contrib-ads to restore~~ --> to prevent videojs-contrib-ads from restoring the snapshot saved from the previous video source.

Making this change

ldayananda · 2018-02-01T18:53:03Z

README.md

 ```js
-if (player.ads.state  === 'ad-playback') {
+if (player.ads.inAdBreak()) {


We should add a definition for inAdBreak too

Updating docs

ldayananda · 2018-02-01T20:45:34Z

example/example-integration.js

@@ -100,7 +100,7 @@
    }

    player.on('contentended', function() {
-      if (!state.postrollPlayed && player.ads.state === 'postroll?' && playPostroll) {
+      if (!state.postrollPlayed && playPostroll) {


should this check for isInAdMode?

No, contentended cannot happen in ad mode.

ldayananda · 2018-02-01T20:46:05Z

scripts/umd.rollup.config.js

@@ -40,7 +40,7 @@ export default {
        }]
      ],
      plugins: [
-        // 'external-helpers',
+        'external-helpers',


Is this an intended change?

Yes, Pat suggested it was probably not commented out on purpose.

ldayananda · 2018-02-01T21:58:31Z

src/states/Postroll.js

+  startLinearAdMode() {
+    const player = this.player;
+
+    if (!player.ads.isAdPlaying() && !this.contentResuming) {


old name isAdPlaying should be changed, and use the method for content resuming too

ldayananda · 2018-02-01T21:59:25Z

src/states/Postroll.js

+   *
+   */
+  onAdStarted(player) {
+    player.removeClass('vjs-ad-loading');


can this go in the AdState class?

vjs-ad-loading management is very different for prerolls and postrolls, so to keep it as clear as possible I isolated it to those classes rather than trying to share code.

I am updating the empty comment here though.

ldayananda · 2018-02-01T22:00:41Z

src/states/Postroll.js

+    }
+  }
+
+  abort() {


Any reason why this is so different to the other AdState subclasses?

It just evolved to be this way based on what this class was doing. I'm open to other ideas, but this doesn't seem problematic per se.

Not necessarily problematic, it just seemed to me at first glance that most of what is in abort () can go in cleanup(). Except for the contentResuming change and triggering ended, I suppose.

I think ideally it would all go in cleanup, but I'm hesitant to change the ended event more than necessary, just to keep the risk of this PR to a minimum. Definitely something to consider in the future.

ldayananda · 2018-02-01T22:01:39Z

src/states/Postroll.js

+  }
+
+  onNoPostroll(player) {
+    if (!this.contentResuming && !player.ads.inAdBreak()) {


another place to switch to the methods

ldayananda · 2018-02-01T22:03:04Z

src/states/AdsDone.js

+   */
+  startLinearAdMode() {
+    videojs.log.warn('Unexpected startLinearAdMode invocation (AdsDone)');
+  }


It logically makes sense to trigger ended in this state to me...what do you think?

I'm not sure it's safe to do that. For example, one of the ended events happens in a 1 millisecond timeout, but not others. I think this is a good area of future improvement, but I don't think it can be safely changed in this PR without lots of re-testing.

ldayananda · 2018-02-05T16:06:14Z

README.md

+
+#### inAdBreak()
+
+This method returns true during the time between startLinearAdMode and endLinearAdMode where an integratino may play ads. This is part of ad mode.


Typo integratino

ldayananda · 2018-02-05T16:09:53Z

src/plugin.js

@@ -155,7 +133,7 @@ const contribAdsPlugin = function(options) {

  // Replace the plugin constructor with the ad namespace
  player.ads = {
-    state: 'content-set',
+    settings,


Yeah, and that state is no longer available and should not have been relied on

ldayananda · 2018-02-05T16:47:42Z

example/example-integration.js

@@ -92,7 +92,7 @@
    // initialize the ads plugin, passing in any relevant options
    player.ads(options);

-    // request ad inventory whenever the player gets new content to play
+    // request ad inventory whenever the player gets content to play


maybe this listener should be updated to listen to contentchanged?

Originally I didn't change it because it seems to run on the initial contentupdate. Now that I look at it again it seems like it might request ads twice sometimes as it is? I am going to update it.

ldayananda · 2018-02-05T16:51:25Z

src/states/Midroll.js

@@ -17,7 +17,6 @@ export default class Midroll extends AdState {
    const player = this.player;

    if (!this.inAdBreak() && !this.isContentResuming()) {
-      player.ads.debug('startLinearAdMode (Midroll)');


why remove this log?

I had added it in a relatively recent commit and found it's basically redundant with the "Starting ad break" log.

ldayananda · 2018-02-05T16:51:49Z

src/states/Postroll.js

@@ -43,7 +43,6 @@ export default class Postroll extends AdState {
    const player = this.player;

    if (!player.ads.isAdPlaying() && !this.contentResuming) {
-      player.ads.debug('startLinearAdMode (Postroll)');


why remove this log?

I had added it in a relatively recent commit and found it's basically redundant with the "Starting ad break" log.

ldayananda · 2018-02-05T16:56:59Z

src/states/abstract/State.js

@@ -106,8 +106,8 @@ export default class State {
      this.onAdTimeout(player);
    } else if (type === 'ads-ad-started') {
      this.onAdStarted(player);
-    } else if (type === 'contentupdate') {
-      this.onContentUpdate(player);
+    } else if (type === 'contentchanged') {


just for confirmation: contentupdate will no longer be used for state transitions?

ldayananda · 2018-02-05T17:05:56Z

test/test.ads.js

 };

 QUnit.module('Ad Framework', window.sharedModuleHooks());

-QUnit.test('begins in content-set', function(assert) {


seems like a good test to replace rather than delete

ldayananda · 2018-02-05T18:48:33Z

test/test.ads.js

@@ -47,29 +41,26 @@ QUnit.test('pauses to wait for prerolls when the plugin loads AFTER play', funct
 QUnit.test('stops canceling play events when an ad is playing', function(assert) {
  var setTimeoutSpy = sinon.spy(window, 'setTimeout');


we should probably change this to spy on the the player.setTimeout method

player.setTimeout ultimately calls window.setTimeout so it ends up being the same. I think a better improvement for this test would be to test the functionality more directly. The test description says "stops canceling play events when an ad is playing". A better test would start an ad playing and see if play events get canceled. There are tons of improvements that could be made to these tests, so I had to pick my battles as I updated these.

ldayananda · 2018-02-05T18:49:59Z

test/test.ads.js


  this.player.trigger('adsready');
-  assert.strictEqual(setTimeoutSpy.callCount, 3, '`adTimeoutTimeout` was re-scheduled');


Was this line was intentionally removed?

Yes, the implementation changed without the behavior changing and this assertion started failing without an underlying defect in the code. I removed it to help our tests focus more on tests of functionality and reduce false positives.

ldayananda · 2018-02-05T19:04:38Z

test/test.ads.js

+  this.clock.tick(1);
+  this.player.ads.startLinearAdMode();
+  this.player.ads.endLinearAdMode();
+  assert.notOk(timerExists(this, this.player.ads.cancelPlayTimeout), '`cancelPlayTimeout` was canceled');


test message seems wrong - play was canceled ? Or do we want the timeout to not exist?

It's saying that the timeout named "cancelPlayTimeout" was canceled, confirmed by the fact that the variable saving the timeout ID was deleted.

ldayananda · 2018-02-05T19:06:28Z

test/test.ads.js


-  this.clock.tick(1);
-  assert.strictEqual(this.player.ads.state, 'content-playback');
+  this.clock.tick(1000);


this can go back to 1 now, right?

Yeah changing it back

ldayananda · 2018-02-05T19:09:32Z

test/test.ads.js

-  this.player.trigger('playing');
-  assert.strictEqual(this.player.ads.state, 'content-playback');
-  assert.strictEqual(spy.getCall(0).args[0].type, 'adend', 'adend should be fired when we enter content-playback from adserror');
+  assert.strictEqual(adendSpy.callCount, 0, 'no adend yet');


what is this test testing now?

Updated this test to do what it says it should

ldayananda · 2018-02-05T19:15:53Z

test/test.ads.js

+  assert.strictEqual(sawAdpause.callCount, 1, 'adpause fired');
+  assert.strictEqual(sawAdended.callCount, 1, 'adended fired');
+  assert.strictEqual(sawAdfirstplay.callCount, 1, 'adfirstplay fired');
+  assert.strictEqual(sawAdloadedalldata.callCount, 1, 'adloadedalldata fired');


I think that firstplay, loadstart, playing, loadedalldata, pause, ended were meant to be checked as well to make sure that they were canceled?

Looks like those are there, above this line.

Ah missed that 🆒

ldayananda · 2018-02-05T19:17:49Z

test/test.ads.js

 });

 QUnit.test('ad impl can notify contrib-ads there is no preroll', function(assert) {

-  this.player.ads.state = 'preroll?';
+  this.player.trigger('loadstart');
+  this.player.trigger('nopreroll');


what happens if nopreroll is triggered after the first of play or adsready? i.e loadstart -> play -> nopreroll

Added two more tests to cover these variants.

ldayananda · 2018-02-05T19:20:24Z

test/test.ads.js

  this.player.trigger('contentended');

  this.clock.tick(10000);
-
  assert.ok(ended.calledOnce, 'Ended triggered');


seems like this is a duplicate of the previous test now

removing the duplicate test

ldayananda · 2018-02-05T19:21:11Z

test/test.ads.js

@@ -1062,7 +887,8 @@ QUnit.test('Check incorrect addition of vjs-live during ad-playback', function(a

 QUnit.test('Check for existence of vjs-live after ad-end for LIVE videos',
  function(assert) {
-    this.player.trigger('adstart');
+    this.player.trigger('adsready');


ldayananda · 2018-02-13T23:34:59Z

test/test.events-no-postroll.js

  var endedEvents = 0;

+  this.player.ads.settings.postrollTimeout = 1;


why was this necessary?

The test times out otherwise. This is just so the test runs faster basically. I'll add a comment.

ldayananda · 2018-02-13T23:41:23Z

README.md

+
+If you are loading `videojs-contrib-ads` using modules, do this:
+
+TODO


This TODO still meant to be here?

Yeah, I want to link to the example from #312 once that is merged. Was planning to do that in rebase. I'll add more info.

ldayananda · 2018-02-13T23:44:31Z

README.md

+to end without an ad starting, in which case the spinner stays up the whole time.
+* Integration calls `player.ads.endLinearAdMode()` (METHOD) -- This ends an ad break. As a result, content will play.
+* Content plays.
+* To play a Midroll ad, start an end an ad break with `player.ads.startLinearAdMode()` and `player.ads.endLinearAdMode()` at any time during content playback.


Typo ~~start an end an ad break~~ start and end an ad break

ldayananda · 2018-02-13T23:45:06Z

README.md

+* Content plays.
+* To play a Midroll ad, start an end an ad break with `player.ads.startLinearAdMode()` and `player.ads.endLinearAdMode()` at any time during content playback.
+* Contrib Ads triggers `contentended` (EVENT) -- This event means that it's time to play a postroll ad.
+* To play a Postroll ad, start an end an ad break with `player.ads.startLinearAdMode()` and `player.ads.endLinearAdMode()`.


ldayananda · 2018-02-13T23:46:29Z

README.md

+This is the basic flow for a simple use case, but there are other things the integration can do:
+
+* `skipLinearAdMode` (METHOD) -- At a time when `startLinearAdMode` is expected, calling `skipLinearAdMode` will immediately resume content playback instead.
+* `nopreroll` (EVENT) -- You can trigger this event even before `readyforpreroll` to indicate that no preroll will play. The ad plugin will not check for prerolls and will instead immediately begin content playback after the `play` event (or immidiately, if playback was already requested).


Typo immidiately.

Also I would delete the first immediately in this sentence

ldayananda · 2018-02-13T23:49:09Z

README.md

+* `skipLinearAdMode` (METHOD) -- At a time when `startLinearAdMode` is expected, calling `skipLinearAdMode` will immediately resume content playback instead.
+* `nopreroll` (EVENT) -- You can trigger this event even before `readyforpreroll` to indicate that no preroll will play. The ad plugin will not check for prerolls and will instead immediately begin content playback after the `play` event (or immidiately, if playback was already requested).
+* `nopostroll` (EVENT) -- Similar to `nopreroll`, you can trigger this event even before `contentended` to indicate that no postroll will play.  The ad plugin will not wait for a postroll to play and will instead immediately trigger the `ended` event.
+* `adserror` (EVENT) -- This event skips prerolls when seen before a preroll ad break. It skips postrolls if called after contentended and before a postroll ad break. It ends linear ad mode if seen during an ad break.


We should say when the integration should trigger this event

I don't think there is a clear answer to that, it depends on if you want the extra behavior or not. I think we need to change how adserror works. For now, just want to make it clear what it does.

To clarify, for prerolls and postrolls the break is skipped if it is triggered before ad mode or before the ad break is started? I think that's not clear for prerolls

It says before the ad break, which is exactly accurate.

ldayananda · 2018-02-13T23:50:15Z

README.md

-Note: A `contentplayback` event is sent but should not be used as it is being removed. The `playing` event has the same meaning and is far more reliable.
-
-And here are the interaction points you use to send information to the ads plugin:
+ * `contentchanged` (EVENT) -- Fires when a new content video has been loaded in the player (specially, at the same time as the `loadstart` media event for the new source). This means the ad workflow has restarted from the beginning. Your integration will need to trigger `adsready` again, for example. Note that when changing sources, the playback state of the player is retained: if the previous source was playing, the new source will also be playing and the ad workflow will not wait for a new `play` event.


specially? Or specifically?

Updated to specifically

ldayananda · 2018-02-13T23:51:51Z

README.md


-In addition, video.js provides a number of events and APIs that might be useful to you.
-For example, the `ended` event signals that the content video has played to completion.
+* `contentplayback` (EVENT) -- The `playing` media event has the same meaning and is far more reliable.


Although I like these descriptions, they seem to fit better in the migration guide. I would then link to the most recent migration guide in this section

Oh yeah, since we're removing it in this version we can move it from "deprecated" to the migration guide.

ldayananda · 2018-02-13T23:53:07Z

README.md


 #### isInAdMode()

-Returns true if player is in ad mode.
+Returns true if the player is in ad mode.


didn't this change to be isInAdBreak?

Ad mode includes loading ads and content resuming. The ad break is just a part of that.

Ah sorry, keep getting confused

ldayananda · 2018-02-13T23:56:45Z

README.md


 ### adskip
-The player is skipping a linear ad opportunity and content-playback should resume immediately.  This event is fired directly as a consequence of calling `skipLinearAdMode()`. It can indicate that an ad response was made but returned no linear ad content or that no ad call is going to be made at either the preroll or postroll timeout opportunities.
+The player is skipping a linear ad opportunity and content-playback should resume immediately.  This event is fired directly as a consequence of calling `skipLinearAdMode()`. As examples, it can indicate that an ad response was made but returned no linear ad content or that no ad call is going to be made due to an error.


As For example? Also I think you meant ad request?

Updating this

ldayananda

Looks mostly good, I'll wait for the tests to pass before approving

ldayananda · 2018-02-14T23:41:43Z

README.md

+* `skipLinearAdMode` (METHOD) -- At a time when `startLinearAdMode` is expected, calling `skipLinearAdMode` will immediately resume content playback instead.
+* `nopreroll` (EVENT) -- You can trigger this event even before `readyforpreroll` to indicate that no preroll will play. The ad plugin will not check for prerolls and will instead immediately begin content playback after the `play` event (or immidiately, if playback was already requested).
+* `nopostroll` (EVENT) -- Similar to `nopreroll`, you can trigger this event even before `contentended` to indicate that no postroll will play.  The ad plugin will not wait for a postroll to play and will instead immediately trigger the `ended` event.
+* `adserror` (EVENT) -- This event skips prerolls when seen before a preroll ad break. It skips postrolls if called after contentended and before a postroll ad break. It ends linear ad mode if seen during an ad break.


To clarify, for prerolls and postrolls the break is skipped if it is triggered before ad mode or before the ad break is started? I think that's not clear for prerolls

ldayananda · 2018-02-14T23:42:45Z

README.md


 Deprecated events:

-* `contentplayback` (EVENT) -- The `playing` media event has the same meaning and is far more reliable.
 * `contentupdate` (EVENT) -- Replaced by `contentchanged`, which is more reliable.


I meant for these two lines to move to the migration guide as well

I'll add a "deprecation" section to the migration guide and will leave this here so there is still a deprecation warning in the readme.

ldayananda

minor comments, overall really liking the new tests

ldayananda · 2018-02-20T22:50:14Z

test/states/abstract/test.AdState.js

+
+    this.adState = new AdState(this.player);
+    this.adState.transitionTo = (newState) => {
+      this.transitionTo = newState.name;


I think transitionTo is a confusing name since it sounds like a method. Maybe transitionedTo?

I was also considering transitionedTo and newState, newState is probably the most obvious

ldayananda · 2018-02-20T22:55:01Z

test/states/abstract/test.ContentState.js

+  this.contentState.onContentChanged(this.player);
+  assert.equal(this.transitionTo, 'Preroll');
+  assert.equal(this.player.pause.callCount, 1, 'paused player');
+  assert.equal(this.player.ads._pausedOnContentupdate, true, 'set _pausedOnContentupdate');


I think this should have been checked on the last test, right?

it's extra, might as well add it

ldayananda · 2018-02-20T23:00:51Z

test/states/test.ContentPlayback.js

+  this.player.ads._cancelledPlay = false;
+  this.player.ads._pausedOnContentupdate = false;
+  this.contentPlayback.init(this.player);
+  assert.equal(this.played, false);


this doesn't seem right since paused() -> false

maybe played should be playTriggered

changing it

ldayananda · 2018-02-20T23:03:49Z

test/states/test.Midroll.js

+
+QUnit.test('ends an ad break on endLinearAdMode', function(assert) {
+  this.midroll.init(this.player);
+  this.player.ads._inLinearAdMode = true;


do you have to hardcode this? could we do this as part of the stub?

I wanted the stub to reflect an "initial" state and have each test advance it forward to the necessary state, rather than having the stub be kind of a random assortment of properties.

I see. Would calling startLinearAdMode be equivalent then?

Yeah, originally I wanted to limit the test to only calling one function, but looking back on it now, using startLinearAdMode is a more solid test.

My mistake. startLinearAdMode is not defined in Midroll.js, so that's not an option.

Ah, this.player.ads._inLinearAdMode is always mocked in this test file. I suppose that is fine then

Ah! Just realized that when you enter the Midroll state, _inLinearAdMode would always be true, so it make sense for this to change in the mock. We've gone full circle on this one.

ldayananda · 2018-02-20T23:07:20Z

test/states/test.Postroll.js

+  this.postroll.init(this.player);
+  this.player.ads.inAdBreak = () => false;
+  this.postroll.onAdsError(this.player);
+  assert.equal(this.player.ads.endLinearAdMode.callCount, 0, 'linear ad mode ended');


does ended get called?

might as well assert not

sorry the other way, we assert that ended is still triggered

ldayananda · 2018-02-20T23:11:44Z

test/states/test.Preroll.js

+  assert.equal(this.preroll.inAdBreak(), false, 'not in ad break');
+
+  this.preroll.startLinearAdMode();
+  this.player.ads._inLinearAdMode = true; // Because adBreak.start is mocked.


comments should go on the line above

ldayananda · 2018-02-20T23:12:28Z

test/states/test.Preroll.js

+  this.preroll.startLinearAdMode();
+  this.player.ads._inLinearAdMode = true; // Because adBreak.start is mocked.
+  assert.equal(this.adBreakStartStub.callCount, 1, 'ad break started');
+  assert.equal(this.player.ads.adType, 'preroll', 'adType is preroll');


did we have one of these for postroll?

ldayananda · 2018-02-20T23:18:44Z

Also, this comment got hidden: #315 (comment)

incompl · 2018-02-21T16:15:58Z

That link isn't taking me to the right place, which comment is that?

…

On Tue, Feb 20, 2018 at 6:18 PM, ldayananda ***@***.***> wrote: Also, this comment got hidden: #315 (comment) <#315 (comment)> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAM4Z9MZj22eAtB8KQd-P884UGOfhHKiks5tW1LVgaJpZM4RRLIn> .

…king

ldayananda

I think you actually got the comment that was hidden.

ldayananda · 2018-02-21T21:35:09Z

src/states/BeforePreroll.js

@@ -57,7 +57,7 @@ export default class BeforePreroll extends ContentState {
   */
  onNoPreroll() {
    this.player.ads.debug('Skipping prerolls due to nopreroll event (BeforePreroll)');
-    this.transitionTo(Preroll, this.adsReady);
+    this.transitionTo(ContentPlayback);


ldayananda · 2018-02-22T16:59:12Z

README.md

+* `skipLinearAdMode` (METHOD) -- At a time when `startLinearAdMode` is expected, calling `skipLinearAdMode` will immediately resume content playback instead.
+* `nopreroll` (EVENT) -- You can trigger this event even before `readyforpreroll` to indicate that no preroll will play. The ad plugin will not check for prerolls and will instead begin content playback after the `play` event (or immediately, if playback was already requested).
+* `nopostroll` (EVENT) -- Similar to `nopreroll`, you can trigger this event even before `contentended` to indicate that no postroll will play.  The ad plugin will not wait for a postroll to play and will instead immediately trigger the `ended` event.
+* `adserror` (EVENT) -- This event skips prerolls when seen before a preroll ad break. It skips postrolls if called after contentended and before a postroll ad break. It ends linear ad mode if seen during an ad break.


The thread might get a bit convoluted so I'll just summarize: for prerolls, if you trigger adserror in BeforePreroll, will it skip ads?

ldayananda · 2018-02-22T17:02:58Z

test/states/test.Midroll.js

+
+QUnit.test('ends an ad break on endLinearAdMode', function(assert) {
+  this.midroll.init(this.player);
+  this.player.ads._inLinearAdMode = true;


I see. Would calling startLinearAdMode be equivalent then?

ldayananda

LGTM

This was referenced Jan 24, 2018

Feedback wanted on refactor #320

Closed

Outstanding ad timeouts still trigger after content video changed #161

Closed

ldayananda reviewed Feb 1, 2018

View reviewed changes

incompl force-pushed the ad-state-refactor branch from b9654f4 to 75a1f01 Compare February 2, 2018 20:28

ldayananda reviewed Feb 5, 2018

View reviewed changes

alex-barstow mentioned this pull request Feb 5, 2018

Fix: content can play too early after contentupdate #318

Closed

ldayananda reviewed Feb 5, 2018

View reviewed changes

incompl changed the title ~~[WIP] Ad state refactor~~ Ad state refactor Feb 13, 2018

ldayananda reviewed Feb 13, 2018

View reviewed changes

ldayananda reviewed Feb 14, 2018

View reviewed changes

ldayananda reviewed Feb 20, 2018

View reviewed changes

Greg Smith added 16 commits February 21, 2018 11:49

first

c4f92b2

lint

ffeb62a

test stuff

a248e63

fix tests

a65fbe8

finish first pass at BeforePreroll.js

e3432a0

wip

a721c03

fix "this" bugs in endlinearadmode

a3271a4

wip

6e0141d

get rollup and imports worked out

c4ed56e

progress

b436b17

fix ended event w/ no postroll

14bbec4

cleanup

e1bcaa0

finishing up postroll refactor

b00ceeb

fix some tests

6b54458

updating more tests

6fe4ac9

tests pass!

9d2627c

Greg Smith added 22 commits February 21, 2018 11:49

more tidiness

f37decc

roll back attempt to fix contentupdate beforepreroll, needs more thin…

77c12fe

…king

tweaks, hopefully close to done for this pr

ce2d2a3

fix bug with transitionTo

a7dacce

further improve transitionTo

af8aacd

lint

3d17d46

last things for this pr i promise maybe!

d78d1e4

finishing touches

6725e42

contentchanged

25d2669

code review changes

9f4a745

Code review updates / fixes

48171b4

code review changes to tests

3bf25bb

didnt mean to add this

e884fb0

update readme and timeouts

b79a936

fix typo

c6b749e

update migration guide

d59171a

update

09d652f

readme code review changes

086f176

see if this fixes test timeouts

50367c6

new state unit tests

0a60617

try to prevent timeout by speeding up test

73b18a9

test code review changes

e50d8fe

incompl force-pushed the ad-state-refactor branch from 1d82e5b to e50d8fe Compare February 21, 2018 16:53

Improve nopreroll handling in BeforePreroll

5b6291c

ldayananda reviewed Feb 22, 2018

View reviewed changes

code review feedback

360fe44

ldayananda approved these changes Feb 22, 2018

View reviewed changes

set contentResuming to true before ending midroll break

1431c33

incompl merged commit d11ef39 into master Feb 23, 2018

incompl deleted the ad-state-refactor branch February 23, 2018 15:42


		#### inAdBreak()

		This method returns true during the time between startLinearAdMode and endLinearAdMode where an integratino may play ads. This is part of ad mode.

		@@ -47,29 +41,26 @@ QUnit.test('pauses to wait for prerolls when the plugin loads AFTER play', funct
		QUnit.test('stops canceling play events when an ad is playing', function(assert) {
		var setTimeoutSpy = sinon.spy(window, 'setTimeout');


		this.player.trigger('adsready');
		assert.strictEqual(setTimeoutSpy.callCount, 3, '`adTimeoutTimeout` was re-scheduled');

		var endedEvents = 0;

		this.player.ads.settings.postrollTimeout = 1;


		If you are loading `videojs-contrib-ads` using modules, do this:

		TODO

Ad state refactor #315

Ad state refactor #315

Conversation

incompl commented Jan 2, 2018

ldayananda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment