Skip to content

Video header bidding support to RhythmOne bidder adapter#1052

Merged
protonate merged 3 commits intoprebid:masterfrom
jstocker76:master
Mar 28, 2017
Merged

Video header bidding support to RhythmOne bidder adapter#1052
protonate merged 3 commits intoprebid:masterfrom
jstocker76:master

Conversation

@jstocker76
Copy link
Contributor

@jstocker76 jstocker76 commented Mar 16, 2017

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other

Description of change

Added video header bidding support to RhythmOne bidder adapter

  • test parameters for validating bids
{
	bidder: 'rhythmone',
	params: {
		endpoint: "//1r-qa-ads.rhythmxchange.com/rmp/{placementId}/0/{path}?z={zone}",
		placementId: "389633",
		zone: "csmith5",
		path: "mvo",
		debug: true,
		method:"get"
	}
}
  • contact email of the adapter’s maintainer
  • official adapter submission

Other information

@matthewlane matthewlane self-assigned this Mar 20, 2017
@matthewlane matthewlane self-requested a review March 20, 2017 18:24
Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

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

The test params return bids for a regular ad unit, but I'm not getting bids when used with an ad unit configured with mediaType: 'video', should that be expected? Also a few notes below for your review

import {ajax as ajax} from '../ajax';

function track(debug, p1, p2, p3) {
function track(debug) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function doesn't seem to do anything now, is it still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use google analytics to test this bidder on some of our own sites, and use this function for that purpose. Having to add it and all its references back, only to remove it before a push, repeatedly, would be cumbersome.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, thanks for clarifying

var d = global.document.location.ancestorOrigins;
if(d && d.length > 0)
return d[d.length-1];
return global.top.document.location.hostname;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please wrap any top references in a try/catch block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attempt function wraps this callback in a try/catch - somewhere around line 78

return d[d.length-1];
return global.top.document.location.hostname;
},""));
p("title", attempt(function(){return global.top.document.title;},""));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another top reference to wrap in try/catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attempt function wraps this callback in a try/catch - somewhere around line 78

@jstocker76
Copy link
Contributor Author

My bad, I've updated the bidder parameters to an endpoint and placement id combo that reliably return video bids

@protonate protonate merged commit 2e6d865 into prebid:master Mar 28, 2017
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request May 19, 2017
…19.0 to aolgithub-master

* commit '5109273bd4317535b23e26ff609345101a3d038d': (49 commits)
  Disable unit tests that fails on PhantomJs.
  Fixed unit tests for adapter loader.
  Changed way of including analytic adapters.
  Added adapters in aolPartnersIds.json.
  Added changelog entry.
  Replace removed utils.extend functionality by object.assign.
  Add Inneractive adapter (prebid#1048)
  Add alias freewheel-ssp to stickyadstv bidder adapter  (prebid#1043)
  Add Facebook Audience Network adapter (prebid#1068)
  Add Atomx support (prebid#1056)
  Updated rubicon video bid endpoint in source and test files (prebid#1097)
  Pass through params to server (prebid#1084)
  Reset the list of slots to be requested between each action for pubmatic (prebid#1079)
  Support for downloading Analytics Adapters via http://prebid.org/download.html (prebid#1021)
  update PR template to include link to dev docs page (prebid#1075)
  Prebid 0.21.0
  Lifestreet adapter: ignore unnecessary events from creative. (prebid#1054)
  Video header bidding support to RhythmOne bidder adapter (prebid#1052)
  Add rubicon targeting to rubicon bid responses for bidderSettings use (prebid#1045)
  Fix adapter getSize (prebid#1064)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments