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

Added type to content source #433

Merged
merged 5 commits into from
Oct 24, 2017
Merged

Added type to content source #433

merged 5 commits into from
Oct 24, 2017

Conversation

valse
Copy link
Contributor

@valse valse commented Oct 17, 2017

Resolved an issue with IMA plugin and HLS plugin on post-roll completed ads: without the source type the player hangs with a format not supported error.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@valse
Copy link
Contributor Author

valse commented Oct 17, 2017

I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@shawnbuso
Copy link
Contributor

Added a small comment. Thanks for the PR!

@valse
Copy link
Contributor Author

valse commented Oct 18, 2017

Hi,
I've tried the "simple" example changing the source with an HLS one (I used the "videojs-contrib-hls" 5.11.1 plugin and upgraded the video.js to 5.19.1 version to support the HLS plugin).

After the post-roll complete:
vjs-ima-postroll

the player crash with a not supported format error:
vjs-ima-hls-error

because it needs to know the correct source type.

@shawnbuso
Copy link
Contributor

Makes sense - once you resolve the comment I left and the CLA we should be good to go :)

@valse
Copy link
Contributor Author

valse commented Oct 18, 2017

Hi,
I'm sorry but I didn't see any comment on your side :-
I've already solve the CLA changing the email author... what I'm wrong?

@googlebot
Copy link

CLAs look good, thanks!

@@ -1333,6 +1337,7 @@
* post-roll on iOS.
*/
this.contentSource = '';
this.contentSourceType = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment for this like we have for the other variables indicating what it is?

@shawnbuso
Copy link
Contributor

Oh whoops, it looks like I started a review but never submitted it - you should see it now.

@shawnbuso
Copy link
Contributor

Thanks for the fix!

@shawnbuso shawnbuso merged commit 1232b41 into googleads:master Oct 24, 2017
shawnbuso pushed a commit that referenced this pull request Oct 27, 2017
* Initial commit for gh-pages

* Added a link to the iPhone sample on the gh-pages index.

* Merging latest updates from master

* Updating videojs-contrib-ads to 3.1.2

* Added autoplay example to samples index.

* Tech agnostic contentPlayer

Changed contentPlayer selector to be tech agnostic (will get object
irrespective of it being an html5_api or Flash_api instance). Adds
compatibility for IE 11 browsers using Flash and HLS.

* Revert "Merge remote-tracking branch 'origin/gh-pages'"

This reverts commit d3b9e49, reversing
changes made to 1232b41.

* Revert "Merge pull request #433 from valse/master"

This reverts commit 1232b41, reversing
changes made to dcb063f.

# Conflicts:
#	src/videojs.ima.js

* Revert "Revert "Merge pull request #433 from valse/master""

This reverts commit 0c6651a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants