Core: unify rendering paths; codify (and test) cross-domain render example #9647
Conversation
| try { | ||
| if (!adId || !doc) { | ||
| // eslint-disable-next-line standard/no-callback-literal | ||
| cb({ |
There was a problem hiding this comment.
we can add return cb({ ... and remove the else after
| } | ||
| const repl = { | ||
| AUCTION_PRICE: originalCpm || cpm, | ||
| CLICKTHROUGH: options?.clickUrl || '' |
There was a problem hiding this comment.
I've worked on prebid-unviersal-creative in the past to add clickUrlUnesc variable for native ads: a variable that, if is we are in GAM, it is substituted with GAM's %%CLICK_URL_UNESC%% that can be used in native templates to track clicks on them. This is the PR: prebid/prebid-universal-creative#196
I am adding this comment because we probably may want to unify the two things, in the future...
| .pipe(gulp.dest('build/creative')) | ||
| } | ||
|
|
||
| function updateCreativeExample(cb) { |
There was a problem hiding this comment.
what was the problem you wanted to solve with buildCreative and updateCreative ?
There was a problem hiding this comment.
they build the creative code and its example page respectively, using "real" code instead of what's in the current example (that means it shares what it can with safeframe / renderAd and is unit-tested).
The alternative is what we have right now - someone occasionally figures out what to put in the example, then we let it drift out of sync with the rest until someone notices something doesn't work anymore.
| reply(Object.assign({ | ||
| message: PREBID_RESPONSE, | ||
| }, adData)); | ||
| }, {options: message.option, adId: message.adId, bidResponse}); | ||
| } | ||
|
|
||
| function handleNativeRequest(reply, data, adObject) { |
There was a problem hiding this comment.
not related to this PR, but file secureCreatives.js is under 75% coverage
|
@musikele forgive me for having trouble following, have you finished your review and awaiting changes, or are your changes addressed? |
| events.emit(BID_WON, adObject); | ||
| function handleRenderRequest(reply, message, bidResponse) { | ||
| handleRender(function (adData) { | ||
| resizeRemoteCreative(bidResponse); |
There was a problem hiding this comment.
Would this resize for outstream renders? My assumption here is that it could if the bid has video dimensions defined as the width/height and could cause white space, but perhaps I am overlooking something
There was a problem hiding this comment.
video doesn't go through this, both pbjs.renderAd and PUC throw an error if they're asked to render a video bid - since they don't know the player to use or how to interact with it. The flow for it is very different and I think it wouldn't result in a GPT frame unless you set up the player in your creative.
Our guides for it do not use GPT at all and instead generate a VAST url that the pub is expected to submit to their player (or the new-ish video module can do it for them, but still without a separate creative iframe that could send messages here).
There was a problem hiding this comment.
I think I misunderstood your question - if an outstream renderer is in use and the frame here is therefore useless, are we resizing it to be large and empty? I will do some testing and report back.
There was a problem hiding this comment.
The answer is no, when bids have a renderer this code path is skipped - I added a test case for it.
There was a problem hiding this comment.
Ah yep I see. Thanks for confirming!
|
@dgirardi Does this PR also solve the native template rendering through |
Type of change
Description of change
This attempts to unify
pbjs.renderAdand "safe creative" rendering so that they share as much code as possible.Additionally, this adds
build/creative/creative.js, an optimized (and tested) version of this cross-domain example. The plan is to eventually re-use this from PUC or its successor.Other information
Closes #8358
Also related:
#6130
#9060