-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Criteo Bid Adapter: add iframe syncs support #8577
Conversation
Please note your failing test @afewcc |
207219b
to
2d09a84
Compare
Hello @ChrisHuie |
Should be able to review it in the next couple of days |
modules/criteoBidAdapter.js
Outdated
version: '$prebid.version$'.replace(/\./g, '_'), | ||
}; | ||
|
||
window.addEventListener('message', function (event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this leaves a dangling listener each time it's run; imo it should detach itself when it gets the matching requestId back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also curious: how does the sync figure out which window to message? this is the first time I see this approach and I'm wondering if it needs support from core for the general case (where prebid might be in a window other than top
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the sync frame, we simply try to send the message to window.parent (which should be the one listening to the message as it's the one which dropped the frame)
modules/criteoBidAdapter.js
Outdated
storage.removeDataFromLocalStorage(name); | ||
} | ||
|
||
function hasPurpose1Consent(gdprConsent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use this utility:
Line 9 in 0be4560
export function hasPurpose1Consent(gdprConsent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const refererInfo = getRefererInfo(); | ||
const origin = 'criteoPrebidAdapter'; | ||
|
||
if (syncOptions.iframeEnabled && hasPurpose1Consent(gdprConsent)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prebid.js/modules/pubmaticBidAdapter.js
Line 1300 in 0be4560
if (syncOptions.iframeEnabled) { |
why are you putting all this code in the adapter instead of the iframe you return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, you are trying to decorate the sync call with these id's, nevermind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the hasPurpose1Consent call
We really want the purpose1 to be granted prior to init our user sync as it involves interacting with 1PC the LS to resolve our sync endpoint arguments.
2d09a84
to
fcbab99
Compare
Co-authored-by: le.labat <[email protected]>
Co-authored-by: le.labat <[email protected]>
Co-authored-by: le.labat <[email protected]>
Type of change
Description of change
Added support for user sync through iframe as possible sync option
Contacts