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

Consolidate Dual render ad paths #8358

Closed
patmmccann opened this issue May 2, 2022 · 2 comments · Fixed by #9647
Closed

Consolidate Dual render ad paths #8358

patmmccann opened this issue May 2, 2022 · 2 comments · Fixed by #9647

Comments

@patmmccann
Copy link
Collaborator

patmmccann commented May 2, 2022

Type of issue

Two areas of the code base can render an ad

if (data && data.adId && data.message) {

and
$$PREBID_GLOBAL$$.renderAd = hook('async', function (doc, id, options) {

We've had various problems with these getting out of sync, including #7702 and potentially recording analytics events eg bid_won.

It seems best if we consolidate them, potentially by removing the latter and always forcing the post message into an iframe with the creative in it.

@GLStephen
Copy link
Collaborator

GLStephen commented May 11, 2022

I think the first path is for SafeFrames and the second is for non-safe frames. I'm not 100% sure one is compatible in the the environment of the other. I would agree with the desire to remove the "GLOBAL" path though.

@dgirardi
Copy link
Collaborator

dgirardi commented Feb 2, 2023

It's unclear to me how we should deal with some of the differences:

  • there is partial, awkward support for CLICKTHROUGH macro substitution (docs, also see this issue). It does not work, and with the current interface cannot work, on safeframe.
  • there's hair-raising scraper-targeted markup generated only from renderAd, again not from safeframes.
  • the new video module has some ad-hoc support only in renderAd. I think this might become a "normal" renderer (@karimMourra?)

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

Successfully merging a pull request may close this issue.

3 participants