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

Support SafeFrame / x-domain on prebid creatives #885

Merged
merged 9 commits into from
Jan 12, 2017

Conversation

protonate
Copy link
Collaborator

Type of change

  • Feature

Description of change

A creative served in a cross domain iframe can request the ad from the auction winner via postMessage.

};

var listenAdFromPrebid = function () {
window.addEventListener("message", renderAd, false);
Copy link
Member

Choose a reason for hiding this comment

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

IE support

message: 'Prebid creative requested: %%PATTERN:hb_adid%%',
adId: '%%PATTERN:hb_adid%%'
});
window.parent.postMessage(message, '*');
Copy link
Member

Choose a reason for hiding this comment

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

No way to avoid * here I guess?

@@ -289,6 +289,35 @@ function setRenderSize(doc, width, height) {
}
}

function listenAdRequestFromCreative() {
addEventListener('message', sendAdToCreative, false);
Copy link
Member

Choose a reason for hiding this comment

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

missing window?


var ad = adObject.ad;
var adUrl = adObject.adUrl;
var message = JSON.stringify({
Copy link
Member

Choose a reason for hiding this comment

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

prefer a factory pattern for creating messages.

@protonate protonate force-pushed the feature/x-domain-iframe branch from 4a3065a to d7f1f48 Compare January 5, 2017 17:39
var ad = adObject.ad;
var doc = window.document;

// if (doc === document || adObject.mediaType === 'video') {
Copy link
Member

Choose a reason for hiding this comment

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

conversation piece?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. This code is running in the x-domain iframe, so doc === document. Also utils module is not available so need to solve for a local console.log.

function requestAdFromPrebid() {
var message = JSON.stringify({
message: 'Prebid Request',
adId: window.prebidAdId
Copy link
Member

Choose a reason for hiding this comment

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

Where does prebidAdId come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The DFP code snippet needs to set the adId for the creative.js, and while developing using an external creative.js file this simply sticks it on the x-domain iframe window object.

message: 'Prebid Request',
adId: window.prebidAdId
});
window.parent.postMessage(message, '*');
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a marco for hostname? This could be set as a keyword by Prebid and then communication would be limited between those domains.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, indeed, I'll make the change.

…ins for postMessage (no wildcards), resize cross domain iframes
@protonate
Copy link
Collaborator Author

@mkendall07 this is ready for further review.

@protonate protonate assigned mkendall07 and unassigned protonate Jan 9, 2017
// creative will be rendered, e.g. DFP delivering a SafeFrame

const publisherDomain = 'http://localhost:9999';
const adServerDomain = 'http://tpc.googlesyndication.com';
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a note about making sure to match the protocol. I assume that this becomes HTTPS on secure pages?

@@ -0,0 +1,53 @@
// this script can be returned by an ad server delivering a cross domain iframe, into which the
// creative will be rendered, e.g. DFP delivering a SafeFrame
Copy link
Member

Choose a reason for hiding this comment

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

Can make a note about using a macro here instead of a hardcoded string. Many creatives will be delivered to multiple domains.

@@ -288,6 +291,58 @@ function setRenderSize(doc, width, height) {
}
}

function listenMessagesFromCreative() {
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this function into a separate file

@protonate
Copy link
Collaborator Author

@mkendall07 review notes addressed.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM.

@mkendall07 mkendall07 changed the title enable postMessage listener for cross-domain iframe support Support SafeFrame / x-domain on prebid creatives Jan 12, 2017
@mkendall07 mkendall07 merged commit 3ec356f into master Jan 12, 2017
@mkendall07 mkendall07 deleted the feature/x-domain-iframe branch January 12, 2017 19:08
Walexander pushed a commit to MbidIO/Prebid.js that referenced this pull request Mar 6, 2017
Support SafeFrame / x-domain on prebid creatives
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…ebid-official-0.18.0 to release/1.13.0

* commit 'e145489bc5dd6d292cf16e7fe80adfdf991562ac': (27 commits)
  Add changelog entry.
  Prebid 0.18.0 Release
  Add Criteo adapter (prebid#928)
  add an event that fires when requestBids is called (prebid#939)
  Xaxis adapter submitted by Daniel hoffmann (prebid#938)
  Add flash detection to TripleLift adapter (prebid#855)
  OpenX Adapter: Fixed bug regarding cross-domain iframe support (prebid#931)
  Emit event when setTargetingForGPTAsync is called (prebid#873)
  Maintenance/refactor hb deal (prebid#935)
  Reset hb_* keys only for registered aduniits (prebid#934)
  update code style - smartyads adapter
  Catch errors in bidsBackHandler.  Also fix test cleanup in pbjs api spec. (prebid#905)
  Smartyads Adapter (prebid#895)
  Appnexus targeting function (prebid#920)
  There are 2 changes- (prebid#913)
  Adding support for all AST parameters (prebid#923)
  GumGum adapter - include the bid timeout as `tmax` (prebid#908)
  Add pixel size (prebid#892)
  enable postMessage listener for cross-domain iframe support (prebid#885)
  Add Sharethrough adapter (prebid#865)
  ...
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…13.0 to master

* commit '7d32ed18c8636d9241ef8b299b6abd885536db69': (27 commits)
  Add changelog entry.
  Prebid 0.18.0 Release
  Add Criteo adapter (prebid#928)
  add an event that fires when requestBids is called (prebid#939)
  Xaxis adapter submitted by Daniel hoffmann (prebid#938)
  Add flash detection to TripleLift adapter (prebid#855)
  OpenX Adapter: Fixed bug regarding cross-domain iframe support (prebid#931)
  Emit event when setTargetingForGPTAsync is called (prebid#873)
  Maintenance/refactor hb deal (prebid#935)
  Reset hb_* keys only for registered aduniits (prebid#934)
  update code style - smartyads adapter
  Catch errors in bidsBackHandler.  Also fix test cleanup in pbjs api spec. (prebid#905)
  Smartyads Adapter (prebid#895)
  Appnexus targeting function (prebid#920)
  There are 2 changes- (prebid#913)
  Adding support for all AST parameters (prebid#923)
  GumGum adapter - include the bid timeout as `tmax` (prebid#908)
  Add pixel size (prebid#892)
  enable postMessage listener for cross-domain iframe support (prebid#885)
  Add Sharethrough adapter (prebid#865)
  ...
@StaymanHou
Copy link

Would the doc also be updated regarding the safeframe option?

Here it still says the safeframe box has to be unchecked. And the code snippet doesn't work for safeframe.

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.

3 participants