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

Conversation

@waxidiotic
Copy link
Member

commented Aug 13, 2019

This PR supercedes #3465 .

JW8-10170

This PR will...

  • Make it so we are not checking for the player state when destroying instream. The check was previously put in for the pauseAds work but caused issues (see next section).
  • Check whether or not we should resume when calling skipAd(), either via the skip button or the API.
    • If player autostarted and was never viewable and skipAd() is called, resume playback.
    • If player autostarted, was viewable and then paused due to viewability and skipAd() is called, do not resume playback.

Why is this Pull Request needed?

  • The logic for when deciding whether or not to resume content playback when destroying the instream adapter was flawed as it checked for when the player state was paused. This is not good because we destroy instream on setup and, when doing so, set the player's state to paused. This would cause instream to not call to play the content once destroyed.
  • The pauseAds logic was also incorrectly implemented so that the player would be paused, whether in ads mode or not, if it was autostarting out of view and pauseAds: true.

Are there any points in the code the reviewer needs to double check?

  • I checked everything I can think of but the one edge case that I'm not sure if we need to worry about is the following:
    • Autostarting in view
    • Skippable ad playback is paused by going out of view
    • Skipping an ad within an ad pod

In this scenario, the next pod item will play, even though the player is out of view. Once the entire pod is finished, playback will not resume. The issue is that subsequent pod items will play even though the player was auto-paused due to viewability. I don't think this is a widely used use case, however.

(cc @egreaves )

Are there any Pull Requests open in other repos which need to be merged with this?

n/a

Addresses Issue(s):

JW8-10170

@johnBartos

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Warnings
⚠️

🛠 There are modified src files, but no test changes. Add tests if you're able to.

Generated by 🚫 dangerJS

@jwplayer-robot

This comment was marked as outdated.

Copy link

commented Aug 13, 2019

❗️ MULTI Build for commit b1b9dd8 did not complete (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer unit tests SUCCESS
🏗 jwplayer-commercial build ${JW7_COMMERCIAL_BUILD_RESULT}
🏗 jwplayer-commercial unit tests FAILURE
🥒.js Allure Report ${JW7_COMMERCIAL_CUCUMBER_WDIO_BUILD_RESULT}
🥒 Allure Report ${JW7_COMMERCIAL_CUCUMBER_BUILD_RESULT}
🍆 Manual Tests
📺 Views

@waxidiotic

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

test this please

@jwplayer-robot

This comment has been minimized.

Copy link

commented Aug 14, 2019

⚠️ MULTI Build for commit b1b9dd8 is unstable (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer unit tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial unit tests SUCCESS
🥒.js Allure Report FAILURE
🥒 Allure Report UNSTABLE
🍆 Manual Tests
📺 Views

@waxidiotic

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Allure failures are known issues or otherwise unrelated.

@pajong

pajong approved these changes Aug 14, 2019

Copy link
Member

left a comment

Once you and Mark has an agreement on the variable name, I think this is good to go

@waxidiotic waxidiotic merged commit dc4034b into master Aug 14, 2019

3 of 4 checks passed

jw7-pr-multi-opensource Build started sha1 is merged.
Details
Danger ⚠️ Danger found some issues. Don't worry, everything is fixable.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@waxidiotic waxidiotic deleted the bugfix/jw8-10170_pauseAds branch Aug 14, 2019

@robwalch robwalch removed their request for review Aug 14, 2019

@jwplayer-robot

This comment has been minimized.

Copy link

commented Aug 14, 2019

⚠️ MULTI Build for commit c0cb6df is unstable (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer unit tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial unit tests SUCCESS
🥒.js Allure Report FAILURE
🥒 Allure Report UNSTABLE
🍆 Manual Tests
📺 Views

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.